core: reduce peak memory usage during reorg #30600

MariusVanDerWijden · 2024-10-15T09:16:47Z

~~Opening this as a draft to have a discussion.~~ Pressed the wrong button
I had a previous PR a long time ago which reduced the peak memory used during reorgs by not accumulating all transactions and logs.
This PR reduces the peak memory further by not storing the blocks in memory.
However this means we need to pull the blocks back up from storage multiple times during the reorg.
I collected the following numbers on peak memory usage:

// Master: BenchmarkReorg-8 10000 899591 ns/op 820154 B/op 1440 allocs/op 1549443072 bytes of heap used
// WithoutOldChain: BenchmarkReorg-8 10000 1147281 ns/op 943163 B/op 1564 allocs/op 1163870208 bytes of heap used
// WithoutNewChain: BenchmarkReorg-8 10000 1018922 ns/op 943580 B/op 1564 allocs/op 1171890176 bytes of heap used

Each block contains a transaction with ~50k bytes and we're doing a 10k block reorg, so the chain should be ~500MB in size

jwasinger · 2024-10-15T12:00:50Z

core/blockchain_test.go

+	// Insert an easy and a difficult chain afterwards
+	easyBlocks, _ := GenerateChain(params.TestChainConfig, blockchain.GetBlockByHash(blockchain.CurrentBlock().Hash()), ethash.NewFaker(), db, chainLength, genValueTx(50000))
+	diffBlocks, _ := GenerateChain(params.TestChainConfig, blockchain.GetBlockByHash(blockchain.CurrentBlock().Hash()), ethash.NewFaker(), db, chainLength, genValueTx(50000))


It's not clear to me what "easy" and "difficult" means here. These chains are of the same length and form.

The original meaning was probably that the 'difficult' chain was heavier, and should take precedence over the 'easy'?

Yes, its copied from another test, will rename

MariusVanDerWijden · 2024-10-16T03:11:07Z

This PR is kinda blocked on https://github.com/ethereum/go-ethereum/pull/30601/files
Would like to get that one in first, and then rebase these changes on top

karalabe · 2024-10-16T07:35:56Z

Pls rebase

karalabe · 2024-10-16T13:28:09Z

core/blockchain.go

+		if len(rebirthLogs) > 512 {
+			bc.logsFeed.Send(rebirthLogs)
+			rebirthLogs = nil
+		}


Please revert this block. The previous code sent the log removals first, and then the log additions. You changes it so now it sends events for new logs first and them removes old logs.

karalabe · 2024-10-16T13:47:43Z

core/blockchain.go

 			for _, tx := range oldBlock.Transactions() {
 				deletedTxs = append(deletedTxs, tx.Hash())
 			}
 		}
 	} else {
 		// New chain is longer, stash all blocks away for subsequent insertion
 		for ; newBlock != nil && newBlock.NumberU64() != oldBlock.NumberU64(); newBlock = bc.GetBlock(newBlock.ParentHash(), newBlock.NumberU64()-1) {
-			newChain = append(newChain, newBlock)
+			newChain = append(newChain, newBlock.Header())


Actually, this aprt will nt be correct. The headBlock is not part of the chain yet I think at this point, it's written later.

Actually, doc says head block is not processsed here

In this codepath, it looks to me like it is written

func (bc *BlockChain) writeBlockAndSetHead(block *types.Block, receipts []*types.Receipt, logs []*types.Log, state *state.StateDB, emitHeadEvent bool) (status WriteStatus, err error) { if err := bc.writeBlockWithState(block, receipts, state); err != nil { return NonStatTy, err } currentBlock := bc.CurrentBlock() // Reorganise the chain if the parent is not the head block if block.ParentHash() != currentBlock.Hash() { if err := bc.reorg(currentBlock, block); err != nil { return NonStatTy, err } } // Set new head. bc.writeHeadBlock(block)

holiman

bleh, my comments were left on pending

core/blockchain.go

holiman · 2024-10-16T13:46:09Z

core/blockchain.go

@@ -2278,14 +2278,14 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error {
 	// as it will be handled separately outside of this function
 	for i := len(newChain) - 1; i >= 1; i-- {
 		// Insert the block in the canonical way, re-writing history
-		bc.writeHeadBlock(newChain[i])
+		newBlock = bc.GetBlock(newChain[i].Hash(), newChain[i].Number.Uint64())


This begins at the latest in the new chain.

Q1: Is the latest block in the new chain already retrievable via bc.GetBlock? I think the answer is Yes.

Q2: Isn't it the same as the incoming parameter newHead? If so, we can use that. Most calls to reorg will only be 1 block, right?

@karalabe I thought you were commenting on this comment, but it's not quite the same, is it?

holiman · 2024-10-16T13:58:04Z

core/blockchain.go

 			for _, tx := range oldBlock.Transactions() {
 				deletedTxs = append(deletedTxs, tx.Hash())
 			}
 		}
 	} else {
 		// New chain is longer, stash all blocks away for subsequent insertion
 		for ; newBlock != nil && newBlock.NumberU64() != oldBlock.NumberU64(); newBlock = bc.GetBlock(newBlock.ParentHash(), newBlock.NumberU64()-1) {
-			newChain = append(newChain, newBlock)
+			newChain = append(newChain, newBlock.Header())


In this codepath, it looks to me like it is written

func (bc *BlockChain) writeBlockAndSetHead(block *types.Block, receipts []*types.Receipt, logs []*types.Log, state *state.StateDB, emitHeadEvent bool) (status WriteStatus, err error) { if err := bc.writeBlockWithState(block, receipts, state); err != nil { return NonStatTy, err } currentBlock := bc.CurrentBlock() // Reorganise the chain if the parent is not the head block if block.ParentHash() != currentBlock.Hash() { if err := bc.reorg(currentBlock, block); err != nil { return NonStatTy, err } } // Set new head. bc.writeHeadBlock(block)

karalabe · 2024-10-16T15:32:05Z

I've reworked the entire code to operate on headers and not blocks.

karalabe · 2024-10-16T15:33:48Z

The reason it did so macny block loads is because the logic was implemented originally with blocks and Marius only changes the accumulators but left the logic based on blocks. Rewriting it to headers cleaned up everything with every block being loaded max once.

That said, my code also fixed the log ordering, which is a breadking change on the API, though this is how it's correct and previously it was borked, so unsure what to do here.

karalabe · 2024-10-16T15:43:36Z

The problem with the logs is:

The original code from 2 years ago collected all the logs and emitted them in one RPC message. E.g. with 2048 logs across say 4 blocks (each 512 logs), it emitted:

Revert [Log1, Log2, ..., Log2048]; where 1 is the earlier and 2048 is the later

2 years ago #25711 introduced batching and kept the emission order:

Revert [Log1,    Log2,    ..., Log512]
Revert [Log513,  Log514,  ..., Log1024]
Revert [Log1025, Log1026, ..., Log1536]
Revert [Log1537, Log1538, ..., Log2048]

This is a problem, because anyone reacting to logs, needs to revert them in reverse order, Log 2048 first, 2047 then, etc, down to Log 1. In the original code from 2 years ago the user sub got a huge log list and it was up to them to iterate it in reverse.

In the current code however, they get the Revert [Log1, Log2, ..., Log512] network packet first, having no idea if anyithng else is coming. But they cannot handle this until the rest arrives, because reverting Log512 cannot be done until Logs[513..2048] are reverted first. Hence why the current code is borked.

This PR changes emission to reverse order, so we emit:

Revert [Log2048, Log2047, ..., Log1537]
Revert [Log1536, Log1535, ..., Log1025]
Revert [Log1024, Log1023, ..., Log513]
Revert [Log512,  Log511,  ..., Log1]

This can be meaningfully handles client side again on the RPC subscription because you can apply log revertals immediately as they arrive. Unfortunately, this breaks the API.

The problem is, that the current API is not usable, so I'm unsure what we're breaking here.

karalabe · 2024-10-16T16:24:12Z

I've added a shadow log filtering, so the events are kept emitted in the old faulty behavior for legacy APIs and there's a second emission pathway (unsued in this PR) that emits events in the correct order.

The old API ordering can't really change as it would bork everything.
The old API can only be fixed by undoing the batching, which would blow Geth up.

All in all, the old API is foobar, so IMO we can leave it be and introduce an alternative with the correct ordering.

~~Opening this as a draft to have a discussion.~~ Pressed the wrong button I had [a previous PR ](#24616 long time ago which reduced the peak memory used during reorgs by not accumulating all transactions and logs. This PR reduces the peak memory further by not storing the blocks in memory. However this means we need to pull the blocks back up from storage multiple times during the reorg. I collected the following numbers on peak memory usage: // Master: BenchmarkReorg-8 10000 899591 ns/op 820154 B/op 1440 allocs/op 1549443072 bytes of heap used // WithoutOldChain: BenchmarkReorg-8 10000 1147281 ns/op 943163 B/op 1564 allocs/op 1163870208 bytes of heap used // WithoutNewChain: BenchmarkReorg-8 10000 1018922 ns/op 943580 B/op 1564 allocs/op 1171890176 bytes of heap used Each block contains a transaction with ~50k bytes and we're doing a 10k block reorg, so the chain should be ~500MB in size --------- Co-authored-by: Péter Szilágyi <peterke@gmail.com>

MariusVanDerWijden requested review from karalabe, holiman and rjl493456442 as code owners October 15, 2024 09:16

jwasinger reviewed Oct 15, 2024

View reviewed changes

MariusVanDerWijden added 3 commits October 16, 2024 10:04

core: try to reduce peak memory usage during reorg

56011b6

core: add better benchmark numbers

99c0c18

core: also reduce memory of newChain

c7b3922

MariusVanDerWijden force-pushed the reduce-memory branch from 100ffee to c7b3922 Compare October 16, 2024 08:04

core: reorganize the code a bit

3da0fa5

karalabe reviewed Oct 16, 2024

View reviewed changes

core: revert log emission order

b0799c2

karalabe reviewed Oct 16, 2024

View reviewed changes

holiman reviewed Oct 16, 2024

View reviewed changes

core: rework reorg to use headers only; emit reverted logs in reverse

a6c4273

core: revert faulty API behavior, add shador code with correct behavior

bcd993a

karalabe added this to the 1.14.12 milestone Oct 16, 2024

karalabe merged commit 18a5918 into ethereum:master Oct 16, 2024
2 of 3 checks passed

BrewTestBot mentioned this pull request Nov 19, 2024

ethereum 1.14.12 Homebrew/homebrew-core#198233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: reduce peak memory usage during reorg #30600

core: reduce peak memory usage during reorg #30600

MariusVanDerWijden commented Oct 15, 2024 •

edited

Loading

jwasinger Oct 15, 2024

holiman Oct 15, 2024

MariusVanDerWijden Oct 16, 2024

MariusVanDerWijden commented Oct 16, 2024

karalabe commented Oct 16, 2024

karalabe Oct 16, 2024

karalabe Oct 16, 2024

karalabe Oct 16, 2024

holiman Oct 16, 2024

holiman left a comment

holiman Oct 16, 2024

holiman Oct 16, 2024

holiman Oct 16, 2024

karalabe commented Oct 16, 2024

karalabe commented Oct 16, 2024

karalabe commented Oct 16, 2024 •

edited

Loading

karalabe commented Oct 16, 2024

core: reduce peak memory usage during reorg #30600

core: reduce peak memory usage during reorg #30600

Conversation

MariusVanDerWijden commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden commented Oct 16, 2024

karalabe commented Oct 16, 2024

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

holiman 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

karalabe commented Oct 16, 2024

karalabe commented Oct 16, 2024

karalabe commented Oct 16, 2024 • edited Loading

karalabe commented Oct 16, 2024

MariusVanDerWijden commented Oct 15, 2024 •

edited

Loading

karalabe commented Oct 16, 2024 •

edited

Loading