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

ledger: move spver context hash calculation out from catchpoint writing #5574

Closed

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Jul 17, 2023

Summary

  1. stateproof verification snapshot logic does not handle CatchpointTracking=1 properly - its hash calculation is a side effect of writing spver data chunk into catchpoint file. Fixed that by calculating hash similarly to balance hash on demand.
  2. Improve catchpointdump to allow rebuilding MT and print accounts and stateproof hashes.

Test Plan

  1. Modified unit test for CatchpointTracking=1
  2. Tested locally

algonautshant
algonautshant previously approved these changes Jul 17, 2023
Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

LGTM

@algorandskiy algorandskiy changed the title cmd: catchpointdump utility prints hashes ledger: move spver context hash calculation out from catchpoint writing Jul 17, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #5574 (85f8690) into master (d316914) will increase coverage by 0.02%.
The diff coverage is 65.11%.

@@            Coverage Diff             @@
##           master    #5574      +/-   ##
==========================================
+ Coverage   55.79%   55.81%   +0.02%     
==========================================
  Files         446      446              
  Lines       63418    63425       +7     
==========================================
+ Hits        35381    35401      +20     
+ Misses      25668    25656      -12     
+ Partials     2369     2368       -1     
Impacted Files Coverage Δ
ledger/catchpointwriter.go 62.34% <50.00%> (ø)
ledger/catchupaccessor.go 64.80% <62.50%> (+<0.01%) ⬆️
ledger/catchpointtracker.go 59.64% <76.92%> (-0.05%) ⬇️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

LGTM

@jsgranados
Copy link
Contributor

Just a suggestion, but the ledger readme doesn't mention the stateproof tracker. Should we add that?

}

wrappedContext := catchpointStateProofVerificationContext{Data: rawData}
stateProofVerificationHash = crypto.HashObj(wrappedContext)
Copy link
Contributor

@cce cce Jul 18, 2023

Choose a reason for hiding this comment

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

You are adding an extra DB call to GetAllSPContexts here, to get the hash and drop the data. Then later in WriteStateProofVerificationContext() the DB is called again to get the data but throw away the hash. I guess it is safe to assume the DB will return the same data/hash between the two calls (is it possible it won't?). The data is written out when finishFirstStage is called by finishFirstStageAfterCrash for example and I wasn't sure whether there are cases where things can get mixed up between the two DB calls to get the SP data..

I'm looking at the code now trying to figure out, is there a way to have the hash and data be generated from the same DB call.. just to have one less thing to worry about getting out of sync

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe if you just have if !ct.enableGeneratingCatchpointFiles around this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there is the same "issue" with accounts - they are written into a file in generateCatchpointData and hash is calculated later in recordFirstStageInfo by reading MT from DB. There is no difference in approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about a smaller change like this 7b28351

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could work, why not

Copy link
Contributor

Choose a reason for hiding this comment

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

Both calls come from lt.postCommitUnlocked, and the database is changed in lt.commitRound sequentially before the postCommitUnlocked call.

@cce
Copy link
Contributor

cce commented Jul 18, 2023

What do you think of #5579?

@algorandskiy
Copy link
Contributor Author

replaced by #5579

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.

None yet

4 participants