miner: only fill the pending block pre-merge #28440

MariusVanDerWijden · 2023-10-31T09:22:00Z

This PR changes the way the pending block is constructed.
It will leave the pending block empty after the merge.

The pending block still behaves correctly (e.g. increased block number etc), but it is just constructed as if the node has no transactions available.

The only issue I see with this approach is that some people might rely on the pending state for nonce tracking. However you can track the pending nonces via eth_getTransactionCount which uses the pendingNoncer in the txpool instead of the pending state.

rjl493456442 · 2023-10-31T12:35:24Z

miner/worker.go

@@ -1068,35 +1068,39 @@ func (w *worker) commitWork(interrupt *atomic.Int32, timestamp int64) {
 	if err != nil {
 		return
 	}
-	// Fill pending transactions from the txpool into the block.


Note, this change will break the subscription(pending transactions and pending logs)

Oh yeah, didn't think about that

Hmm from what I can tell the pending transaction subscription goes via the txpool not the miner
Only the pending logs are retrieved via the miner

You are right. It might be a breaking change imo, I guess some MEV guys rely on the pending logs. Just want to mention here, not to block the pull request.

Interesting point about pending logs. I think it's fair to remove them since they will be able to get this on demand via multicall.

How can we obtain the pending logs via multicall though?

Originally people can subscribe the pending logs with or without filter criteria. The target is the pending transactions in the txpool which will have very high chance to be included in the next block.

But for multicall, the transactions/calls are constructed by the users themselves, the target is different right?

They can fetch list of pending transactions and pass them to multicall. Tho yes there will be no filtering on the server-side. But to your point, yes they will not know which txes are more likely to be mined. Because we simply return a flat list of transactions as opposed to a weighted list. Perhaps that's something that can be changed?

MariusVanDerWijden · 2023-11-15T15:20:57Z

It would be good to benchmark this to see the impact of prefetching done by creating the pending block

MariusVanDerWijden · 2023-11-24T11:09:25Z

Some stats:

Looks like CPU went down significantly

Block processing is not impacted significantly

The prefetcher stats, the miner drops from ~30 ops/s, most of them duplicates, and most of them wastes down to 0.

MariusVanDerWijden · 2023-11-27T12:31:19Z

One thing that changed between this PR and master is that snapshot storage read went from 5.9ms to 12.9ms. All other metrics stayed the same, execution actually went down from 45ms to 38ms

s1na · 2023-11-28T08:06:10Z

ethclient/ethclient_test.go

-	if pending != 1 {
-		t.Fatalf("unexpected pending, wanted 1 got: %v", pending)
+	// After nerfing the pending block, pending transaction count will be 0
+	if pending != 0 {


Why is the pending nonce changed here? I thought that's fetched through txpool.

This is not the pending nonce, but the pending transaction count. The pending nonce does not change because its fetched through the txpool. The pending transaction count returns the number of transactions in a block. I don't think it is a useful metric at all for the pending block

Argh I thought this is a call to eth_getTransactionCount. That method name is confusing.

s1na · 2023-11-28T08:08:52Z

Some eth protocol tests are failing. If they are flaky I haven't seem them fail before:

--- FAIL: TestEthSuite (11.07s)
    --- FAIL: TestEthSuite/TestBroadcast (10.01s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestLargeAnnounce (0.06s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestOldAnnounce (0.00s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestBlockHashAnnounce (0.00s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestMaliciousStatus (0.01s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestTransaction (0.00s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestMaliciousTx (0.06s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestLargeTxRequest (0.01s)
        suite_test.go:53: 
    --- FAIL: TestEthSuite/TestNewPooledTxs (0.00s)
        suite_test.go:53: 
FAIL
FAIL	github.com/ethereum/go-ethereum/cmd/devp2p/internal/ethtest	11.100s

MariusVanDerWijden · 2023-11-28T10:26:54Z

Weird, it works on my AMD64 Linux machine, I think these might be flaky and if the first one fails, all subsequent ones fail as well

MariusVanDerWijden · 2023-11-28T12:49:50Z

During the refactor of the miner package I kinda came to the conclusion that it would be possible to just create the pending block (with transactions) on the fly when requested. This is kind-of a costly operation but it would not break any users

holiman · 2023-11-28T12:57:20Z

If they are flaky I haven't seem them fail before:

I've seen those fail from time to time, they're flaky

holiman · 2023-11-28T13:00:16Z

ethclient/ethclient_test.go

@@ -598,8 +598,9 @@ func testAtFunctions(t *testing.T, client *rpc.Client) {
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if pending != 1 {
-		t.Fatalf("unexpected pending, wanted 1 got: %v", pending)
+	// After nerfing the pending block, pending transaction count will be 0


Is this what you mean? Or do you mean 'yeet'?

I meant nerf as in weaken. The pending block is still constructed, just without any transactions

MariusVanDerWijden · 2023-11-29T09:55:34Z

I'm going to close this for now. During the bigger refactor of the miner I noticed that we could just produce the pending block on demand. This will be a bit heavier for people depending on it, but it won't bork people depending on the behavior. And it has the same benefit as this PR as most users will not have to compute the pending block at all.

A good learning from this PR though is that the block processing is not impacted much by the prefetching done during construction of the pending block. And not computing the pending block will save a lot of CPU

miner: only fill the pending block pre-merge

ab0d224

rjl493456442 reviewed Oct 31, 2023

View reviewed changes

MariusVanDerWijden marked this pull request as ready for review November 21, 2023 11:53

s1na mentioned this pull request Nov 24, 2023

Deprecating the pending state ethereum/execution-apis#495

Open

miner: don't update the pending block, fix tests

72f4467

s1na reviewed Nov 28, 2023

View reviewed changes

holiman reviewed Nov 28, 2023

View reviewed changes

MariusVanDerWijden closed this Nov 29, 2023

MariusVanDerWijden mentioned this pull request Mar 6, 2024

miner: refactor the miner, make the pending block on demand #28623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

miner: only fill the pending block pre-merge #28440

miner: only fill the pending block pre-merge #28440

MariusVanDerWijden commented Oct 31, 2023 •

edited

Loading

rjl493456442 Oct 31, 2023

MariusVanDerWijden Oct 31, 2023

MariusVanDerWijden Oct 31, 2023 •

edited

Loading

rjl493456442 Oct 31, 2023

s1na Nov 24, 2023

rjl493456442 Nov 24, 2023

s1na Nov 24, 2023

MariusVanDerWijden commented Nov 15, 2023

MariusVanDerWijden commented Nov 24, 2023

MariusVanDerWijden commented Nov 27, 2023

s1na Nov 28, 2023

MariusVanDerWijden Nov 28, 2023

s1na Nov 29, 2023

s1na commented Nov 28, 2023

MariusVanDerWijden commented Nov 28, 2023 •

edited

Loading

MariusVanDerWijden commented Nov 28, 2023

holiman commented Nov 28, 2023

holiman Nov 28, 2023

MariusVanDerWijden Nov 28, 2023

MariusVanDerWijden commented Nov 29, 2023

miner: only fill the pending block pre-merge #28440

miner: only fill the pending block pre-merge #28440

Conversation

MariusVanDerWijden commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden Oct 31, 2023 • edited Loading

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

MariusVanDerWijden commented Nov 15, 2023

MariusVanDerWijden commented Nov 24, 2023

MariusVanDerWijden commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na commented Nov 28, 2023

MariusVanDerWijden commented Nov 28, 2023 • edited Loading

MariusVanDerWijden commented Nov 28, 2023

holiman commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden commented Nov 29, 2023

MariusVanDerWijden commented Oct 31, 2023 •

edited

Loading

MariusVanDerWijden Oct 31, 2023 •

edited

Loading

MariusVanDerWijden commented Nov 28, 2023 •

edited

Loading