-
Notifications
You must be signed in to change notification settings - Fork 995
Bonsai Archive with State Proofs #8918
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
a5b4d22 to
23eafc1
Compare
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
matkt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first round of my review, added some comments and asked some questions
| : storageFormat == DataStorageFormat.BONSAI | ||
| ? DataStorageConfiguration.DEFAULT_BONSAI_CONFIG | ||
| : DataStorageConfiguration.DEFAULT_BONSAI_ARCHIVE_CONFIG) | ||
| : storageFormat == DataStorageFormat.X_BONSAI_ARCHIVE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a switch will e better in this case ?
| * @param number The height of the block whose hash should be retrieved. | ||
| * @return The hash of the block at the given height. | ||
| */ | ||
| Optional<Hash> getBlockHashByNumberSafe(long number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the diff with getBlockHashByNumber ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this change can be removed. The intention was to add a synchronized version of getBlockHashByNumber() to resolve a race condition I was observing testing the state-proof code, similar to those fixed in #6344 and #6140. But I refactored the calling code and I don't think the synchronized version of getBlockHashByNumber() is currently required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getBlockHeaderSafe() I added is still required and called from BonsaiArchiveProofsWorldStateProvider so I'll leave that in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tidied this up in the latest commits
| private Optional<BlockHeader> getCheckpointStateStartBlock( | ||
| final Blockchain blockchain, final Hash targetHash) { | ||
| long nearestCheckpointBlock = | ||
| (((blockchain.getBlockHeader(targetHash).get().getNumber() + trieNodeCheckpointInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure the block will be always here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. I've updated it to tolerate the block not being found
...g/hyperledger/besu/ethereum/trie/pathbased/bonsai/BonsaiArchiveProofsWorldStateProvider.java
Show resolved
Hide resolved
| final BlockHeader checkpointBlock, | ||
| final Hash targetBlockHash) { | ||
|
|
||
| // "Create" (fake) a world state representation at the next checkpoint block after the target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure to understand what is the fake worldstate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just setting the block; like that we are reading the right trie nodes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think I could clarify the comment wording here. "Fake" perhaps isn't the correct term. It's really just taking the current world state for the current chain head, and then starting the roll back process by asserting "this world state is for block X". The rollback logic interprets the asserted block number correctly when determining which trie logs it needs to roll. I think I will refactor the function names & comments to be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the function name in the latest commit, and reworded the comment.
...dger/besu/ethereum/trie/pathbased/bonsai/storage/flat/BonsaiArchiveProofsFlatDbStrategy.java
Show resolved
Hide resolved
| Bytes keyNearest = | ||
| calculateArchiveKeyWithMaxSuffix( | ||
| getStateArchiveContextForRead(storage), | ||
| Bytes.concatenate(accountHash, location).toArrayUnsafe()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe pushing Bytes.concatenate(accountHash, location).toArrayUnsafe()) into a byte[] field and use it for the size
Using tuweni is impacting performance so if we can avoid some call it's better
accountHash.size()
- location.size()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought - done
| + 8)) // TODO - change for CONST length | ||
| .filter( | ||
| found -> | ||
| Bytes.concatenate(accountHash, location).commonPrefixLength(found.key()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here we are doing the concatenate multiple time and this one is really slow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| return mapper.apply( | ||
| worldStateProofProvider.getAccountProof( | ||
| ws.getWorldStateRootHash(), accountAddress, accountStorageKeys)); | ||
| blockHeader.getStateRoot(), accountAddress, accountStorageKeys)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the getWorldStateRootHash will not be the good one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked back at my change to see if there was a reason for this commit. I couldn't find one, so I've reverted the code to the original ws.getWorldStateRootHash() and re-tested with a short QBFT chain and I don't see any issues. So this change has now been removed.
...erledger/besu/ethereum/trie/pathbased/common/storage/PathBasedWorldStateKeyValueStorage.java
Show resolved
Hide resolved
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
da1096e to
f120446
Compare
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
…ruption by misconfiguration Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
|
Moving out of draft as it's only PR review comments I believe I have left to address |
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
1b1a335 to
e0b22dc
Compare
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
e88078b to
b4f8de1
Compare
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
9a4fbae to
88aa451
Compare
|
I tried testing this on Linea Sepolia and it is failing on to import block 1. I get an error with the trie node not being found. So looks like there is an issue with genesis trie values. This is the error I get: |
.../main/java/org/hyperledger/besu/tests/acceptance/dsl/node/configuration/BesuNodeFactory.java
Outdated
Show resolved
Hide resolved
| + trieNodeCheckpointInterval | ||
| + ")"); | ||
| } | ||
| ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary semicolon
| @@ -371,6 +371,11 @@ public Optional<BlockHeader> getBlockHeader(final long blockNumber) { | |||
| return blockchainStorage.getBlockHash(blockNumber).flatMap(this::getBlockHeader); | |||
| } | |||
|
|
|||
| @Override | |||
| public synchronized Optional<BlockHeader> getBlockHeaderSafe(final long blockNumber) { | |||
| return blockchainStorage.getBlockHash(blockNumber).flatMap(this::getBlockHeader); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could just delegate to the non-safe one with this method using synchronized
| return blockchainStorage.getBlockHash(blockNumber).flatMap(this::getBlockHeader); | |
| return getBlockHeader(blockNumber); |
| - SNAP sync will now download only headers for pre-checkpoint (pre-merge) blocks | ||
| - `--snapsync-synchronizer-pre-checkpoint-headers-only-enabled` can be set to false to force SNAP sync to download pre-checkpoint (pre-merge) blocks | ||
| - `--history-expiry-prune` can be used to enable online pruning of pre-checkpoint (pre-merge) blocks as well as modifying database garbage collection parameters to free up disk space from the pruned blocks | ||
| - Experimental Bonsai Archive support enhanced to support `eth_proof` for historic blocks [#8918](https://github.com/hyperledger/besu/pull/8918) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should mention the bonsai archive proof format to make it clear it's not the bonsai archive data storage format that's being changed
| import org.apache.tuweni.bytes.Bytes32; | ||
| import org.apache.tuweni.rlp.RLP; | ||
|
|
||
| public class BonsaiArchivePartialFlatDbStrategy extends BonsaiArchiveFlatDbStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the plan is to leave partial flat db strategy in for now, it would be good to include the metrics that we have in the BonsaiPartialFlatDbStrategy
| final NodeLoader nodeLoader, | ||
| final Bytes location, | ||
| final SegmentedKeyValueStorage storage) { | ||
| Optional<Bytes> accountFound; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplicate of getFlatAccountTrieNode can we just delegate to that
| storage | ||
| .getNearestBeforeMatchLength(TRIE_BRANCH_STORAGE, Bytes.of(keySuffixed)) | ||
| .filter(found -> found.key().size() == (location.size() + KEY_SUFFIX_LENGTH)) | ||
| .filter(found -> location.commonPrefixLength(found.key()) >= location.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks common between a lot of the methods can the logic be moved into a common method
| trieContext = Bytes.wrap(archiveRollingContext.get()).toLong(); | ||
| } else { | ||
| trieContext = | ||
| (((Bytes.wrap(archiveContext.get()).toLong() + 1) / trieNodeCheckpointInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this breaks if the trieNodeCheckpointInterval is 0 or 1 or possibly negative. I think this should be enforced in the config that is > 1 and add tests.
|
|
||
| Optional<byte[]> archiveRollingContext = | ||
| storage.get(TRIE_BRANCH_STORAGE, ARCHIVE_PROOF_BLOCK_NUMBER_KEY); | ||
| if (archiveRollingContext.isPresent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying on an temporary state being written to the database to get historical data isn't nice. Not sure there a better to solve this. Only thing I can think of it providing some context to the trie methods the archive proof strategy and retrieve the correct trie nodes. Curious to know if in your discussions with @matkt and @garyschulte whether you came up with any other better ideas?
| @@ -52,6 +52,20 @@ public interface SegmentedKeyValueStorage extends Closeable { | |||
| Optional<NearestKeyValue> getNearestBefore(final SegmentIdentifier segmentIdentifier, Bytes key) | |||
| throws StorageException; | |||
|
|
|||
| /** | |||
| * Finds the key and corresponding value that is "nearest before" the specified key. "Nearest | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add to the description about the key length matching as well
…acceptance/dsl/node/configuration/BesuNodeFactory.java Co-authored-by: Jason Frame <jasonwframe@gmail.com> Signed-off-by: Matt Whitehead <matthew1001@hotmail.com>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>

PR description
(Replaces previous draft PR #8669 after having completed a fairly involved merge.)
Outstanding TODOs:
X_BONSAI_ARCHIVE_PROOFSandX_BONSAI_ARCHIVE_PROOFSas discrete DB format types, or whether we should just have proofs enabled for all bonsai archive DBsflatnaming convention in the classesThis PR follows on from the first Bonsai Archive PR and enhances it to provide full state proofs for Bonsai Archive state.
X_BONSAI_ARCHIVE_PROOFSX_BONSAI_ARCHIVEandX_BONSAI_ARCHIVE_PROOFSoptions are merged into a singleBONSAI_ARCHIVEdata storage format. Currently I think it will be useful to have separate options while in experimental to make it easier to recreate any issues with and without the state proof behaviour.The aim of this feature is to provide feature parity with FOREST DB and allow us to finally start the process of removing FOREST DB from Besu.
Testing
Aside from the updated tests in the PR which exercise the new flat DB format by adding
X_BONSAI_ARCHIVE_PROOFSto various existing tests, I have run a number of QBFT chains using a combination ofFOREST,BONSAI, andX_BONSAI ARCHIVE_PROOFSnodes and exercised a variety of state update & state proof requests.I have configured a node to sync with Ethereum mainnet and it is currently at block 7m without issues:
In addition to syncing with mainnet, I've created 2 test scripts which exercise
eth_getTransactionCount,eth_getStorageAt, andeth_getProoffor known accounts/states in the first 3m blocks. Theeth_getProofscript uses proofs obtained from aFORESTnode synced up to 3+m blocks, and then useseth_getProofagainst theBONSAIarchive node to check that the proofs returned byBONSAImatch those returned byFOREST.Script for eth_getProof testing on mainnet
Script for testing historic account states on mainnet
#!/bin/bash nonceResponseMatches () { echo -n "Checking nonce == $2 for account $3, block $4 - " if [[ "${1^^}" == "${2^^}" ]]; then echo OK else echo "Unexpected JSON/RPC response. $1 != $2" exit 1 fi } storageResponseMatches () { echo -n "Checking storage slot $1 == $3 for account $4, block $5 - " if [[ "${2^^}" == "${3^^}" ]]; then echo OK else echo "Unexpected JSON/RPC response. $2 != $3" exit 1 fi } # Retrive the transaction count for a number of different accounts in the first 1,000,000 blocks of Ethereum L1 # Some of these have been specfically selected for accounts that change several times in a block, or that change in several contiguous blocks. # Block 150003 - 2 transactions from the same sender. Check nonce for blocks 150002, 150003, and 150004 ACCOUNT="0x32Be343B94f860124dC4fEe278FDCBD38C102D88" BLOCK="0x249F2" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x805" $ACCOUNT $BLOCK BLOCK="0x249F3" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x807" $ACCOUNT $BLOCK BLOCK="0x249F4" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x807" $ACCOUNT $BLOCK # Blocks 138719 and 138720 - transactions from the same sender in 2 contiguous blocks. Check blocks 138718, 138719, 138720 and 138721 ACCOUNT="0x1DCb8d1F0FCc8CbC8C2d76528E877F915e299fbE" BLOCK="0x21DDE" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x59" $ACCOUNT $BLOCK BLOCK="0x21DDF" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x5a" $ACCOUNT $BLOCK BLOCK="0x21DE0" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x5b" $ACCOUNT $BLOCK BLOCK="0x21DE1" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x5b" $ACCOUNT $BLOCK # Some storage lookups for slots that are known to change # Blocks 2018260, 2020000, 2300000 for a specific smart contract ACCOUNT="0x684282178b1d61164FEbCf9609cA195BeF9A33B5" BLOCK="0x1ECBD4" SLOT="0x5" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000003" $ACCOUNT $BLOCK SLOT="0x7" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000001" $ACCOUNT $BLOCK SLOT="0xa" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000000" $ACCOUNT $BLOCK BLOCK="0x1ED2A0" SLOT="0x5" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000004" $ACCOUNT $BLOCK SLOT="0x7" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000001" $ACCOUNT $BLOCK SLOT="0xa" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000000" $ACCOUNT $BLOCK BLOCK="0x231860" SLOT="0x5" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000005" $ACCOUNT $BLOCK SLOT="0x7" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000001" $ACCOUNT $BLOCK SLOT="0xa" STORAGE_VAL=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getStorageAt","params":["'$ACCOUNT'","'$SLOT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` storageResponseMatches "$SLOT" $STORAGE_VAL "0x0000000000000000000000000000000000000000000000000000000000000001" $ACCOUNT $BLOCK # Some other random checks for nonce on accounts at later blocks # Block 2000000 ACCOUNT="0x32Be343B94f860124dC4fEe278FDCBD38C102D88" BLOCK="0x1E8480" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x1EFC6" $ACCOUNT $BLOCK # Block 3000000 ACCOUNT="0xEA674fdDe714fd979de3EdF0F56AA9716B898ec8" BLOCK="0x2DC6C0" NONCE=`curl -s -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionCount","params":["'$ACCOUNT'","'$BLOCK'"],"id":1}' http://127.0.0.1:8545 | jq .result -r` nonceResponseMatches $NONCE "0x10AA05" $ACCOUNT $BLOCK exit 0