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.

Sign up for GitHub

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

Jump to bottom

4034 implement sql invariants in bucketdb #4189

Merged
merged 1 commit into from
Feb 23, 2024
Merged

4034 implement sql invariants in bucketdb #4189

merged 1 commit into from
Feb 23, 2024

Conversation

ThomasBrady
Copy link
Contributor

@ThomasBrady ThomasBrady commented Feb 13, 2024 •
edited
Loading

Description

Adds functions to LedgerTxn.cpp to enforce pre and post load invariants on key loads, lifted from the various LedgerTxn*SQL.cpp implementations.

Resolves #4034

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

SirTyson
SirTyson requested changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small things but LGTM, thanks!

src/ledger/LedgerTxn.cpp Outdated
case OFFER:
// Ensure offer is not XLM -- possibly?
// special case for offer loading from allow trust op frame?
// See LedgerTxnOfferSQL.cpp::loadOffersByAccountAndAsset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadOffersByAccountAndAsset is actually only ever performed on SQL. Even if BucketListDB is enabled, we still keep the offers table around for this very query. It's very expensive to run on a DB without relational tables. You should be fine omitting the check here, but it might be good to include a comment as to why we don't need that specific invariant for BucketListDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense, thanks!

src/ledger/LedgerTxn.cpp Outdated
void
enforcePreLoadInvariants(LedgerKey const& key, uint32_t ledgerVersion)
{
switch (key.type())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since TRUSTLINE is the only type here that needs this check, probably cleaner to use and if statement instead of switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I wrote the switch before I realized how little invariants we are actually enforcing 😅

src/ledger/LedgerTxn.cpp Outdated
@@ -2986,9 +3068,17 @@ LedgerTxnRoot::Impl::prefetch(UnorderedSet<LedgerKey> const& keys)
{
insertIfNotLoaded(keysToSearch, key);
}

for (auto key : keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto const&

src/ledger/LedgerTxn.cpp Outdated
auto blLoad = mApp.getBucketManager().loadKeys(keysToSearch);
cacheResult(populateLoadedEntries(keysToSearch, blLoad));
auto loadedEntries = populateLoadedEntries(keysToSearch, blLoad);
for (auto entry : loadedEntries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto const&

src/ledger/LedgerTxn.cpp Outdated
return;
}
releaseAssert(key.type() == entry->data.type());
switch (key.type())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If statement instead of switch.

@ThomasBrady ThomasBrady force-pushed the 4034-implement-sql-invariants-in-bucketdb branch from 814895b to 5b66603 Compare February 14, 2024 19:38
@ThomasBrady ThomasBrady requested a review from SirTyson February 14, 2024 19:39
SirTyson
SirTyson approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small issue. I don't have merge authority, so might want to add @sisuresh as a reviewer as well since he opened the original issue.

src/ledger/LedgerTxn.cpp Outdated
@@ -2986,9 +3023,17 @@ LedgerTxnRoot::Impl::prefetch(UnorderedSet<LedgerKey> const& keys)
{
insertIfNotLoaded(keysToSearch, key);
}

for (auto const& key : keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do this in insertIfNotLoaded loaded loop, no need to loop twice.

src/ledger/LedgerTxn.cpp Outdated
// See LedgerTxnOfferSQL.cpp::loadOffersByAccountAndAsset.
}
void
enforcePostLoadInvariants(LedgerKey const& key,
Copy link
Contributor

@SirTyson SirTyson Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Rename to something like enforceBucketListPostLoadInvariants so it's clear we only use this for BucketListDB loads and not SQL, same goes for the other function as well.

@ThomasBrady ThomasBrady requested a review from sisuresh February 15, 2024 19:10
@ThomasBrady ThomasBrady force-pushed the 4034-implement-sql-invariants-in-bucketdb branch from 5b66603 to 87a8026 Compare February 15, 2024 19:16
@ThomasBrady ThomasBrady marked this pull request as ready for review February 15, 2024 19:16
@ThomasBrady ThomasBrady requested review from sisuresh and removed request for sisuresh February 20, 2024 23:52
sisuresh
sisuresh reviewed Feb 22, 2024
View reviewed changes
@ThomasBrady ThomasBrady force-pushed the 4034-implement-sql-invariants-in-bucketdb branch from 87a8026 to 7180ed7 Compare February 22, 2024 23:38
@ThomasBrady ThomasBrady requested a review from sisuresh February 22, 2024 23:39
@sisuresh
Copy link
Contributor

r+ 7180ed7

@latobarita latobarita merged commit 25455f9 into stellar:master Feb 23, 2024
15 checks passed
@sisuresh sisuresh mentioned this pull request Mar 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SirTyson SirTyson SirTyson approved these changes

@sisuresh sisuresh Awaiting requested review from sisuresh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement SQL invariants in BucketDB
4 participants
@ThomasBrady @sisuresh @latobarita @SirTyson