core, triedb, trie: don't track node hash in pathdb

[rjl493456442]

This pull request drops the hash cache of nodes in the pathdb.

Originally, for each node tracked in pathdb, the node hash(32 bytes) is cached as well. As for each node retrieval, the found node is required to be hash-matched. The cached hash can avoid unnecessary hashing(it takes around 400ns on my laptop).

However, the cached hash increases the total memory consumption. If we say the average size of trie node is 100 byte, then the memory overhead is about 24% 32 / (100+32). Dropping hash cache allows us to use less memory.

Besides, it also benefits for abstraction. As for the upcoming verkle, the slim format(internal node will only store the commitment of children instead of the whole children hash list) is chosen and node hash notion is kind of nuked. This change allows us to better support verkle mode.

But we have to keep in mind that this pr indeed introduces the computation overhead for each node retrieval, which is around 400ns on my laptop). The performance degradation is reflected as below, the average block execution time is increased by ~2m.

Memory wise, the held memory size is decreased for about 1GB.

More statistics can be found here https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&from=now-12h&to=now

[rjl493456442]

More performance data is coming:

After running full sync for more than 5 days(142 hours in total), more than 13.5m blocks have been synced.
Master branch is 5 hours ahead, namely this pull request is ~3.5% slower.

Memory usage wise:

• This pull request constantly uses ~800 megabytes less memory
• Memory allocation has no difference
• This pull request has higher GC pause

[holiman]

I'm not quite finished yet, but have some comments

[holiman]

Why did you change from HasStorageTrieNode into ReadStorageTrieNode? Seems like you did it to avoid doing a keccak on len(blob)==0? perhaps then that should be internalized into the rawdb methods?

[rjl493456442]

Originally the HasStorageTrieNode will compare if the found node is hash-matched with the given hash. In this pull request, the hash comparison in HasStorageTrieNode is dropped.

We use ReadStorageTrieNode here to manually compare the hash. The new code should be logic equivalent with the old one.

[holiman]

It "looks good". I have not performed any deeper testing though

[rjl493456442]

The failing unit test is interesting. I can also repro it on master, but I believe the test is wrong. Will fix it.

EDIT: It's a bug in test and be fixed already

[holiman]

Windows 386 failure

ok  	github.com/ethereum/go-ethereum/eth/protocols/eth	1.166s
--- FAIL: TestSyncNoStorageAndOneCappedPeer (0.43s)
    sync_test.go:1056: accounts: 3000, slots: 0
    sync_test.go:250: Remote side rejected our delivery: proof node (hash 75659376764002ccf460eb80ae7e9070b1defe231e2300e5241d91965adf5f5f) missing
    sync_test.go:1053: sync failed: sync cancelled
FAIL
FAIL	github.com/ethereum/go-ethereum/eth/protocols/snap	145.538s
ok  	github.com/ethereum/go-ethereum/eth/tracers	2.088s

Linux 386 error

 --- FAIL: TestSyncNoStorageAndOneCappedPeer (0.24s)
    sync_test.go:1056: accounts: 3000, slots: 0
    sync_test.go:250: Remote side rejected our delivery: proof node (hash 75659376764002ccf460eb80ae7e9070b1defe231e2300e5241d91965adf5f5f) missing
    sync_test.go:250: Remote side rejected our delivery: proof node (hash f3d5fddd9b908502006b4ea926d4b1146e6034e7e531abc91eccb24c20e5d32e) missing
    sync_test.go:250: Remote side rejected our delivery: proof node (hash 75659376764002ccf460eb80ae7e9070b1defe231e2300e5241d91965adf5f5f) missing
    sync_test.go:1053: sync failed: sync cancelled
--- FAIL: TestSyncBoundaryAccountTrie (0.28s)

[fjl]

LGTM but please figure out the failed test.

[rjl493456442]

Close it in favor of #29362