Feature/multi version flat db #5865
Conversation
|
51c12ef to
1700245
Compare
| public void putFlatCode( | ||
| final SegmentedKeyValueStorageTransaction transaction, | ||
| final Hash accountHash, | ||
| final Bytes32 codeHash, |
Check notice
Code scanning / CodeQL
Useless parameter
2963fa3 to
ca74aab
Compare
|
|
||
| var contextSafeCopy = worldStateStorage.getContextSafeCopy(); | ||
| contextSafeCopy.getFlatDbStrategy().updateBlockContext(blockHeader); | ||
| return Optional.of(new BonsaiWorldState(this, contextSafeCopy)); |
There was a problem hiding this comment.
not sure why we are doing that . seems not clear to clone only the flatdb . It seems to be better to follow the logic of bonsai to clone all the storage if we need a clone and pass this clone to BonsaiWorldstate . like that no need to have a specific clone for flat db .
just passing a clone of the storage to the worldstate and the internal flat db will check directly the snapshot automatically
I'm also not fan to clone everytime. we had problem before because of that . I think we can use the same code as before . using the clone in the cache and if not present create a new one (without rollback)
return trieLogManager.getHeadWorldState(blockchain::getBlockHeader)
.map(MutableWorldState::freeze);When we will have the checkpointed trie we will have to add again the rollback. maybe an optional rolling depending if we need the state or not
There was a problem hiding this comment.
just saw that the current implemention is a hack for testing 👍
There was a problem hiding this comment.
There isn't really any cloning exactly, contextSafeClone exists only to directly reuse the flatdb storage, but with a different bonsai context for a different block number. For example we don't want RPC queries to modify the bonsai context, because that will mess up the block suffixing when we persist.
We need to guard the bonsai context for the primary worldstate, but otherwise as long as we are not persisting, there is no need to snapshot or clone the database itself.
So clone is probably not the best name for this, better naming suggestions welcome :). My thinking here is that as long as this is a 'non-persisting' mutable worldstate, we can just hand out the primary worldstate storage with whatever bonsai context the caller is requesting.
There was a problem hiding this comment.
Perhaps we should have a different BonsaiWorldState subclass that better reflects that it is non-persisting and limited to flatdb only for reads. Right now we use non-persisting worldstates to propose blocks, so we might need some other "marker" to indicate we can hand out a worldstate that is both non-persisting and non-mutable
There was a problem hiding this comment.
I think Layered storage is already doing this. It is not a clone and prevents modification. I would like to limit the number of classes to keep the same logic as much as possible. We also have the notion of Frozen BonsaiWorldState . The idea imo is to have a single worldstate and give a storage to this one. And at the storage level it will be different . Maybe I'm wrong but I think we have to think about this because we did the refactoring to reduce the number of worldstate class etc and I think that we must avoid redoing what we removed
So I would say that we should have just a Layered Storage that wraps it and give it to the worldstate. And we freeze the worldstate as we are doing with bonsai before rpc call. Then find out if we should have a special worldstate for flat DB. I think we have to wait to see the implementation of the checkpointed trie. We could access in one worldstate to the flat db directly and the checkpointed trie in lazy mode.
There was a problem hiding this comment.
Freeze's call is already wrapping the storage with layered storage https://github.com/hyperledger/besu/blob/main/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/worldview/BonsaiWorldState.java#L592
Here
There was a problem hiding this comment.
Finally, I think that it is in the creation of the worldstate storage that we will give the context of the block number.
BonsaiWorldstate doesn't need to know this.
ca74aab to
73edff2
Compare
| /** | ||
| * record type used to wrap responses from getNearestTo, includes the matched key and the value. | ||
| * | ||
| * @param key the matched (nearest) key |
Check notice
Code scanning / CodeQL
Spurious Javadoc @param tags
| * record type used to wrap responses from getNearestTo, includes the matched key and the value. | ||
| * | ||
| * @param key the matched (nearest) key | ||
| * @param value the corresponding value |
Check notice
Code scanning / CodeQL
Spurious Javadoc @param tags
d0e7d08 to
9d9fe8c
Compare
5e359f9 to
511ec7e
Compare
511ec7e to
9d9fe8c
Compare
d11389d to
21a23ba
Compare
Signed-off-by: garyschulte <garyschulte@gmail.com>
21a23ba to
eea30f8
Compare
|
Context from conversation with @mattnelson, this implementation is a single column family implementation. Read and write performance degrades as multiple versions 'pile up' in the flat worldstate column families. A partitioned version of this implementation that uses separate column families for 'hot' (read/write) and a 'history' (read-only) should prevent degradation of execution performance, and should provide adequate historical RPC performance for use cases like block explorers. @matkt, @jframe - this would be like a manual checkpoint implementation, where the check point is dynamic, and we use trie logs to push old state into the history partition. |
seems to be a good idea |
|
@garyschulte and @matthew1001 Do we still need this draft PR open? |
PR description
Draft version of multi-version flat db
There are a handful of hacks that are marked with TODOs that are there for testing expedience.
skipping CI while in draft form
Fixed Issue(s)
fixes #5846