Refactor get mutable methods of the worldtstate provider #8113

matkt · 2025-01-13T12:45:39Z

PR description

This PR refactors Bonsai/DiffBased and its provider to simplify the future addition of Verkle and the stateless part. The goal is to maintain a logical code structure, ensuring easy integration of the stateless code.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

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

matkt · 2025-01-13T14:53:27Z

test in progress, not merge for the moment

garyschulte

naming is hard. I see your intent 👍 Only thing I am not keen on is over use of the builder pattern for small classes

garyschulte · 2025-01-13T22:56:57Z

...java/org/hyperledger/besu/ethereum/trie/diffbased/common/provider/WorldStateQueryParams.java

+   *
+   * @return a new builder
+   */
+  public static Builder newBuilder() {


Seems to me like we should either expose a builder pattern or have static constructors. This class seems to be doing both.

garyschulte · 2025-01-13T23:00:38Z

...um/core/src/main/java/org/hyperledger/besu/ethereum/trie/forest/ForestWorldStateArchive.java

  }

  @Override
-  public Optional<MutableWorldState> getMutable(final Hash rootHash, final Hash blockHash) {
+  public MutableWorldState getWorldState() {


would it be reasonable to rename this to getEmptyWorldState() ?

I had a hard time choosing the name for this method because, for Forest, it's the empty state, whereas for Bonsai, it's the head without rollback. So I left it as getWorldState because I couldn't figure out how to align the two. I think once Forest is deprecated, we can rename it or maybe remove it.

garyschulte · 2025-01-13T23:05:00Z

...a/org/hyperledger/besu/ethereum/trie/diffbased/common/worldview/WorldStateStorageConfig.java

+  public static WorldStateStorageConfig.Builder newBuilder() {
+    return new WorldStateStorageConfig.Builder();
+  }


A class with a single boolean member var (for now) having a builder seems like overkill IMO. A single static constructor would probably be sufficient. Will Verkle or stateless extend this class with its own configs?

yes I think it will be needed for stateless. I can remove it later if I don't use it in the future

removed the class

garyschulte · 2025-01-13T23:14:37Z

.../java/org/hyperledger/besu/ethereum/trie/diffbased/common/worldview/DiffBasedWorldState.java

+   * @param worldStateKeyValueStorage the world state key-value storage to check.
+   * @return {@code true} if the provided storage is modifying the head, {@code false} otherwise.
+   */
+  private boolean isHeadModifyingWorldState(


should probably be renamed to isModifyingHeadWorldState() since it differentiates between modifications to worldstate or a snapshot of worldstate.

garyschulte · 2025-01-13T23:18:30Z

...java/org/hyperledger/besu/ethereum/trie/diffbased/common/provider/WorldStateQueryParams.java

+   *
+   * @return the state root
+   */
+  public Hash getStateRoot() {


IMO we should return Optional using Optional.ofNullable(stateRoot) since it isn't a required field. Same for the other fields which are effectively Optional. returning null can be surprising

right updated it

garyschulte · 2025-01-13T23:20:43Z

...rg/hyperledger/besu/ethereum/trie/diffbased/common/provider/DiffBasedWorldStateProvider.java

+   * @param queryParams the query parameters
+   * @return the stateful world state, if available
+   */
+  private Optional<MutableWorldState> getStatefulWorldState(


I think having Stateful in these method names is redundant. The worldstate is state, and hence should be stateful. We are trying to signify whether or not it is backed by a trie or a flat db versus a witness/proof?

Maybe FullWorldState vs SparseWorldState or CompleteWorldState vs WitnessWorldState ?

I think I will do a mix and use FullWorldState vs WitnessWorldState. Seems to be more clear, because SparseWorldState doesn't seems to be a good one

garyschulte · 2025-01-13T23:41:46Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java

@@ -115,7 +116,7 @@ public BlockProcessingResult validateAndProcessBlock(
      final Block block,
      final HeaderValidationMode headerValidationMode,
      final HeaderValidationMode ommerValidationMode,
-      final boolean shouldPersist,
+      final boolean shouldUpdateHead,


I like this new naming. It conveys how dangerous that param really is for diff based worldstates. Maybe even shouldUpdateGlobalState or similar that conveys it mutates the root worldstate upon which all diffs apply

I think head is better, but I'm not opposed to global state.

I like as is now

garyschulte · 2025-01-13T23:47:53Z

...rg/hyperledger/besu/ethereum/trie/diffbased/common/provider/DiffBasedWorldStateProvider.java

        || worldStateKeyValueStorage.isWorldStateAvailable(rootHash, blockHash);
  }

+  /**
+   * Gets the mutable world state based on the provided query parameters.


nitpick, should be Gets a mutable world state. "The" to me would imply the head worldstate

yes clearly better like that

lu-pinto · 2025-01-14T13:52:53Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/WorldStateArchive.java

+   *
+   * @return the head world state
+   */
+  MutableWorldState getWorldState();


So from what I understand, there's no way to get an immutable WorldState correct?

There is one called Optional get(final Hash rootHash, final Hash blockHash), but it is used very rarely (only by Forest) and in test classes or nearly deprecated subcommands. Therefore, I am considering removing it in the future as well, but I would prefer to do it in a separate PR.

lu-pinto · 2025-01-14T13:55:38Z

.../hyperledger/besu/ethereum/mainnet/precompiles/privacy/PrivacyPluginPrecompiledContract.java

@@ -81,7 +83,9 @@ public PrecompileContractResult computePrecompile(
        privateStateRootResolver.resolveLastStateRoot(privacyGroupId, privateMetadataUpdater);

    final MutableWorldState disposablePrivateState =
-        privateWorldStateArchive.getMutable(lastRootHash, null).get();


I would prefer not passing null

lu-pinto · 2025-01-14T14:16:22Z

...java/org/hyperledger/besu/ethereum/trie/diffbased/common/provider/WorldStateQueryParams.java

+   * @param blockHash the block hash
+   * @return an instance of WorldStateQueryParams
+   */
+  public static WorldStateQueryParams withStateRootAndBlockHashAndUpdateNodeHead(


I don't think this is future proof enough because the more parameters you add to the method the longer will be the name and less readable it will be. I was thinking and what about postponing calling build() using a lambda?
You then also have the benefit of postponing constructing any object until you need it:

public interface WorldStateQueryParamsSupplier extends Supplier<WorldStateQueryParams> { WorldStateQueryParams.Builder createBuilder(); default WorldStateQueryParams get() { return createBuilder().build(); } }

Usage:

worldStateArchive .getWorldState(() -> newBuilder() .stateRoot(lastRootHash) .blockHash(blockHash) .shouldWorldStateUpdateHead(true))

When you want to use it (e.g. DiffBasedWorldStateProvider):

WorldStateQueryParams queryParams = queryParamsSupplier.get();

make sense I can try to do it and see how is the result

I tested it, and I don't find it clearer. I understand that it's not future-proof, but I think that in the majority of cases, we won't need to pass more fields, except in a few calls (e.g., for stateless scenarios). In those cases, we could call the builder instead of the method, but in many cases, this method will be sufficient (e.g., for private parts). So, overcomplicating things for a somewhat custom call makes me doubt. I would suggest leaving it as is for now and revisiting in the future to see if your approach makes more sense.

example of

privacyParameters .getPrivateWorldStateArchive() .getWorldState( () -> newBuilder() .withStateRoot( privacyController .getStateRootByBlockNumber( privacyGroupId, enclaveKey, blockNumber) .get()) .withBlockHash(parentHash) .withShouldUpdateHead() .build()) .get();

instead of

.getWorldState( withStateRootAndBlockHashAndUpdateNodeHead( privacyController .getStateRootByBlockNumber(privacyGroupId, enclaveKey, blockNumber) .get(), parentHash))

what do you think ? @lu-pinto @garyschulte

I just suggested because I imagined this might grow and then your 50 character method name will turn into 80, 100 as you add fields :D
But I'm also fine with keeping it as is for now and revisit later if needed.

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

lu-pinto

Some suggestions for improvement

lu-pinto · 2025-01-14T14:19:43Z

...java/org/hyperledger/besu/ethereum/trie/diffbased/common/provider/WorldStateQueryParams.java

+     *     otherwise
+     * @return the builder
+     */
+    public Builder withShouldWorldStateUpdateHead(final boolean shouldWorldStateUpdateHead) {


nit: create separate methods for withShouldWorldStateUpdateHead instead of passing a flag. It would make it more readable

lu-pinto · 2025-01-14T14:35:24Z

.../main/java/org/hyperledger/besu/ethereum/trie/diffbased/bonsai/BonsaiWorldStateProvider.java

@@ -61,9 +61,9 @@ public BonsaiWorldStateProvider(
    provideCachedWorldStorageManager(
        new BonsaiCachedWorldStorageManager(
            this, worldStateKeyValueStorage, this::cloneBonsaiWorldStateConfig));


nit: why do you pass this::cloneBonsaiWorldStateConfig instead of just copying on the spot? Do you intend to create a StorageManager and not use it? From how I see it, using a config doesn't seem to be optional so you will use for sure IMO.

just checked, I think it was necessary in an old commit but not needed anymore, so I changed the code to remove that

lu-pinto · 2025-01-14T14:40:15Z

...perledger/besu/ethereum/trie/diffbased/bonsai/cache/NoOpBonsaiCachedWorldStorageManager.java

@@ -18,7 +18,7 @@
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.trie.diffbased.bonsai.storage.BonsaiWorldStateKeyValueStorage;
 import org.hyperledger.besu.ethereum.trie.diffbased.common.worldview.DiffBasedWorldState;
-import org.hyperledger.besu.ethereum.trie.diffbased.common.worldview.DiffBasedWorldStateConfig;
+import org.hyperledger.besu.ethereum.trie.diffbased.common.worldview.WorldStateSharedConfig;


What does WorldStateSharedConfig mean? I can find many different world state config classes and I don't know which is used for: WorldStateSharedConfig, WorldStateConfiguration and WorldStateStorageConfig

I checked and WorldStateConfiguration is not used in main . I removed it.

WorldStateSharedConfig is the configuration shared across all world states. When we launch Besu, we provide a base configuration that specifies whether the tree is disabled or if it is stateful. All world states will share this configuration.

WorldStateStorageConfig is the configuration specific to the world state at the level of key-value storage. It determines, for instance, whether the storage in this world state will be frozen.

I’m not sure how to make this explanation clearer, but I’m open to any ideas you might have. I understand that this can be challenging to understand.

I think I can try to remove WorldStateStorageConfig if it's simplify the code

removed and added more comment to explain the goal of this flag

Thanks removing the other configs really simplifies the code.

It's a nitpick but I'm still not sure about the WorldStateSharedConfig naming. This to me jumps out as a shared config at runtime, among different running entities (EL / CL - though I know it doesn't make sense to share world state with CL but couldn't remember of a better example 😓) rather than a shared config across multiple storage type definitions (Bonsai/Verkle). It implies that there is an exchange of configuration at runtime and each entity has a part that is common.
What about WorldStateCommonConfig? or just WorldStateConfig? In fact you would be extending WorldStateConfig and having extra configs that are specific to each storage type, no?

lu-pinto · 2025-01-14T14:42:49Z

...ain/java/org/hyperledger/besu/ethereum/trie/diffbased/bonsai/worldview/BonsaiWorldState.java

@@ -305,7 +302,7 @@ private void updateAccountStorageState(
                        writeStorageTrieNode(
                            bonsaiUpdater, updatedAddressHash, location, key, value)));
        // only use storage root of the trie when trie is enabled
-        if (!worldStateConfig.isTrieDisabled()) {
+        if (!worldStateSpec.isTrieDisabled()) {


nit: personal preference I think the name was good before

I can use worldStateConfig again

changed and use again worldStateConfig

lu-pinto · 2025-01-14T14:50:59Z

...yperledger/besu/ethereum/trie/diffbased/common/cache/DiffBasedCachedWorldStorageManager.java

@@ -92,7 +94,7 @@ public synchronized void addCachedLayer(
        Optional.ofNullable(this.cachedWorldStatesByHash.get(blockHeader.getBlockHash()));
    if (cachedDiffBasedWorldView.isPresent()) {
      // only replace if it is a layered storage
-      if (forWorldState.isPersisted()
+      if (forWorldState.isHeadModifyingWorldState()


nit: isMutable ?

It's not isMutable. Technically, all the world states returned by getWorldState will be mutable at their own level. The difference lies in whether the mutation can affect the head of the node (i.e., the node's primary world state), or if it's just a copy that only impacts itself and can be deleted when it's no longer needed.

For instance, in cases like transaction simulation or block building, the mutations would only affect the copy and not the main world state.

added more logs to explain this method

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

lu-pinto · 2025-01-17T15:10:37Z

...java/org/hyperledger/besu/ethereum/trie/diffbased/common/provider/WorldStateQueryParams.java

-      // retrieve the world state from the cache.
-      throw new IllegalArgumentException(
-          "Either blockHash or stateRoot must be provided to find the worldstate in the cache");
-    }


Why did you remove these validation methods? I think that was good to validate before crashing on the spot where you're using them

lu-pinto

LGTM, just have the following queries open right now:
#8113 (comment)
#8113 (comment)

matkt added 2 commits January 13, 2025 11:31

refactor of getWorldstate provider

8c69985

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

fix refactoring

43e25bf

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

matkt added verkle bonsai labels Jan 13, 2025

matkt added 2 commits January 13, 2025 13:48

merge main

ebf339a

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

fix build and tests

3a7f8b9

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

garyschulte reviewed Jan 13, 2025

View reviewed changes

lu-pinto reviewed Jan 14, 2025

View reviewed changes

fix for review

2a3cabf

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

lu-pinto reviewed Jan 14, 2025

View reviewed changes

matkt added 2 commits January 14, 2025 16:58

remove unused class

478b3a8

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

fix for review

103ad88

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

matkt force-pushed the feature/refactor-get-mutable branch from 3d9d9a2 to 103ad88 Compare January 15, 2025 16:53

matkt added 4 commits January 17, 2025 11:33

fix build issues

3e09764

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

merge main

257b568

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

fix plugin api hash

f5ef7e0

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

Merge branch 'main' into feature/refactor-get-mutable

42bfcd0

lu-pinto reviewed Jan 17, 2025

View reviewed changes

Refactor get mutable methods of the worldtstate provider #8113

Are you sure you want to change the base?

Refactor get mutable methods of the worldtstate provider #8113

Conversation

matkt commented Jan 13, 2025

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

matkt commented Jan 13, 2025 • edited Loading

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

matkt Jan 14, 2025 • 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

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

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

lu-pinto Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

matkt Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-pinto Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

lu-pinto 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

matkt Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-pinto Jan 17, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-pinto left a comment • edited Loading

Choose a reason for hiding this comment

matkt commented Jan 13, 2025 •

edited

Loading

matkt Jan 14, 2025 •

edited

Loading

lu-pinto Jan 14, 2025 •

edited

Loading

matkt Jan 14, 2025 •

edited

Loading

lu-pinto Jan 17, 2025 •

edited

Loading

matkt Jan 14, 2025 •

edited

Loading

lu-pinto Jan 17, 2025 •

edited

Loading

lu-pinto left a comment •

edited

Loading