Prioritize Substrate txns by tip-per-effective-gas

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 =
 						<Runtime as pallet_evm::Config>::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
 				}
 			})

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::<Runtime>::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::*;

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)
     );