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

catchpoints: Add onlineaccounts and onlineroundparamstail tables to snapshot files #6177

Merged
merged 25 commits into from
Dec 20, 2024

Conversation

cce
Copy link
Contributor

@cce cce commented Nov 25, 2024

Summary

The new opcodes in #5984 require access to historical lookups of online account details and total circulation amounts. During catchpoint restore, replaying and evaluating old blocks bootstraps the tracker DBs. However transactions that use these opcodes will fail, since they need access to historical ledger amounts earlier than the horizon of the catchpoint snapshot. This PR adds the onlineaccounts and onlineroundparamstail tables to the snapshot files, which provide online account lookup and total circulation amounts, respectively.

Test Plan

Existing tests should pass — added a test TestCatchpointAfterStakeLookupTxns to use the voter_params_get and online_stake opcodes (similar to some catchpoint tests written for testing use of box opcodes).

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 42.09246% with 238 lines in your changes missing coverage. Please review.

Project coverage is 51.93%. Comparing base (a896dfd) to head (767e369).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
ledger/store/trackerdb/sqlitedriver/kvsIter.go 0.00% 58 Missing ⚠️
ledger/catchupaccessor.go 63.80% 25 Missing and 13 partials ⚠️
ledger/store/trackerdb/sqlitedriver/catchpoint.go 0.00% 37 Missing ⚠️
.../store/trackerdb/dualdriver/accounts_reader_ext.go 0.00% 20 Missing ⚠️
ledger/catchpointfilewriter.go 77.21% 9 Missing and 9 partials ⚠️
ledger/store/trackerdb/sqlitedriver/accountsV2.go 0.00% 14 Missing ⚠️
ledger/ledgercore/catchpointlabel.go 27.77% 13 Missing ⚠️
ledger/catchpointtracker.go 81.25% 5 Missing and 4 partials ⚠️
...edger/store/trackerdb/sqlitedriver/sqlitedriver.go 0.00% 6 Missing ⚠️
ledger/store/trackerdb/dualdriver/dualdriver.go 0.00% 5 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6177      +/-   ##
==========================================
- Coverage   52.00%   51.93%   -0.08%     
==========================================
  Files         642      643       +1     
  Lines       85898    86223     +325     
==========================================
+ Hits        44674    44777     +103     
- Misses      38386    38584     +198     
- Partials     2838     2862      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ledger/catchpointfileheader.go Show resolved Hide resolved
ledger/catchpointfilewriter.go Show resolved Hide resolved
ledger/catchpointtracker.go Show resolved Hide resolved
ledger/ledgercore/catchpointlabel.go Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Implementation looks correct. Our versioning is very confusing though

ledger/catchpointfilewriter_test.go Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM. I guess a manual test needs to be done:

  1. make a new catchpoint and restore using new algod. Ensure it joins the network and processes blocks/transactions.
  2. make sure the old algod rejects a new catchpoint with a clear error message

ledger/catchupaccessor.go Outdated Show resolved Hide resolved
algorandskiy
algorandskiy previously approved these changes Dec 18, 2024
ledger/catchupaccessor.go Outdated Show resolved Hide resolved
gmalouf
gmalouf previously approved these changes Dec 19, 2024
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Submitted my comments, tests were failing at last check so pending resolving those.

@cce cce dismissed stale reviews from gmalouf and algorandskiy via 8adddbc December 19, 2024 19:39
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM. I'll an e2e test as a followup

@algorandskiy
Copy link
Contributor

Please merge master in (to fix #6205)

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Wow. I read it all, but catchpoints are a huge mess. I think everything makes sense, in so far as one can claim the existing code makes sense.

config/consensus.go Show resolved Hide resolved
ledger/catchpointfilewriter.go Show resolved Hide resolved
@cce cce merged commit a10b6b3 into algorand:master Dec 20, 2024
18 of 19 checks passed
@cce cce deleted the catchpoint-onlineaccts-separatehashes branch December 20, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants