core/state/snapshot: replace diffToDisk ideal batch size with 64MB

[aaronbuchwald]

This PR updates diffToDisk(...) within snapshot flattening to write the entire diff to disk in a single batch.

Writing the diff to disk across multiple batches leads to the possibility that the snapshot will be corrupted during an ungraceful shutdown and can cause degraded performance during snapshot re-generation on startup.

[rjl493456442]

Unfortunately, this will be problematic. In Ethereum, it's possible some contracts will be deleted, and the associated storage should be deleted from the snapshot. However, these contracts can be unbound and might result in out-of-memory(theoretically possible to build such contract, but have the cost to fulfill the slots though).

Therefore, we split the big write into several small ones.

[holiman]

These checks were added because they were needed, see #22582 for more details, and also #22497 (comment) for some history:

The ropsten BitlleGasStation was self-destructed at block 6490899, in this tx .

[aaronbuchwald]

Thanks for the context! I was under the mistaken impression that this change was made as a performance optimization as opposed to avoid an OOM.

[aaronbuchwald]

As an alternative to avoid snapshot corruption. Applying the diff from the same block should be an idempotent operation. Would you be open to a PR that:

• write the previous and next block hash when applying a snapshot diffToDisk operation so that there's a marker when in the middle of a flatten operation of exactly which diff is being applied
• on restart, if you observe a corrupted snapshot, re-process the same block to re-create the same diff and apply it to complete the incomplete diffToDisk operation

I understand that this may break some abstractions, so curious whether you'd be open to this idea, think it would be too gross to add, or even if snapshot corruption is not much of a concern in your view.

[karalabe]

Can't we have a middle ground? The IdealBatchSize is something like 100KB. That will trigger on a lot of contracts. But if we cap it at some significantly higher number (something reasonable in comparison with the max allowed deletion), then it should be fine almost always, except if you nuke your node exactly when it's deleting something larger?

[karalabe]

I guess there's no max allowed deletion in the snapshots. Still, could we special case this and cap it at a significantly higher number instead of 100KB? Say 64MB-256MB?

[holiman]

Say 64MB-256MB?

Should be fine; IMO rather 64 than 256

[karalabe]

Can we keep the code for now and replace ethdb.IdealBatchSize with 64 * 1024 * 1024 in both invocations? When removing SELFDESTRUCT lands in Cancun, we can nuke it out according to the original PR. In between we'd at least have a more robust code.

[aaronbuchwald]

Just updated during only those two invocations.