Switch read-only LedgerTxn to BucketList snapshots #4431
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marta-lokhova
force-pushed
the
removeReadOnlyLedgerTxn
branch
5 times, most recently
from
ec3d1ce
to
8540866
Compare
sisuresh
reviewed
Aug 21, 2024
SirTyson
requested changes
Aug 21, 2024
There was a problem hiding this comment.
Overall a very helpful change! A few comments about hardening some asserts around BucketList based load access. Other than that, I think moving the snapshot logic into more of the domain of the LedgerManager will help us consolidate the snapshotting logic, as well as prevent potential footguns in the future.
dmkozh
reviewed
Aug 21, 2024
marta-lokhova
force-pushed
the
removeReadOnlyLedgerTxn
branch
4 times, most recently
from
d1c7816
to
71b5f89
Compare
|
I believe all the comments are addressed now, and parallel catchup on both testnet and pubnet passed. PTAL. I haven't squashed newer commits so it's easier to review, but I can do so once the PR is approved. |
SirTyson
previously approved these changes
Sep 9, 2024
Thanks for the change, LGTM! |
dmkozh
reviewed
Sep 10, 2024
marta-lokhova
force-pushed
the
removeReadOnlyLedgerTxn
branch
7 times, most recently
from
4315f1c
to
f4f04b0
Compare
marta-lokhova
force-pushed
the
removeReadOnlyLedgerTxn
branch
from
b5f0423
to
cbb7de3
Compare
dmkozh
approved these changes
Sep 11, 2024
…ingle snapshot interface
marta-lokhova
force-pushed
the
removeReadOnlyLedgerTxn
branch
from
cbb7de3
to
2f6c917
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #4315 and #3800 This PR automatically switches overlay/herder to using BucketListDB snapshots instead of LedgerTxn.
This change introduces a single "read-only ledger state snapshot" interface, which supports both SQL (via LedgerTxn) and BucketListDB snapshots. A unified interface avoids invasive changes to the rest of the codebase to support both snapshot types, and allows to have a centralized validation flow for both overlay and apply time.
A notable change is the removal of nested LegderTxn in validation paths, and using loadWithoutRecord instead in newer protocols. This creates a stronger invariant that validation flow is only meant to be accessing read-only version of the ledger, and prevents it from making modifications by accident. Note that nested LedgerTxn is preserved for older buggy versions of the protocol (making the code a bit uglier than I would have preferred, I'm open to ideas on simplifying/refactoring old protocol logic out of the critical path).