diff --git a/runtime/moonbase/src/lib.rs b/runtime/moonbase/src/lib.rs index 5f3da5c1452..392764a28fe 100644 --- a/runtime/moonbase/src/lib.rs +++ b/runtime/moonbase/src/lib.rs @@ -87,7 +87,7 @@ use sp_runtime::{ transaction_validity::{ InvalidTransaction, TransactionSource, TransactionValidity, TransactionValidityError, }, - ApplyExtrinsicResult, FixedPointNumber, Perbill, Permill, Perquintill, SaturatedConversion, + ApplyExtrinsicResult, FixedPointNumber, Perbill, Permill, Perquintill, }; use sp_std::{ convert::{From, Into}, @@ -1397,8 +1397,10 @@ moonbeam_runtime_common::impl_runtime_apis_plus_common! { let dispatch_info = xt.get_dispatch_info(); // If this is a pallet ethereum transaction, then its priority is already set - // according to gas price from pallet ethereum. If it is any other kind of transaction, - // we modify its priority. + // according to effective priority fee from pallet ethereum. If it is any other kind of + // transaction, we modify its priority. The goal is to arrive at a similar metric used + // by pallet ethereum, which means we derive a fee-per-gas from the txn's tip and + // weight. Ok(match &xt.0.function { RuntimeCall::Ethereum(transact { .. }) => intermediate_valid, _ if dispatch_info.class != DispatchClass::Normal => intermediate_valid, @@ -1412,31 +1414,18 @@ moonbeam_runtime_common::impl_runtime_apis_plus_common! { } }; - // Calculate the fee that will be taken by pallet transaction payment - let fee: u64 = TransactionPayment::compute_fee( - xt.encode().len() as u32, - &dispatch_info, - tip, - ).saturated_into(); - - // Calculate how much gas this effectively uses according to the existing mapping let effective_gas = ::GasWeightMapping::weight_to_gas( dispatch_info.weight ); - - // Here we calculate an ethereum-style effective gas price using the - // current fee of the transaction. Because the weight -> gas conversion is - // lossy, we have to handle the case where a very low weight maps to zero gas. - let effective_gas_price = if effective_gas > 0 { - fee / effective_gas + let tip_per_gas = if effective_gas > 0 { + tip.saturating_div(effective_gas as u128) } else { - // If the effective gas was zero, we just act like it was 1. - fee + 0 }; // Overwrite the original prioritization with this ethereum one - intermediate_valid.priority = effective_gas_price; + intermediate_valid.priority = tip_per_gas as u64; intermediate_valid } }) diff --git a/runtime/moonbase/tests/integration_test.rs b/runtime/moonbase/tests/integration_test.rs index 38972f093bd..38d770f7b47 100644 --- a/runtime/moonbase/tests/integration_test.rs +++ b/runtime/moonbase/tests/integration_test.rs @@ -3103,6 +3103,36 @@ fn evm_success_keeps_substrate_events() { }); } +#[test] +fn validate_transaction_fails_on_filtered_call() { + use sp_runtime::transaction_validity::{ + InvalidTransaction, TransactionSource, TransactionValidityError, + }; + use sp_transaction_pool::runtime_api::runtime_decl_for_TaggedTransactionQueue::TaggedTransactionQueueV3; // editorconfig-checker-disable-line + + ExtBuilder::default().build().execute_with(|| { + let xt = UncheckedExtrinsic::new_unsigned( + pallet_evm::Call::::call { + source: Default::default(), + target: H160::default(), + input: Vec::new(), + value: Default::default(), + gas_limit: Default::default(), + max_fee_per_gas: Default::default(), + max_priority_fee_per_gas: Default::default(), + nonce: Default::default(), + access_list: Default::default(), + } + .into(), + ); + + assert_eq!( + Runtime::validate_transaction(TransactionSource::External, xt, Default::default(),), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); + }); +} + #[cfg(test)] mod fee_tests { use super::*; diff --git a/tests/tests/test-staking/test-staking-locks.ts b/tests/tests/test-staking/test-staking-locks.ts index 2882e4ce154..39ab34d674b 100644 --- a/tests/tests/test-staking/test-staking-locks.ts +++ b/tests/tests/test-staking/test-staking-locks.ts @@ -8,7 +8,13 @@ import { execTechnicalCommitteeProposal, notePreimage, } from "../../util/governance"; -import { GLMR, MIN_GLMR_STAKING, MIN_GLMR_DELEGATOR } from "../../util/constants"; +import { + MICROGLMR, + MILLIGLMR, + GLMR, + MIN_GLMR_STAKING, + MIN_GLMR_DELEGATOR, +} from "../../util/constants"; import { describeDevMoonbeam, DevTestContext } from "../../util/setup-dev-tests"; import { KeyringPair } from "@polkadot/keyring/types"; import { expectOk } from "../../util/expect"; @@ -200,9 +206,6 @@ describeDevMoonbeam("Staking - Locks - execute revoke", (context) => { before("setup account balance", async function () { await expectOk( context.createBlock([ - context.polkadotApi.tx.sudo.sudo( - context.polkadotApi.tx.parachainStaking.setBlocksPerRound(5) - ), context.polkadotApi.tx.balances.transfer( randomAccount.address, MIN_GLMR_DELEGATOR + 1n * GLMR @@ -258,9 +261,6 @@ describeDevMoonbeam("Staking - Locks - multiple delegations single revoke", (con await expectOk( context.createBlock([ - context.polkadotApi.tx.sudo.sudo( - context.polkadotApi.tx.parachainStaking.setBlocksPerRound(5) - ), context.polkadotApi.tx.balances.transfer(randomAccount.address, 2n * MIN_GLMR_STAKING), context.polkadotApi.tx.parachainStaking .joinCandidates(MIN_GLMR_STAKING, 1) @@ -516,6 +516,11 @@ describeDevMoonbeam("Staking - Locks - bottom and top delegations", (context) => before("setup candidate & delegations", async function () { this.timeout(20000); + const numBottomDelegations = + await context.polkadotApi.consts.parachainStaking.maxBottomDelegationsPerCandidate.toNumber(); + + const numTopDelegations = + await context.polkadotApi.consts.parachainStaking.maxTopDelegationsPerCandidate.toNumber(); // Create the delegators to fill the lists bottomDelegators = new Array( @@ -533,7 +538,7 @@ describeDevMoonbeam("Staking - Locks - bottom and top delegations", (context) => context.createBlock( [...bottomDelegators, ...topDelegators].map((account, i) => context.polkadotApi.tx.balances - .transfer(account.address, MIN_GLMR_DELEGATOR + 2n * GLMR) + .transfer(account.address, MIN_GLMR_DELEGATOR + 20n * GLMR) .signAsync(alith, { nonce: i }) ) ) @@ -543,39 +548,61 @@ describeDevMoonbeam("Staking - Locks - bottom and top delegations", (context) => it("should be set for bottom and top list delegators", async function () { await expectOk( context.createBlock( - [...topDelegators].map((account, i) => - context.polkadotApi.tx.parachainStaking - .delegate(alith.address, MIN_GLMR_DELEGATOR + 1n * GLMR, topDelegators.length + 1, 1) - .signAsync(account) - ) + [...topDelegators].map((account, i) => { + // add a tip such that the delegation ordering will be preserved, e.g. the first txns sent + // will have the highest tip + let tip = BigInt(topDelegators.length - i + 1) * MILLIGLMR; + return context.polkadotApi.tx.parachainStaking + .delegate(alith.address, MIN_GLMR_DELEGATOR + 1n * GLMR, i + 1, 1) + .signAsync(account, { tip }); + }) ) ); + + // allow more block(s) for txns to be processed... + // note: this only seems necessary when a tip is added, otherwise all 300 txns make it into a + // single block. A tip is necessary if the txns are not otherwise executed in order of + // submission, which is highly dependent on txpool prioritization logic. + // TODO: it would be good to diagnose this further: why does adding a tip appear to reduce the + // number of txns included? + const numBlocksToWait = 1; + let numBlocksWaited = 0; + while (numBlocksWaited < numBlocksToWait) { + await context.createBlock(); + const topLocks = await context.polkadotApi.query.balances.locks.multi( + topDelegators.map((delegator) => delegator.address) + ); + let numDelegatorLocks = topLocks.filter((lockSet) => + lockSet.find((lock) => lock.id.toHuman().toString() == "stkngdel") + ).length; + + if (numDelegatorLocks < topDelegators.length) { + numBlocksWaited += 1; + expect(numBlocksWaited).to.be.lt( + numBlocksToWait, + "Top delegation extrinsics not included in time" + ); + } else { + expect(numDelegatorLocks).to.eq(topDelegators.length, "More delegations than expected"); + break; + } + } + await expectOk( context.createBlock( - [...bottomDelegators].map((account, i) => - context.polkadotApi.tx.parachainStaking - .delegate( - alith.address, - MIN_GLMR_DELEGATOR, - topDelegators.length + bottomDelegators.length + 1, - 1 - ) - .signAsync(account) - ) + [...bottomDelegators].map((account, i) => { + // add a tip such that the delegation ordering will be preserved, e.g. the first txns sent + // will have the highest tip + let tip = BigInt(topDelegators.length + (bottomDelegators.length - i) + 1) * MILLIGLMR; + return context.polkadotApi.tx.parachainStaking + .delegate(alith.address, MIN_GLMR_DELEGATOR, topDelegators.length + i + 1, 1) + .signAsync(account, { tip }); + }) ) ); - const topLocks = await context.polkadotApi.query.balances.locks.multi( - topDelegators.map((delegator) => delegator.address) - ); - expect( - topLocks.filter((lockSet) => - lockSet.find((lock) => lock.id.toHuman().toString() == "stkngdel") - ).length - ).to.equal( - context.polkadotApi.consts.parachainStaking.maxTopDelegationsPerCandidate.toNumber() - ); - + // note that we don't need to wait for further blocks here because bottom delegations is much + // smaller than top delegations, so all txns reliably fit within one block. const bottomLocks = await context.polkadotApi.query.balances.locks.multi( bottomDelegators.map((delegator) => delegator.address) ); diff --git a/tests/tests/test-txpool/test-txpool-fairness.ts b/tests/tests/test-txpool/test-txpool-fairness.ts new file mode 100644 index 00000000000..15813ad3be6 --- /dev/null +++ b/tests/tests/test-txpool/test-txpool-fairness.ts @@ -0,0 +1,272 @@ +import "@moonbeam-network/api-augment"; + +import { expect } from "chai"; +import Web3 from "web3"; + +import { + alith, + baltathar, + charleth, + CHARLETH_PRIVATE_KEY, + dorothy, + DOROTHY_PRIVATE_KEY, + ethan, + generateKeyringPair, +} from "../../util/accounts"; +import { verifyLatestBlockFees } from "../../util/block"; +import { customWeb3Request } from "../../util/providers"; +import { describeDevMoonbeam } from "../../util/setup-dev-tests"; +import { MILLIGLMR, GLMR, WEIGHT_PER_GAS } from "../../util/constants"; +import { createTransfer } from "../../util/transactions"; + +// for Ethereum txns, we need to send the tip as per-gas so there is no conversion necessary. +// However, we need to specify a maxFeePerGas that is high enough to allow the priority fee to +// be used as-is, e.g. it must be at least (block.baseFee + maxPriorityFeePerGas) +const HIGH_MAX_FEE_PER_GAS = "0x" + GLMR.toString(16); + +describeDevMoonbeam("Tip should be respected", (context) => { + it("should prefer txn with higher tip", async function () { + const NO_TIP = 0n; + const MED_TIP = 5n * MILLIGLMR; + const HIGH_TIP = 20n * MILLIGLMR; + + await context.polkadotApi.tx.balances + .transfer(dorothy.address, GLMR) + .signAndSend(alith, { tip: NO_TIP }); + + await context.polkadotApi.tx.balances + .transfer(dorothy.address, GLMR) + .signAndSend(baltathar, { tip: MED_TIP }); + + await context.polkadotApi.tx.balances + .transfer(dorothy.address, GLMR) + .signAndSend(charleth, { tip: HIGH_TIP }); + + const result = await context.createBlock(); + const hash = result.block.hash; + const apiAt = await context.polkadotApi.at(hash); + const { block } = await context.polkadotApi.rpc.chain.getBlock(hash); + + // filter out inherent extrinsics, which should leave us with the ones we sent in their + // inclusion order + const transferExts = block.extrinsics.filter( + (ext) => ext.signer.toHex() !== "0x0000000000000000000000000000000000000000" + ); + + expect(transferExts.length).to.eq(3); + expect(transferExts[0].tip.toBigInt()).to.eq(HIGH_TIP); + expect(transferExts[1].tip.toBigInt()).to.eq(MED_TIP); + expect(transferExts[2].tip.toBigInt()).to.eq(NO_TIP); + }); + + it("should treat eth and substrate txns fairly", async function () { + context.ethTransactionType = "EIP1559"; + + // tip 1 and 3 will be substrate txns, we express their tip above as per-gas but must send them + // expressed as a flat tip. So we query the weight and convert to gas, then multiply by the + // per-gas tip. We do this because it's the inverse of the txpool algo, and we want to show that + // this algo will order txns by tip in this test. + // + // so the expected order is: + // tip_0 (eth) + // tip_1 (substrate) + // tip_2 (eth) + // tip_3 (substrate) + const TIP_PER_GAS_0 = 10000n; + const TIP_PER_GAS_1 = 20000n; + const TIP_PER_GAS_2 = 30000n; + const TIP_PER_GAS_3 = 40000n; + + // here we query the weight of a substrate balance transfer + const dummyTransfer = context.polkadotApi.tx.balances.transfer(alith.address, GLMR); + const info = await context.polkadotApi.rpc.payment.queryInfo(dummyTransfer.toHex()); + const weight = info.weight.toBigInt(); + const balances_transfer_effective_gas = weight / WEIGHT_PER_GAS; + + // tx0 is an eth txn + const tx0 = await createTransfer(context, ethan.address, 1, { + from: charleth.address, + privateKey: CHARLETH_PRIVATE_KEY, + maxFeePerGas: HIGH_MAX_FEE_PER_GAS, + maxPriorityFeePerGas: "0x" + TIP_PER_GAS_0.toString(16), + }); + + // tx1 is a substrate txn + const tx1 = await context.polkadotApi.tx.balances + .transfer(ethan.address, GLMR) + .signAsync(alith, { tip: TIP_PER_GAS_1 * balances_transfer_effective_gas }); + + // tx2 is an eth txn + const tx2 = await createTransfer(context, ethan.address, 1, { + from: dorothy.address, + privateKey: DOROTHY_PRIVATE_KEY, + maxFeePerGas: HIGH_MAX_FEE_PER_GAS, + maxPriorityFeePerGas: "0x" + TIP_PER_GAS_2.toString(16), + }); + + // tx3 is a substrate txn + const tx3 = await context.polkadotApi.tx.balances + .transfer(ethan.address, GLMR) + .signAsync(baltathar, { tip: TIP_PER_GAS_3 * balances_transfer_effective_gas }); + + const result = await context.createBlock([ + // use an order other than by priority + tx2, + tx3, + tx0, + tx1, + ]); + + // get and filter the block's extrinsics + const hash = result.block.hash; + const apiAt = await context.polkadotApi.at(hash); + const { block } = await context.polkadotApi.rpc.chain.getBlock(hash); + const transferExts = block.extrinsics.filter((ext) => { + return ( + (ext.method.section == "balances" && ext.method.method == "transfer") || + (ext.method.section == "ethereum" && ext.method.method == "transact") + ); + }); + + expect(transferExts.length).to.eq(4); + expect(transferExts[0].method.section).to.eq("balances"); + expect(transferExts[1].method.section).to.eq("ethereum"); + expect(transferExts[2].method.section).to.eq("balances"); + expect(transferExts[3].method.section).to.eq("ethereum"); + }); + + it("should allow Substrate txn replacement with higher priority", async function () { + const LOW_TIP = 10n * MILLIGLMR; + const HIGH_TIP = 20n * MILLIGLMR; + + const nonce = await context.web3.eth.getTransactionCount(alith.address); + + await context.polkadotApi.tx.balances + .transfer(dorothy.address, GLMR) + .signAndSend(alith, { tip: LOW_TIP, nonce }); + + await context.polkadotApi.tx.system.remark("").signAndSend(alith, { tip: HIGH_TIP, nonce }); + + const result = await context.createBlock(); + const hash = result.block.hash; + const apiAt = await context.polkadotApi.at(hash); + const { block } = await context.polkadotApi.rpc.chain.getBlock(hash); + + // filter out inherent extrinsics, which should leave us with the ones we sent in their + // inclusion order + const txnExts = block.extrinsics.filter( + (ext) => ext.signer.toHex() !== "0x0000000000000000000000000000000000000000" + ); + + expect(txnExts.length).to.eq(1); + expect(txnExts[0].tip.toBigInt()).to.eq(HIGH_TIP); + }); + + it("should allow Ethereum txn replacement with higher priority", async function () { + context.ethTransactionType = "EIP1559"; + + const LOW_TIP = 10n * MILLIGLMR; + const HIGH_TIP = 20n * MILLIGLMR; + + const randomAccount = generateKeyringPair(); + const randomAccount2 = generateKeyringPair(); + + const nonce = await context.web3.eth.getTransactionCount(alith.address); + + // create a txn we don't expect to execute (because it will be replaced). it would send some + // funds to randomAccount + await customWeb3Request(context.web3, "eth_sendRawTransaction", [ + await createTransfer(context, randomAccount.address, 1, { + maxFeePerGas: HIGH_MAX_FEE_PER_GAS, + maxPriorityFeePerGas: "0x" + LOW_TIP.toString(16), + nonce, + }), + ]); + + // replace with a transaction that sends funds to a different account + await customWeb3Request(context.web3, "eth_sendRawTransaction", [ + await createTransfer(context, randomAccount2.address, 1, { + maxFeePerGas: HIGH_MAX_FEE_PER_GAS, + maxPriorityFeePerGas: "0x" + HIGH_TIP.toString(16), + nonce, + }), + ]); + + const result = await context.createBlock(); + + const account1Balance = ( + await context.polkadotApi.query.system.account(randomAccount.address.toString()) + ).data.free.toBigInt(); + const account2Balance = ( + await context.polkadotApi.query.system.account(randomAccount2.address.toString()) + ).data.free.toBigInt(); + + expect(account1Balance).to.eq(0n); + expect(account2Balance).to.eq(1n); + }); + + it("should allow Ethereum txn replacement with Substrate txn", async function () { + const randomAccount = generateKeyringPair(); + const randomAccount2 = generateKeyringPair(); + + const nonce = await context.web3.eth.getTransactionCount(alith.address); + + // create a txn we don't expect to execute (because it will be replaced). it would send some + // funds to randomAccount + await customWeb3Request(context.web3, "eth_sendRawTransaction", [ + await createTransfer(context, randomAccount.address, 1, { nonce }), + ]); + + // replace with a transaction that sends funds to a different account + await context.polkadotApi.tx.balances + .transfer(randomAccount2.address, 1) + .signAndSend(alith, { nonce, tip: GLMR }); + + const result = await context.createBlock(); + + const account1Balance = ( + await context.polkadotApi.query.system.account(randomAccount.address.toString()) + ).data.free.toBigInt(); + const account2Balance = ( + await context.polkadotApi.query.system.account(randomAccount2.address.toString()) + ).data.free.toBigInt(); + + expect(account1Balance).to.eq(0n); + expect(account2Balance).to.eq(1n); + }); + + it("should allow Substrate txn replacement with Ethereum txn", async function () { + context.ethTransactionType = "EIP1559"; + const randomAccount = generateKeyringPair(); + const randomAccount2 = generateKeyringPair(); + + const nonce = await context.web3.eth.getTransactionCount(alith.address); + + // create a txn we don't expect to execute (because it will be replaced). it would send some + // funds to randomAccount + await context.polkadotApi.tx.balances + .transfer(randomAccount.address, 1) + .signAndSend(alith, { nonce, tip: 0 }); + + // replace with a transaction that sends funds to a different account + await customWeb3Request(context.web3, "eth_sendRawTransaction", [ + await createTransfer(context, randomAccount2.address, 1, { + maxFeePerGas: HIGH_MAX_FEE_PER_GAS, + maxPriorityFeePerGas: "0x1", + nonce, + }), + ]); + + const result = await context.createBlock(); + + const account1Balance = ( + await context.polkadotApi.query.system.account(randomAccount.address.toString()) + ).data.free.toBigInt(); + const account2Balance = ( + await context.polkadotApi.query.system.account(randomAccount2.address.toString()) + ).data.free.toBigInt(); + + expect(account1Balance).to.eq(0n); + expect(account2Balance).to.eq(1n); + }); +});