Build up to +65% more profitable blocks #7640

benaadams · 2024-10-21T16:01:38Z

Current Geth block (with forced local production for PR on Nethermind)

0.0145 Eth Fees | 3.41 MGas | 67 txs

PR

0.024 Eth Fees | 4.81 MGas | 69 txs

Current Nethermind block (with forced local production for PR on Nethermind)

0.0124 Eth fees | 3.88 MGas | 75 txs

PR

0.014 Eth fees | 5.82 MGas | 94 txs

Note: Doesn't compete with builders as 50% of mainnet orderflow is private and local builder doesn't have access to include in block

Changes

For first 65% of slot time build on timer so as not to over build; but also to have a good block ready if requested
For remaining slot trigger build every time a new tx arrives that is above base fee and not already building block
Cancel optimistic building on GetPayload (if a block is already built) and ForkChoice/NewPayload
Sort mempool tx by priority fee of first transaction by sender; maintain nonce order for a sender's txs and priority fee otherwise (e.g. nonce 2 with priority fee 100 doesn't go ahead of nonce 1 with priority fee 50)
If one of txs by a sender is invalid, too low basefee or jumps nonce, don't consider any of following txs by same sender with higher nonces; while considering tx replacement (same nonce, higher priority replaces but doesn't blank following)
Improve logging when building

Types of changes

What types of changes does your code introduce?

Optimization

Testing

Requires testing

Yes

If yes, did you write tests?

Yes - changed current

Notes on testing

Should also be run on testnets where node is doing a portion of local building

Documentation

Requires documentation update

No

Requires explanation in Release Notes

Yes

As above

LukaszRozmej

Extreamly complicated, potentially suboptimal, might have some unforseen issues. Not sure if works at all with parallel block building.

LukaszRozmej · 2024-10-24T07:01:28Z

src/Nethermind/Nethermind.Consensus/Processing/IBlockProcessor.cs

@@ -48,7 +50,7 @@ Block[] Process(

        public interface IBlockTransactionsExecutor
        {
-            TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec);
+            TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec, CancellationToken token);


Maybe to reduce boilerplate changes:

Suggested change

TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec, CancellationToken token);

TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec, CancellationToken token = default);

LukaszRozmej · 2024-10-24T07:02:08Z

src/Nethermind/Nethermind.Consensus/Processing/OneTimeProcessor.cs

+                if (token.IsCancellationRequested) return null;
+
+                return _processor.Process(block, options, tracer, token);


sounds like : ?

LukaszRozmej · 2024-10-24T07:06:30Z

...ethermind/Nethermind.Consensus/Processing/BlockProcessor.BlockProductionTransactionPicker.cs

@@ -123,6 +118,7 @@ public enum TxAction
        {
            Add,
            Skip,
+            Invalid,


please double check if we cover all the usages of results of this enum now

Looks like we potentially need change SimulateBridgeHelper

LukaszRozmej · 2024-10-24T07:18:40Z

src/Nethermind/Nethermind.Core/Extensions/CancellationTokenExtensions.cs

+                    catch
+                    {
+                        return false;
+                    }


Can you explain?

LukaszRozmej · 2024-10-24T07:20:15Z

src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.PayloadProduction.cs

+        chain.AddTransactions(BuildTransactions(chain, startingHead, TestItem.PrivateKeyC, TestItem.AddressA, 3, 10, out _, out _));
+        await Task.Delay(timePerSlot / 4);
+        chain.AddTransactions(BuildTransactions(chain, startingHead, TestItem.PrivateKeyC, TestItem.AddressA, 3, 10, out _, out _));
+        await Task.Delay(timePerSlot / 4);
+        chain.AddTransactions(BuildTransactions(chain, startingHead, TestItem.PrivateKeyC, TestItem.AddressA, 3, 10, out _, out _));
+        await Task.Delay(timePerSlot / 4);


Sounds like a loop, and maybe TestCases with different loop count?

LukaszRozmej · 2024-10-25T09:53:26Z

src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/PayloadPreparationService.cs

@@ -207,15 +299,32 @@ private void CleanupOldPayloads(object? sender, EventArgs e)
                bool currentBestBlockIsEmpty = blockContext.CurrentBestBlock?.Transactions.Length == 0;
                if (currentBestBlockIsEmpty && !blockContext.ImprovementTask.IsCompleted)
                {
-                    await Task.WhenAny(blockContext.ImprovementTask, Task.Delay(GetPayloadWaitForFullBlockMillisecondsDelay));
+                    await Task.WhenAny(blockContext.ImprovementTask, Task.Delay(GetPayloadWaitForFullBlockMillisecondsDelay, _tokenSource.Token));


Can throw now?

LukaszRozmej · 2024-10-25T09:56:47Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

-            int checkedTransactions = 0;
-            int selectedTransactions = 0;
-            using ArrayPoolList<Transaction> selectedBlobTxs = new(_eip4844Config.GetMaxBlobsPerBlock());
+            using ArrayPoolList<Transaction> selectedTxs = new(transactionSnapshot.Length + blobTransactionSnapshot.Length);


Suggested change

using ArrayPoolList<Transaction> selectedTxs = new(transactionSnapshot.Length + blobTransactionSnapshot.Length);

using ArrayPoolList<Transaction> selectedTxs = new(gasLimit/Transaction.BaseTxGasCost + _eip4844Config.GetMaxBlobsPerBlock());

LukaszRozmej · 2024-10-25T09:58:37Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

+                .Where(tx =>
+                    tx.SenderAddress is not null && // Ensure the transaction has a sender address
+                    _txFilterPipeline.Execute(tx, parent) && // Pass the transaction through the filter pipeline
+                    !tx.IsAboveInitCode(spec)) // Exclude transactions that exceed the init code size limit


why is it needed here?

LukaszRozmej · 2024-10-25T09:58:53Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

+                    tx.SenderAddress is not null && // Ensure the transaction has a sender address
+                    _txFilterPipeline.Execute(tx, parent) && // Pass the transaction through the filter pipeline
+                    !tx.IsAboveInitCode(spec)) // Exclude transactions that exceed the init code size limit
+                .DistinctBy((tx) => tx, ByHashTxComparer.Instance)); // Remove duplicate transactions based on their hash


why would there be duplicates in tx pool?

LukaszRozmej · 2024-10-25T10:06:13Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

+            // Sort transactions from the same sender by their nonce in ascending order
+            // Otherwise txs will be invalidated by not being in nonce order
+            SortMultiSendersByNonce(selectedTxs, parent, spec, token);
+            // Remove any transactions that have been invalidated or are no longer valid after sorting
+            RemoveBlankedTransactions(selectedTxs);

-                if (_logger.IsTrace) _logger.Trace($"Selected {tx.ToShortString()} to be potentially included in block.");
+            // Re-sort transactions to maintain the correct order, ensuring transactions from the same sender remain in nonce order
+            // As sorting by nonce may have put txs with lower priorty earlier
+            selectedTxs.AsSpan().Sort((tx0, tx1) =>
+                tx0.SenderAddress == tx1.SenderAddress ?
+                0 :  // If transactions are from the same sender, keep their current order
+                comparer.Compare(tx0, tx1)); // Otherwise, sort based on the main comparer

-                selectedTransactions++;
-                yield return tx;
-            }
+            // Remove blob transactions that cannot be included in the block (e.g. due to block limit constraints)
+            RemoveBlobTransactions(selectedTxs.AsSpan(), parent, spec);
+            // Remove any additional invalidated transactions after blob transaction removal
+            RemoveBlankedTransactions(selectedTxs);

-            if (selectedBlobTxs.Count > 0)
+            // Convert the list of selected transactions to an array for inclusion in the block
+            Transaction[] potentialTxs = selectedTxs.ToArray();
+
+            if (_logger.IsDebug) _logger.Debug($"Potentially selected {potentialTxs.Length} out of {checkedTransactions} pending transactions checked.");
+
+            // Return the selected transactions unless the operation was cancelled
+            return token.IsCancellationRequested ? Array.Empty<Transaction>() : potentialTxs;
+        }


Really dislike the solution:

Sort things multiple times

Allocating memory while previous solution was lazy.

Prioritizing GasPrice sorting over Nonce sorting first.

GetOrderedTransactions where close to optimal, lazy and more readable solution and I would prefer to keep it.

marcindsobczak · 2024-10-28T09:18:04Z

We have a gasBottleneck, which is 0 for non-ready txs and positive for ready ones. When you want to build an optimal block, you can just sort txs by gas bottleneck (it is actually default sorting) and just pick one by one up to the block gas limit (but only positive values)

marcindsobczak · 2024-10-28T09:22:54Z

From PR description:
Sort mempool tx by priority fee of first transaction by sender; maintain nonce order for a sender's txs and priority fee otherwise (e.g. nonce 2 with priority fee 100 doesn't go ahead of nonce 1 with priority fee 50)
It is already managed by gas bottleneck, you can just reuse it.
To be clear - if sender has tx with nonce N and price 10 gwei and tx with nonce N+1 and price 100 gwei, gas bottleneck of both is equal 10 gwei and both will be included before e.g. tx from other sender, with nonce first to be executed, but with price 5 gwei

LukaszRozmej · 2024-10-31T09:02:35Z

From PR description: Sort mempool tx by priority fee of first transaction by sender; maintain nonce order for a sender's txs and priority fee otherwise (e.g. nonce 2 with priority fee 100 doesn't go ahead of nonce 1 with priority fee 50) It is already managed by gas bottleneck, you can just reuse it. To be clear - if sender has tx with nonce N and price 10 gwei and tx with nonce N+1 and price 100 gwei, gas bottleneck of both is equal 10 gwei and both will be included before e.g. tx from other sender, with nonce first to be executed, but with price 5 gwei

I think this won't build optimal blocks, so for example it is better to include transactions with priority fee 10 and next 100, than the one from other sender with fee 20, but that 2nd one will win on gasBottleneck

marcindsobczak · 2024-11-01T12:04:52Z

From PR description: Sort mempool tx by priority fee of first transaction by sender; maintain nonce order for a sender's txs and priority fee otherwise (e.g. nonce 2 with priority fee 100 doesn't go ahead of nonce 1 with priority fee 50) It is already managed by gas bottleneck, you can just reuse it. To be clear - if sender has tx with nonce N and price 10 gwei and tx with nonce N+1 and price 100 gwei, gas bottleneck of both is equal 10 gwei and both will be included before e.g. tx from other sender, with nonce first to be executed, but with price 5 gwei

I think this won't build optimal blocks, so for example it is better to include transactions with priority fee 10 and next 100, than the one from other sender with fee 20, but that 2nd one will win on gasBottleneck

Right, as long as all of them are able to pay for base fee. In this example - tx with fee 10 must be able to pay base fee by itself to make it possible to process tx with fee 100

benaadams added 2 commits October 21, 2024 17:02

Production only accept better blocks

9a468a1

Build better blocks

8f78a67

benaadams force-pushed the block-production-improve branch from abc9ef8 to 8f78a67 Compare October 21, 2024 16:02

benaadams added 16 commits October 22, 2024 07:46

Fix mocks

dc57905

Fix null ref

9ebbcb8

Test

41cb23b

Fix

767bb42

fix

fb7935e

Fix

f8a2ce3

fix

8c0a042

Formatting

2a6bf9c

Merge branch 'master' into block-production-improve

8e159b8

Merge branch 'master' into block-production-improve

c8e0335

Fix

436c2ae

Merge branch 'master' into block-production-improve

0480f50

Merge conflict

7be37f3

Range

cfc5b1a

Tweak

43bd035

65% of block

c5d1dc1

benaadams changed the title ~~Build more profitable blocks~~ Build up to +65% more profitable blocks Oct 23, 2024

Support tx replacement better

51058b3

benaadams marked this pull request as ready for review October 23, 2024 15:54

benaadams added 4 commits October 24, 2024 15:23

Merge branch 'master' into block-production-improve

e0da0f1

Merge branch 'master' into block-production-improve

9a3c958

Merge branch 'master' into block-production-improve

cd85121

Merge branch 'master' into block-production-improve

cbf30c8

LukaszRozmej requested changes Oct 25, 2024

View reviewed changes

LukaszRozmej requested a review from marcindsobczak October 25, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build up to +65% more profitable blocks #7640

Build up to +65% more profitable blocks #7640

benaadams commented Oct 21, 2024 •

edited

Loading

LukaszRozmej left a comment

LukaszRozmej Oct 24, 2024

LukaszRozmej Oct 24, 2024

LukaszRozmej Oct 24, 2024

LukaszRozmej Oct 25, 2024

LukaszRozmej Oct 24, 2024

LukaszRozmej Oct 24, 2024

LukaszRozmej Oct 25, 2024

LukaszRozmej Oct 25, 2024

LukaszRozmej Oct 25, 2024

LukaszRozmej Oct 25, 2024

LukaszRozmej Oct 25, 2024

marcindsobczak commented Oct 28, 2024

marcindsobczak commented Oct 28, 2024

LukaszRozmej commented Oct 31, 2024

marcindsobczak commented Nov 1, 2024

	TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec, CancellationToken token);
	TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processingOptions, BlockReceiptsTracer receiptsTracer, IReleaseSpec spec, CancellationToken token = default);

		if (token.IsCancellationRequested) return null;

		return _processor.Process(block, options, tracer, token);

	using ArrayPoolList<Transaction> selectedTxs = new(transactionSnapshot.Length + blobTransactionSnapshot.Length);
	using ArrayPoolList<Transaction> selectedTxs = new(gasLimit/Transaction.BaseTxGasCost + _eip4844Config.GetMaxBlobsPerBlock());

Build up to +65% more profitable blocks #7640

Are you sure you want to change the base?

Build up to +65% more profitable blocks #7640

Conversation

benaadams commented Oct 21, 2024 • edited Loading

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

LukaszRozmej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcindsobczak commented Oct 28, 2024

marcindsobczak commented Oct 28, 2024

LukaszRozmej commented Oct 31, 2024

marcindsobczak commented Nov 1, 2024

benaadams commented Oct 21, 2024 •

edited

Loading