Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default bonsai to fully flat db and code storage by codehash #6894

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Apr 6, 2024

PR description

This PR changes the default storage mode of bonsai to use code storage by code hash, and uses flat db by default. This supports the future direction of bonsai, and simplifies the configuration necessary for besu to serve snap protocol (see description on #6640, now just --Xsnapsync-server-enabled is necessary).

changes:

  • defaults bonsai to using a fully flattened db
  • defaults bonsai to using code stored by code hash
  • deprecates snap sync full flat config param
  • creates --Xbonsai-full-flat-db-enabled parameter
  • FlatDbStrategyProvider defers config if db is empty and persists a flat db strategy config

tested with:

  • full sync, correctly creates full flattened db
  • snap sync, correctly creates full flattend db, correctly heals flat db
  • existing holesky sync, with FULL flat db and code by CODE_HASH
    • starting with config for partial db and code by account hash correctly defers to db config and logs actual
  • existing 24.3.0 holesky with PARTIAL flat db and code by ACCOUNT_HASH
    • starting with configs for full and code by code hash correctly defers to db config and logs actual

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

@garyschulte garyschulte requested review from jframe and matkt April 6, 2024 05:02
@garyschulte garyschulte force-pushed the feature/default-flat-db-and-codehash-storage branch 2 times, most recently from 7c12480 to d201d64 Compare April 22, 2024 17:57
@garyschulte garyschulte marked this pull request as ready for review April 22, 2024 18:05
@garyschulte
Copy link
Contributor Author

It is worth noting there is a side effect of this PR on worldstate fault heal/recovery. If a db with a FULL flat db strategy encounters an inconsistent worldstate db, as part of fixing the worldstate, the database will be left with a PARTIAL flat db strategy after fixing the worldstate.

We can re-enable flat db after the healing, but this side effect might actually be desirable.

Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added small comments

@@ -217,6 +217,7 @@ receipt-compaction-enabled=true
# feature flags
Xsecp256k1-native-enabled=false
Xaltbn128-native-enabled=false
Xsnapsync-server-enabled=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not add the new full flat db flag ?

@@ -152,6 +152,8 @@ public void assertCloseDisposesOfStateWithoutCommitting() {
assertThat(res.isSuccessful()).isTrue();
shouldCloseSnapshot.persist(oneTx.getHeader());

// TODO: full flat strategy will not be updated with these values, only the trie
// determine how we want to make this assertion for full flat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use an updater in order to fix the test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it and realized it will degrade the performance of the 'normal' use case for frozen/cached worldstates. I think this note in the test superclass should be sufficient to explain why we are using PARTIAL for the test - removed from TODO.

var flatDbMode =
FlatDbMode.fromVersion(
composedWorldStateStorage
.get(TRIE_BRANCH_STORAGE, FLAT_DB_MODE)
.map(Bytes::wrap)
.orElse(FlatDbMode.PARTIAL.getVersion()));
.orElseGet(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -71,12 +72,36 @@ public void loadFlatDbStrategy(final SegmentedKeyValueStorage composedWorldState

@VisibleForTesting
FlatDbMode deriveFlatDbStrategy(final SegmentedKeyValueStorage composedWorldStateStorage) {
final FlatDbMode requestedFlatDbMode =
dataStorageConfiguration.getUnstable().getBonsaiFullFlatDbEnabled()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this flag still in unstable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is, but the generated classes have to be rebuilt to see it. Pretty much all of the bonsai configs are in unstable, we should move them all out of unstable when we change the default to bonsai 👍

@garyschulte garyschulte force-pushed the feature/default-flat-db-and-codehash-storage branch 3 times, most recently from db32129 to b16d74d Compare May 1, 2024 16:16
@@ -183,16 +183,6 @@ public CompletableFuture<Void> run(
} else {
// start from scratch
worldStateStorageCoordinator.clear();
// we have to upgrade to full flat db mode if we are in bonsai mode
if (snapSyncConfiguration.isFlatDbHealingEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have to downgradeToPartialFlatDbMode if a user disable the full flat flag ? as the default is full now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is a good point. I hadn't considered that anybody would want to run in a partial mode. I will double check, but the storage should be created in the correct/requested mode, whether that is partial or full.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can prevent user from doing that. It would not be logic

@garyschulte garyschulte force-pushed the feature/default-flat-db-and-codehash-storage branch from b16d74d to 11a8d25 Compare May 2, 2024 20:59
Signed-off-by: garyschulte <garyschulte@gmail.com>
…torageOption for --Xbonsai-full-flat-db-enabled

Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte force-pushed the feature/default-flat-db-and-codehash-storage branch from 11a8d25 to f7055ec Compare May 3, 2024 23:00
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte enabled auto-merge (squash) May 3, 2024 23:24
@garyschulte garyschulte merged commit 0a24acc into hyperledger:main May 3, 2024
42 checks passed
@garyschulte garyschulte deleted the feature/default-flat-db-and-codehash-storage branch May 4, 2024 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants