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.

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

LedgerTxn: audit shouldUpdateLastModified usage in read-only scenarios #3800

Closed
marta-lokhova opened this issue Jun 28, 2023 · 1 comment · Fixed by #4471
Closed

LedgerTxn: audit shouldUpdateLastModified usage in read-only scenarios #3800

marta-lokhova opened this issue Jun 28, 2023 · 1 comment · Fixed by #4471
Assignees
Labels

Comments

@marta-lokhova
Copy link
Contributor

marta-lokhova commented Jun 28, 2023

It appears that LedgerTxn's flag shouldUpdateLastModified is used inconsistently across the codebase (we now use LedgerTxn a lot more often to load the network config), as sometimes it's set to true and sometimes it's false:

LedgerTxn ltx(mApp.getLedgerTxnRoot(), false,

LedgerTxn ltx(mApp.getLedgerTxnRoot(), /* shouldUpdateLastModified */ true,

LedgerTxn ltx(rootLtx, false,

This should not matter for read-only cases, where we do things like

{
        LedgerTxn ltx(mApp.getLedgerTxnRoot(), false,
                      TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
        auto const& conf = mApp.getLedgerManager().getSorobanNetworkConfig(ltx);
        ... do something with conf...
        ... end of scope, automatic ltx rollback...
}

but in general, this is a footgun that should be fixed. We should audit the usage and see if the flag can be removed (i.e. the condition is always true). If the flag is needed for tests only, we can have a special constructor behind BUILD_TESTS macro.

@marta-lokhova
Copy link
Contributor Author

This issue will be closed by implementing #4315

github-merge-queue bot pushed a commit that referenced this issue Sep 11, 2024
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).
github-merge-queue bot pushed a commit that referenced this issue Oct 16, 2024
Part of #4318 - main
change is to stop passing around Application during validation and
application flows, and use AppConnector instead, which is forced to
either assert main thread or implement thread-safe methods

Resolves #3800 - note that
READ_ONLY_WITHOUT_SQL_TXN LedgerTxn mode should go away completely once
we switch to mandatory BucketListDB. It looks like we were using RO
LedgerTxn during apply scenarios, which I think is not legal? Either
way, all READ_ONLY_WITHOUT_SQL_TXN usages except for the legacy one in
LedgerSnapshot have been removed now.

Note that the second commit has a huge diff but is a no-op, as it's
basically a find and replace for `Application -> AppConnector`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant