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.

Sign up for GitHub

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

Jump to bottom

Snapshot based non-persisting MutableWorldState usage #4531

Merged
merged 24 commits into from
Nov 1, 2022
Merged

Snapshot based non-persisting MutableWorldState usage #4531

merged 24 commits into from
Nov 1, 2022

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Oct 13, 2022 •
edited
Loading

PR description

Adds Bonsai snapshots as an experimental option (--Xbonsai-use-snapshots) into BonsaiWorldStateArchive for arbitrary mutable non-persisting worldstate. Also refactors TrieLogManager into a LayeredTrieLogManager and SnapshotTrieLogManager such that cached worldstates use either layered worldstates or snapshot worldstates.

Since snapshot transactions need to release their snapshot in order to allow their logs to be compacted, this PR refactors direct uses of mutable worldstates to be in try-with-resources blocks (to auto-close snapshots after usage):

  • MainnetTransactionValidator
  • AbstractBlockCreator
  • TransactionSimulator
  • TransactionPool.validateTransaction

Fixed Issue(s)

relates to #4372
relates to #4250
relates to #4199
relates to #4151

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request introduces 1 alert when merging dcfcc53 into d73ce21 - view on LGTM.com

new alerts:

  • 1 for Type mismatch on container modification

@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch 6 times, most recently from 64a6d35 to 6377bb0 Compare October 19, 2022 06:58
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch 3 times, most recently from a9df9b3 to a07d7d2 Compare October 24, 2022 10:00
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch from 267fd18 to 94a0b25 Compare October 26, 2022 20:29
@garyschulte garyschulte marked this pull request as ready for review October 27, 2022 02:55
@garyschulte garyschulte mentioned this pull request Oct 27, 2022
Closed
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch 3 times, most recently from bba8cb0 to 93459f4 Compare October 29, 2022 21:32
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch from 46b8bab to 099a762 Compare October 31, 2022 17:04
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 31, 2022
View reviewed changes
.../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceCallMany.java
} catch (final EmptySimulatorResultException e) {
LOG.error(
"Empty simulator result, call params: {}, blockHeader: {} ",
JsonCallParameterUtil.validateAndGetCallParams(requestContext),

Check notice

Code scanning / CodeQL

Use of default toString()

Default toString(): JsonCallParameter inherits toString() from Object, and so is not suitable for printing.
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch from 7c074aa to c03c476 Compare November 1, 2022 17:36
gezero
gezero approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@gezero gezero left a comment

Choose a reason for hiding this comment

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

LGTM

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/BlockchainQueries.java Show resolved Hide resolved
...rc/test-support/java/org/hyperledger/besu/ethereum/core/InMemoryKeyValueStorageProvider.java Show resolved Hide resolved
jflo
jflo approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

just a couple of minor optional notes, but this is pretty awesome.

.../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/processor/BlockReplay.java Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionSimulator.java Show resolved Hide resolved
.../src/test/java/org/hyperledger/besu/ethereum/bonsai/BonsaiSnapshotWorldStateArchiveTest.java Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateArchive.java
final Optional<Hash> blockHashByNumber = blockchain.getBlockHashByNumber(blockNumber);
if (blockHashByNumber.isPresent()) {
return getMutable(null, blockHashByNumber.get(), isPersistingState);
final Hash rootHash, final Hash blockHash, final boolean shouldPersistState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rootHash is only needed if shouldPersistState == true, and in that case it is not passed any further along. Could be deleted.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/SnapshotTrieLogManager.java Show resolved Hide resolved
@garyschulte
initial plubming of Bonsai snapshots into BonsaiWorldStateArchive and…
8e51417 
… usages

  includes: try-with-resources and AutoCloseable in order to relase snapshots when we are done with them
  still outstanding:
   .. try-with-resources on transaction simulation stuff
   .. jsonrpc query stuff
   .. figure out what to do with the collection of layered worldstates (keep and base on snapshot?, or just ditch?)

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix test mocks, trieLogManager map type
d2acef7 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
implement rocksdb transaction based snapshots as well as BonsaiWorldS…
f2080e4 
…tateUpdater based snapshot worldstates for performance comparison

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add try-with-resources to blockchain queries and some rpc endpoints
4143439 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
initial plumbing so snapshot and non-snapshot bonsai can co-exist
b491895 
 still outstanding:
  - add and implement snapshot-capable subclass of StorageProvider
  - add --Xsnapshot-bonsai CLI flag

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix unit test mock
21abcb5 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
plumb snapshots into StorageProvider(s)
fbcd762 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add CLI param --Xbonsai-use-snapshots
be0839b 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix bonsai worldstate tests broken in refactor
f286122 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add snapshots config to runner/builder tests
f2fd8d3 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix acceptance tests
cf55a97 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
initial implementation of snapshot-based layered worldstate
23f3837 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
create TrieLogManager based on useSnapshots param, add TrieLogManager…
d2e9823 
… interface

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
log and return Optional.empty on failed log rolling
dda3c1e 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
ensure we use cached layers for snapshots, BonsaiWorldStateArchive re…
8486384 
…adability refactor, rethrow unchecked exception in BlockchainQueries

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add reference counting and implicit closing of snapshots
6a61d4a 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
make RocksDBSnapshotTransaction.copy() *also* copy the transaction st…
e184c4b 
…ate, not just restart a new transaction from the original snapshot

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add isDirty() to BonsaiWorldStateUpdater and rely on that in BonsaiSn…
400ff6d 
…apshotPersistedWorldState rather than an explicitly managed flag

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix copy() in BonsaiSnapshotWorldState, and minor cleanup
4c632bf 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
cleanup for review, add changelog, address outstanding TODOs
51f3eb5 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
refactor blockchainqueries and graphql adapters to use try-with-resou…
82749a9 
…rces autoclosing mutableworldstate

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix refactor fallout, javadoc
7ac239f 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
refactor transaction simulator to use try-with-resources
adafa3e 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
remove redunant worldstate mapping function, make getWorldState priva…
b5c64ab 
…te in TransactionSimulator

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte force-pushed the feature/snapshot-based-mergecoordinator branch from c03c476 to b5c64ab Compare November 1, 2022 22:32
@garyschulte garyschulte enabled auto-merge (squash) November 1, 2022 22:34
@garyschulte garyschulte merged commit 76d6429 into hyperledger:main Nov 1, 2022
@garyschulte garyschulte mentioned this pull request Nov 2, 2022
Merged
macfarla pushed a commit to macfarla/besu that referenced this pull request Jan 10, 2023
@garyschulte @macfarla
Snapshot based non-persisting MutableWorldState usage (hyperledger#4531)
8808f06 
* implementation of Bonsai snapshots based BonsaiWorldStateArchive
  includes: try-with-resources and AutoCloseable WorldState in order to release snapshots when we are done with them

Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@garyschulte @eum602
Snapshot based non-persisting MutableWorldState usage (hyperledger#4531)
6e5f06d 
* implementation of Bonsai snapshots based BonsaiWorldStateArchive 
  includes: try-with-resources and AutoCloseable WorldState in order to release snapshots when we are done with them

Signed-off-by: garyschulte <garyschulte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matkt matkt matkt left review comments

@jflo jflo jflo approved these changes

@gezero gezero gezero approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@garyschulte @gezero @jflo @matkt @macfarla