worldstate refactor #6209

matkt · 2023-11-28T10:14:40Z

PR description

for verkle, I’m starting a refactor. In simple terms, many classes can distinguish whether they’re in ‘bonsai’ or ‘forest’ mode, so a generic interface doesn’t offer much value, except for some minor and synchronization parts.
So, I’ve developed a ’WorldstatestorageCoordinator class. This will have a storage strategy, which could be either ‘bonsai’ or ‘forest’. This coordinator is needed when we’re unsure about the storage mode we use, and it helps for the routing process. when we’re sure about the storage mode, we directly use the appropriate strategy.
This approach reduces the number of generic interfaces, eliminating locations that are not applicable to modes like ‘forest’ or other not clean things like that.
Will be better when we will have to add some custom method or field for verkle

Test done on holesky

	Checkpoint with trielog and blockchain pruning	Snapsync with codeByhash
Bonsai	OK	OK

Test done on sepolia

	Full	Checkpoint	Full with Full flat DB
Bonsai	OK	OK	ok
Forest	OK	OK	not needed

Test done on mainnet

	Checkpoint with Full flat DB
Bonsai	OK

Test done on QBFT network

Deploy contract, read storage , selfdestruct, block creation and block import

Fixed Issue(s)

github-actions · 2023-11-28T10:14:57Z

I thought about documentation and added the doc-change-required label to this PR if updates are required.
I thought about the changelog and included a changelog update if required.
If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

.../main/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java

.../main/java/org/hyperledger/besu/ethereum/forest/storage/ForestWorldStateKeyValueStorage.java

...ava/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/heal/TrieNodeHealingRequest.java

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java

Signed-off-by: Karim TAAM <[email protected]>

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java

Signed-off-by: Karim TAAM <[email protected]>

Signed-off-by: Karim Taam <[email protected]>

.../java/org/hyperledger/besu/ethereum/trie/bonsai/storage/BonsaiWorldStateKeyValueStorage.java

.../java/org/hyperledger/besu/ethereum/trie/forest/storage/ForestWorldStateKeyValueStorage.java

Signed-off-by: Karim Taam <[email protected]>

besu/src/main/java/org/hyperledger/besu/cli/subcommands/operator/BackupState.java

...core/src/main/java/org/hyperledger/besu/ethereum/trie/bonsai/worldview/BonsaiWorldState.java

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/trie/forest/pruner/Pruner.java

...m/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateKeyValueStorage.java

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java

...ava/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/heal/TrieNodeHealingRequest.java

ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPOutput.java

gfukushima

lgtm with a few minor questions

Signed-off-by: Karim Taam <[email protected]>

garyschulte

Good refactor with WorldStateStorageCoordinator. I think we should take the opportunity to get rid of some stuff rather than perpetuate it (like the state backup stuff), but that can be in another PR (before or after this one).

Only gripe is in the coordinator where we unnecessarily perpetuate each storage format and its worldstate as a part of the method signature

garyschulte · 2024-01-12T22:34:25Z

besu/src/main/java/org/hyperledger/besu/cli/subcommands/operator/BackupState.java

@@ -81,15 +81,19 @@ public void run() {

    final BesuController besuController = createBesuController();
    final MutableBlockchain blockchain = besuController.getProtocolContext().getBlockchain();
-    final WorldStateStorage worldStateStorage =


this has been an undocumented experimental feature for 3 years. we should get rid of it. probably in a different PR

garyschulte · 2024-01-12T22:38:22Z

besu/src/main/java/org/hyperledger/besu/cli/subcommands/operator/RestoreState.java

@@ -83,7 +83,7 @@ public class RestoreState implements Runnable {
  private long trieNodeCount;
  private boolean compressed;
  private BesuController besuController;
-  private WorldStateStorage.Updater updater;


same, non-blocking. forest is deprecated and we should deprecate these commands too

garyschulte · 2024-01-12T22:51:42Z

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java

+    }
+  }
+
+  public <RESPONSE> RESPONSE applyForStrategy(


this is worse tbh. now we have both forest and bonsai details as part of the interface and package imports, and the method signature so generic it is meaningless.

This should be private and the only external place we use it, in fast sync, that will soon be the same (just code hash) after #5889

see comment about how to remove this via generics and/or WorldStateKeyValueStorage sub-interfaces like PathBasedWorldStorage, HashBasedWorldStorage, etc.

garyschulte · 2024-01-12T23:15:12Z

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/StateBackupService.java

@@ -84,12 +84,12 @@ public StateBackupService(
      final Blockchain blockchain,
      final Path backupDir,
      final EthScheduler scheduler,
-      final WorldStateStorage worldStateStorage) {


garyschulte · 2024-01-12T23:21:35Z

...eum/core/src/main/java/org/hyperledger/besu/ethereum/trie/forest/pruner/MarkSweepPruner.java

@@ -58,7 +58,7 @@ public class MarkSweepPruner {
  private static final int MAX_MARKING_THREAD_POOL_SIZE = 2;

  private final int operationsPerTransaction;
-  private final WorldStateStorage worldStateStorage;
+  private final ForestWorldStateKeyValueStorage worldStateKeyValueStorage;


garyschulte · 2024-01-12T23:26:36Z

...c/main/java/org/hyperledger/besu/ethereum/trie/forest/worldview/ForestMutableWorldState.java

      final WorldStatePreimageStorage preimageStorage,
      final EvmConfiguration evmConfiguration) {
-    this.worldStateStorage = worldStateStorage;
+    this.worldStateKeyValueStorage = (ForestWorldStateKeyValueStorage) worldStateKeyValueStorage;


we can probably get away from this explicit cast by changing the constructor signature. It looks like only evmtool BlockchainModule is calling this from a context that doesn't explicitly have storage as a ForestWorldStateKeyValueStorage.

I tried but if I remove this cast I will have to add it to the other contructor that call this one. I can maybe n ot use this constructor for the other one . I don't know what do you think

public ForestMutableWorldState( final WorldStateKeyValueStorage worldStateKeyValueStorage, final WorldStatePreimageStorage preimageStorage, final EvmConfiguration evmConfiguration) { this( MerkleTrie.EMPTY_TRIE_NODE_HASH, worldStateKeyValueStorage, preimageStorage, evmConfiguration); } public ForestMutableWorldState( final Bytes32 rootHash, final WorldStateKeyValueStorage worldStateKeyValueStorage, final WorldStatePreimageStorage preimageStorage, final EvmConfiguration evmConfiguration) { this.worldStateKeyValueStorage = (ForestWorldStateKeyValueStorage) worldStateKeyValueStorage; this.accountStateTrie = newAccountStateTrie(rootHash); this.preimageStorage = preimageStorage; this.evmConfiguration = evmConfiguration; }`

garyschulte · 2024-01-12T23:36:37Z

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java

+public class WorldStateStorageCoordinator {
+  private final WorldStateKeyValueStorage worldStateKeyValueStorage;
+
+  public WorldStateStorageCoordinator(final WorldStateKeyValueStorage worldStateKeyValueStorage) {


👍 to the coordinator concept. I like that this is the central place to add/remove storage formats and their interfaces.

the only bits I do not care for are where we double-down on forest and bonsai storage modes in the signatures, the apply and consume ForStrategy methods basically. I think we could do some lambda magic to limit the bonsai/forest stuff in these methods, like:

public <RESPONSE, T extends WorldStateKeyValueStorage> RESPONSE applyForStrategy( final Function<T, RESPONSE> strategy) {

Why I am adding Bonsai and Forest in the signature is because I no longer want a common interface between Bonsai and Forest. The coordinator will play the role of redirection and do the cast to simplify other calls. So, as I don't have a common interface, I can no longer create a single strategy interface that I pass to the coordinator. I can do that but I will have to add the cast logic in other calls and in order to simplify this call I will add complexity to the other calls . I must therefore pass both and the coordinator calls the right one.

If I recreate an interface, it's doing what we had before and I specifically wanted to avoid that. Knowing that Forest will be eliminated soon, this should also remove the coordinator and this mechanism. But maybe I didn't understand your idea.

This is what I was suggesting:

garyschulte@aab6187

I didn't remove all the applyWhen, consumeFor, etc but I think we can simplify the coordinator a lot by just keeping an interface hierarchy and using it as a WorldStateCoordinator generic param.

Ideally when we get rid of Forest, we can just remove the hash-based interface (and implementations) and not touch the coordinator interface

could you confirm on this PR that it's ok for you to merge with the current implementation ?

Confirmed 👍

jframe · 2024-01-16T07:28:11Z

Don't see anything to worry about in terms of impact on private networks

Signed-off-by: Karim Taam <[email protected]>

garyschulte

🚢

Signed-off-by: Karim Taam <[email protected]>

gfukushima

left a few nits and non-blocker comments, lgtm in general and it is nice to avoid the nulls we use to pass to match the general storage format interface. This approach seems to be friendly and extensible to add verkle

gfukushima · 2024-03-08T03:07:24Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/trie/forest/pruner/Pruner.java

@@ -51,7 +51,7 @@ public class Pruner {
  private final ExecutorService executorService;

  @VisibleForTesting
-  Pruner(
+  public Pruner(


think this doesn't need to be public.

will fix it thanks

gfukushima · 2024-03-08T03:10:56Z

...core/src/main/java/org/hyperledger/besu/ethereum/trie/bonsai/worldview/BonsaiWorldState.java

    this.worldStateRootHash =
        Hash.wrap(
-            Bytes32.wrap(worldStateStorage.getWorldStateRootHash().orElse(Hash.EMPTY_TRIE_HASH)));
+            Bytes32.wrap(
+                worldStateKeyValueStorage.getWorldStateRootHash().orElse(getEmptyTrieHash())));


nit: I slightly prefer the previous version of this, using the Hash.EMPTY_TRIE_HASH

it's for verkle , because we will override the value . with verkle empty trie hash is different

gfukushima · 2024-03-08T03:53:42Z

.../hyperledger/besu/ethereum/eth/sync/snapsync/request/heal/AccountTrieNodeHealingRequest.java

    return 1;
  }

  @Override
  public Optional<Bytes> getExistingData(
-      final SnapWorldDownloadState downloadState, final WorldStateStorage worldStateStorage) {


What did we use the downloadState for previously? 🤔

I think it was just useless parameter that we forgot to remove. I checked and it was not used

Signed-off-by: Gabriel Fukushima <[email protected]>

… feature/worldstate-refactor

gfukushima · 2024-03-08T05:39:31Z

Took a stab fixing the only unit test broken in this PR, please have a look if it looks good to you.

Signed-off-by: Karim Taam <[email protected]>

* worldstate storage refactor Signed-off-by: Karim TAAM <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]> Signed-off-by: amsmota <[email protected]>

* worldstate storage refactor Signed-off-by: Karim TAAM <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]>

github-advanced-security bot found potential problems Nov 28, 2023

View reviewed changes

worldstate storage refactor

657a78e

Signed-off-by: Karim TAAM <[email protected]>

matkt force-pushed the feature/worldstate-refactor branch from caa73c3 to 657a78e Compare November 28, 2023 10:37

fix unit tests

6d4a4bb

Signed-off-by: Karim TAAM <[email protected]>

github-advanced-security bot found potential problems Nov 28, 2023

View reviewed changes

...ore/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateStorageCoordinator.java Dismissed Show dismissed Hide dismissed

matkt added 4 commits November 28, 2023 17:10

fix unit tests

d8a411e

Signed-off-by: Karim TAAM <[email protected]>

fix unit tests

e9558d3

Signed-off-by: Karim TAAM <[email protected]>

merge main

18e1899

Signed-off-by: Karim Taam <[email protected]>

merge main

4a3ea8a

Signed-off-by: Karim Taam <[email protected]>

github-advanced-security bot found potential problems Dec 19, 2023

View reviewed changes

.../java/org/hyperledger/besu/ethereum/trie/bonsai/storage/BonsaiWorldStateKeyValueStorage.java Fixed Show fixed Hide fixed

.../java/org/hyperledger/besu/ethereum/trie/forest/storage/ForestWorldStateKeyValueStorage.java Dismissed Show dismissed Hide dismissed

fix tests

a368a35

Signed-off-by: Karim Taam <[email protected]>

matkt force-pushed the feature/worldstate-refactor branch from 24a2750 to a368a35 Compare December 20, 2023 16:34

matkt marked this pull request as ready for review December 21, 2023 11:04

gfukushima reviewed Dec 22, 2023

View reviewed changes

gfukushima approved these changes Dec 22, 2023

View reviewed changes

matkt added 4 commits January 9, 2024 14:37

merge main

ccdb980

Signed-off-by: Karim Taam <[email protected]>

fix review

41fc466

Signed-off-by: Karim Taam <[email protected]>

Merge branch 'main' into feature/worldstate-refactor

513734f

Merge branch 'main' into feature/worldstate-refactor

61e985b

garyschulte reviewed Jan 12, 2024

View reviewed changes

matkt added 3 commits January 16, 2024 14:10

Merge branch 'main' into feature/worldstate-refactor

9f4cd99

merge main

772efd7

Signed-off-by: Karim Taam <[email protected]>

Merge branch 'main' into feature/worldstate-refactor

78f64f2

garyschulte approved these changes Jan 23, 2024

View reviewed changes

matkt added 4 commits January 25, 2024 14:07

merge main

36ee939

Signed-off-by: Karim Taam <[email protected]>

fix build

d0ed277

Signed-off-by: Karim Taam <[email protected]>

Merge branch 'main' into feature/worldstate-refactor

93f0476

Merge branch 'main' into feature/worldstate-refactor

6620e1d

Merge branch 'main' into feature/worldstate-refactor

1ca3362

matkt mentioned this pull request Feb 28, 2024

Refactor bonsai in order to add verkle storage format #6627

Closed

matkt added 7 commits March 1, 2024 15:38

merge main

44df1e9

Signed-off-by: Karim Taam <[email protected]>

fix ref tests

e3ae5d1

Signed-off-by: Karim Taam <[email protected]>

fix tests

9dfadc4

Signed-off-by: Karim Taam <[email protected]>

fix build

0f24e3a

Signed-off-by: Karim Taam <[email protected]>

fix build

3f34e01

Signed-off-by: Karim Taam <[email protected]>

fix failed tests

bb66627

Signed-off-by: Karim Taam <[email protected]>

Merge branch 'main' into feature/worldstate-refactor

fadc9c4

gfukushima reviewed Mar 8, 2024

View reviewed changes

gfukushima added 4 commits March 8, 2024 14:23

fix test to use BonsaiWSKVStorage

228a794

Signed-off-by: Gabriel Fukushima <[email protected]>

Merge branch 'main' into feature/worldstate-refactor

7824da4

spotless

b9ffdcf

Signed-off-by: Gabriel Fukushima <[email protected]>

Merge remote-tracking branch 'matkt/feature/worldstate-refactor' into…

4e045c6

… feature/worldstate-refactor

matkt added 2 commits March 8, 2024 10:31

Merge branch 'main' into feature/worldstate-refactor

47e97c0

fiw review

574fac1

Signed-off-by: Karim Taam <[email protected]>

garyschulte merged commit 2360908 into hyperledger:main Mar 8, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worldstate refactor #6209

worldstate refactor #6209

matkt commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023

gfukushima left a comment

garyschulte left a comment

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

garyschulte Jan 12, 2024

matkt Jan 15, 2024 •

edited

Loading

garyschulte Jan 12, 2024

matkt Jan 15, 2024 •

edited

Loading

garyschulte Jan 17, 2024 •

edited

Loading

matkt Jan 19, 2024

garyschulte Jan 23, 2024

jframe commented Jan 16, 2024

garyschulte left a comment

gfukushima left a comment

gfukushima Mar 8, 2024

matkt Mar 8, 2024

gfukushima Mar 8, 2024

matkt Mar 8, 2024

gfukushima Mar 8, 2024

matkt Mar 8, 2024

gfukushima commented Mar 8, 2024

worldstate refactor #6209

worldstate refactor #6209

Conversation

matkt commented Nov 28, 2023 • edited Loading

PR description

Test done on holesky

Test done on sepolia

Test done on mainnet

Test done on QBFT network

Fixed Issue(s)

github-actions bot commented Nov 28, 2023

gfukushima left a comment

Choose a reason for hiding this comment

garyschulte 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

matkt Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkt Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

garyschulte Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jframe commented Jan 16, 2024

garyschulte left a comment

Choose a reason for hiding this comment

gfukushima 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

gfukushima commented Mar 8, 2024

matkt commented Nov 28, 2023 •

edited

Loading

matkt Jan 15, 2024 •

edited

Loading

matkt Jan 15, 2024 •

edited

Loading

garyschulte Jan 17, 2024 •

edited

Loading