-
Notifications
You must be signed in to change notification settings - Fork 838
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
Bonsai snapshot worldstate #4409
Bonsai snapshot worldstate #4409
Conversation
936df87
to
d983043
Compare
Excellent work I will review it on Monday or this weekend |
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 will continue the review but i already share this
} | ||
|
||
public static class SnapshotUpdater implements BonsaiWorldStateKeyValueStorage.BonsaiUpdater { | ||
// private static final Logger LOG = |
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 you missed this line
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.
Seems to be ok. waiting for the next PR related to snapshot
d1edc6b
to
12c045c
Compare
01725fe
to
c675a28
Compare
c675a28
to
158937a
Compare
IMO, this is ready to merge to main to clear the way for subsequent PRs that implement bonsai snapshots. |
4fe232b
to
04a0820
Compare
this.trieBranchSnap = (SnappedKeyValueStorage) snapshotWorldStateStorage.trieBranchStorage; | ||
} | ||
|
||
public static BonsaiSnapshotWorldState create( |
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.
Is there a potential race with the storages being changed while the snapshots are generated?
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.
There is some risk of that theoretically, but considering we write to rocksdb in batch commits and the snapshots are essentially atomic, this is probably very very minimal chance in practice. There is certainly far less concurrency risk than our current strategy where the underlying persisted state can change underneath a BonsaiInMemoryWorldState or BonsaiLayeredWorldState
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.
It seems reasonable to implement locking in BonsaiPersistedWorldState so that we do not commit a batch write (via calculateRootHash) and take a snapshot concurrently. It should be a very short lock for getting a snapshot, and delaying getting a snapshot while waiting on a lock to release from a batch write would normally not be significant.
I will add this in one of the subsequent snapshot PRs and add tests 👍
} catch (final RocksDBException e) { | ||
if (e.getMessage().contains(NO_SPACE_LEFT_ON_DEVICE)) { | ||
LOG.error(e.getMessage()); | ||
System.exit(0); |
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.
System.exit feels too brutal to me. Is there any chance this can corrupt db for instance in case an ongoing update is happening in parallel? Maybe we don't access db from different threads.
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 question. This is the same behavior as RocksDBTransaction. The right course of action isn't really clear since we do not have any disk space to work with.
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Isolation is working, but unclear whether trielogs are persisting correctly since we cannot create a snapshot and roll it to a particular hash in unit tests Signed-off-by: garyschulte <garyschulte@gmail.com>
snapshot trieLog is persistable snapshot roll forward/back is FAILING 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>
…correctly dispose of snapshots 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>
… extend persisted worldstate rather than in-memory worldstate Signed-off-by: garyschulte <garyschulte@gmail.com>
close snapshot worldstate transactions on close release snapshot on close of snapshot transaction (potentially fraught) Signed-off-by: garyschulte <garyschulte@gmail.com>
04a0820
to
947ae51
Compare
* use optimistictransactiondb for mutable isolated snapshots * plumbing necessary to have a snapshot specific updater. * snapshot rolling working * implement AutoCloseable on BonsaiSnapshotWorldState to ensure we can correctly dispose of snapshots * add snapshot transaction cloning, change snapshot based worldstate to extend persisted worldstate rather than in-memory worldstate Signed-off-by: garyschulte <garyschulte@gmail.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
* use optimistictransactiondb for mutable isolated snapshots * plumbing necessary to have a snapshot specific updater. * snapshot rolling working * implement AutoCloseable on BonsaiSnapshotWorldState to ensure we can correctly dispose of snapshots * add snapshot transaction cloning, change snapshot based worldstate to extend persisted worldstate rather than in-memory worldstate Signed-off-by: garyschulte <garyschulte@gmail.com>
PR description
This PR adds support for using rocksdb snapshot transactions as the basis for a bonsai worldstate. This gives us the ability to have an isolated copy of the world state for concurrent operations like block production, block validation, transaction validation, historical state queries, etc.
The notion is to merge this capability into main, and start replacing the uses of BonsaiLayeredWorldState and direct uses of BonsaiInMemoryWorldState when we need isolated and/or concurrent operations on the worldstate.
This is a strictly additive PR and does not change existing implementations. Uses of snapshot worldstates will be useful to resolve issues such as #4372 #4250 #4199 #4151 and other bonsai concurrency issues.
Changes in support of snapshot worldstate include:
BonsaiWorldStateKeyValueStorage.BonsaiUpdater
interface extendingWorldStateStorage.Updater
, so we can have a snapshot specific Updater implementationFixed Issue(s)
relates to #4402
relates to #4403
Documentation
doc-change-required
label to this PR ifupdates are required.
Changelog