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: make catchpoint generation backward compatible #5598

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Jul 24, 2023

Summary

Stateproof verification introduced a new data chunk in a catchpoint file and an extra hash in labels that made impossible to get pre spver catchpoint labels.
This PR fixed it by checking consensus version for appropriate block and either include or not new data into consideration.

Changes overview

  1. Wire block protocol to catchpoint creation and label calculation function
  2. Make stateproof verification data hashing conditional based on EnableCatchpointsWithSPContexts consensus parameter.
  3. In order to support catchpoint crash recovery, introduce cachedDbRound into catchpoint tracker
  • protocol version is obtained is obtained from a block header in loadFromDisk for the dbRound so this tracker must
    maintain min round it can recover from similarly to acct updates and online accounts
  • this was caught by TestArchival

Test Plan

  1. Existing tests
  2. Betanet fast catchup testing:
    I run FC from 26770000#4VW7BZEW32TTEIXFRH2F5KRRVPDM3KKHU3FP2JAWENSKZE3FSRBQ (that is before the upgrades at 26968527 and 26988528) and got labels:
    26780000#VVZUVAPGK64IGPBMQIJUSQN6Q6YYOQW5SE2RIIKIRGMKLESN77OA
    26790000#ETD2KUYGW5OCJUPMXNFEYGVIEX74ITXCXTU5CTLCM5N2AHXECK7Q
    26800000#ACFA5RM2DQD4DE2GKKZOHYV7O26VY66WQT7LJCJ237HRZTC5BCAA
    26810000#QVNFYQW35WUU3EBQKRHXY25GGGQOVGWX25H6CC7UIJPBDAOGWEVQ
    26820000#FUBJMDI2AIGALRJK2Z3GQ5GCNHKAKUG2M2A6IBNOMGASA26ZPRFA
    26830000#W7W6OWNXOKTMT744ET327JBILSQQJPYTZWX3OZKGB2365QBKUYLA
    ...
    26900000#3EGYBUNV4CDVJLN5NJCCGSZ5UJAAO52JC52FX7STHL5GR7SJP3IA
    ...
    26980000#CVRJ3GVF5YWOSITJKOAR375IVDJN7HPWOWX33NV3LERPHMCR7XFA
    26990000#LG6RCETKULFHHPY6HHFACOZ4OPOEGYISILKCJZ7NFTZAZQCS2QZA
    27000000#CINW2LDMJN3DT6VGRRE2WATP7GY6X2WXCNOBILMNXKJDA3GGSHVQ
    27010000#LT4DYXFOETSO5KHKYBA2LFOUHQJRQXHJXD5MTOTS3C3YX5LMR2QA

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #5598 (7791321) into master (92af372) will decrease coverage by 0.01%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #5598      +/-   ##
==========================================
- Coverage   55.64%   55.64%   -0.01%     
==========================================
  Files         475      475              
  Lines       66869    66897      +28     
==========================================
+ Hits        37209    37222      +13     
- Misses      27146    27164      +18     
+ Partials     2514     2511       -3     
Files Coverage Δ
ledger/tracker.go 76.67% <ø> (-1.75%) ⬇️
ledger/catchpointtracker.go 62.89% <92.59%> (+2.16%) ⬆️

... and 15 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@algorandskiy algorandskiy marked this pull request as ready for review August 2, 2023 19:12
@algorandskiy algorandskiy changed the title ledger: make catchpoint generation backward compat ledger: make catchpoint generation backward compatible Aug 2, 2023
Copy link
Contributor

@AlgoAxel AlgoAxel left a comment

Choose a reason for hiding this comment

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

Generally looks good, I feel good about the basic branching logic for making a V6 catchpoint label maker. But I'm going to spend a little more time understanding the new wiring around it, specifically the ct.cachedDBRound and ct.consensusVersion. Will do another pass on this.

ledger/catchpointtracker.go Show resolved Hide resolved
ledger/catchpointtracker.go Show resolved Hide resolved
jannotti
jannotti previously approved these changes Oct 19, 2023
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.

Seems right to me.

* catchpoint's crash recovery needs to be protocol versions aware
* protocol is obtained from a block header so this tracker must
  maintain min round it can recover from similarly to acct updates
  and online accounts

This was caught by TestArchival test
@algorandskiy
Copy link
Contributor Author

rebased/resolved conflicts

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.

Put two questions that popped up in-line - I think looks good once those are addressed (based on betanet catchup as well).

@algorandskiy
Copy link
Contributor Author

Resolved comments

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.

One comment, once that's answered I'm good to approve.

ledger/tracker.go Show resolved Hide resolved
@algorandskiy algorandskiy merged commit c1207a4 into algorand:master Nov 6, 2023
18 checks passed
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.

7 participants