Prioritize Substrate txns by tip-per-effective-gas #1957

notlesh · 2022-11-16T18:49:08Z

What does it do?

This modifies our substrate txn priority-override to compute a "tip per effective gas" metric. This should make the prioritization mechanism comparable to what is currently being done in Frontier (which is solely based on the priority fee paid after considering base fee).

Frontier's calculation currently takes a transaction's maxFeePerGas and maxPriorityFeePerGas as well as the current block's baseFee into account, so it basically calculates the effective tip per-gas at any point in time.

There are probably some issues related to the txpool prioritization when it comes to txn size which are not addressed by this PR. They are discussed in the comments but I'm considering them out-of-scope and want to keep this PR minimal.

tests/tests/test-staking/test-staking-locks.ts

runtime/moonbase/src/lib.rs

nbaztec · 2023-01-30T14:57:30Z

tests/tests/test-txpool/test-txpool-fairness.ts

+      from: charleth.address,
+      privateKey: CHARLETH_PRIVATE_KEY,
+      maxFeePerGas: HIGH_MAX_FEE_PER_GAS,
+      maxPriorityFeePerGas: "0x" + TIP_PER_GAS_0.toString(16),


nToHex(TIP_PER_GAS_0) exists in polkadotjs for this operation.

Ah, thanks, this is definitely the cumbersome way to do it.

I was bothered enough by it that I made createTransaction optionally take a bigint here: #2066 -- I should take advantage of that in this PR :)

nbaztec · 2023-01-30T14:59:55Z

tests/tests/test-txpool/test-txpool-fairness.ts

+    });
+
+    expect(transferExts.length).to.eq(4);
+    expect(transferExts[0].method.section).to.eq("balances");


We should also assert that the correct TX type was ordered in accordance with the Tip and not just the type (since there are 2 of each we can't be sure that the appropriate tx was picked first)

tests/tests/test-txpool/test-txpool-fairness.ts

Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>

tests/tests/test-txpool/test-txpool-fairness.ts

Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>

tests/tests/test-staking/test-staking-locks.ts

Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>

nbaztec · 2023-01-30T15:10:37Z

tests/tests/test-staking/test-staking-locks.ts

+        [...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 });
+        })


This can be simplified byusing tip-- as be build the tx:

let tip = 1 * GLMR; await expectOk( context.createBlock( [...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 return context.polkadotApi.tx.parachainStaking .delegate(alith.address, MIN_GLMR_DELEGATOR + 1n * GLMR, i + 1, 1) .signAsync(account, { tip: tip-- }); }) ) );

nbaztec · 2023-01-30T15:11:19Z

tests/tests/test-staking/test-staking-locks.ts

+        [...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;


same as above

nbaztec · 2023-01-30T15:12:25Z

runtime/moonbase/src/lib.rs

-					// 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 {


We should also add an associated rust test for this validation function (if possible)

I added some tests, but they're not in good shape. The call to Executive::validate_transaction early in the fn fails on an unsigned UncheckedExtrinsic except for an Ethereum one, and signing one is proving difficult.

I'll revisit this again with a fresh mind, but we may also want to consider creating a task to do these tests later.

In any case, I agree that they're a good idea...

…on#1957) * Prioritize Substrate txns by tip-per-effective-gas * s/<spaces>/<tabs>/ * fmt * Remove unused import * Remove useless calls to setBlocksPerRound * wtf * Replace createBlock([]) with one block per extrinsic * A more performant workaround for txn ordering * prettier * Include some basic tests * Fix test * Add txn replacement tests * prettier * Don't assume nonce is 0 * Update tests/tests/test-txpool/test-txpool-fairness.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Update tests/tests/test-txpool/test-txpool-fairness.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Update tests/tests/test-txpool/test-txpool-fairness.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Update tests/tests/test-txpool/test-txpool-fairness.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Update tests/tests/test-staking/test-staking-locks.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Update tests/tests/test-staking/test-staking-locks.ts Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> * Add some (failing) tests around validate_transaction * Remove failing tests * Remove unused imports * Attempt to break up extremely long import line * Another attempt * Respect my authoritah --------- Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>

Prioritize Substrate txns by tip-per-effective-gas

c7bd1c5

notlesh added B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Nov 16, 2022

notlesh added 3 commits November 16, 2022 21:05

s/<spaces>/<tabs>/

d63b5c4

fmt

85fa317

Remove unused import

90667d9

notlesh added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Nov 16, 2022

notlesh added 2 commits November 16, 2022 23:20

Remove useless calls to setBlocksPerRound

c9a1dbd

wtf

3f08d1c

notlesh commented Nov 16, 2022

View reviewed changes

tests/tests/test-staking/test-staking-locks.ts Outdated Show resolved Hide resolved

Replace createBlock([]) with one block per extrinsic

5988d66

notlesh commented Nov 17, 2022

View reviewed changes

tests/tests/test-staking/test-staking-locks.ts Outdated Show resolved Hide resolved

nbaztec reviewed Nov 21, 2022

View reviewed changes

runtime/moonbase/src/lib.rs Show resolved Hide resolved

notlesh added 5 commits December 14, 2022 14:16

Merge branch 'master' into notlesh-fix-txpool-fairness

a7eb034

A more performant workaround for txn ordering

5ada814

Merge branch 'master' into notlesh-fix-txpool-fairness

a4eabc2

prettier

4179c44

Include some basic tests

644ad65

notlesh added the not-breaking Does not need to be mentioned in breaking changes label Jan 6, 2023

notlesh added 6 commits January 12, 2023 20:58

Fix test

9087f55

Merge branch 'master' into notlesh-fix-txpool-fairness

40bb3ca

Add txn replacement tests

d904ee5

prettier

27f3e83

Merge branch 'master' into notlesh-fix-txpool-fairness

eeb4482

Don't assume nonce is 0

68657b4