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

config: Add MaxBlockHistoryLookback option #5749

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

ohill
Copy link
Contributor

@ohill ohill commented Sep 21, 2023

Summary

There is no way to request a node to store more blocks if it is not archival. This adds a MaxBlockHistoryLookback similar to MaxAcctLookback config option.

Test Plan

No tests yet

@CLAassistant
Copy link

CLAassistant commented Sep 21, 2023

CLA assistant check
All committers have signed the CLA.

ledger/ledger.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #5749 (63c4c79) into master (ea20873) will increase coverage by 0.00%.
Report is 14 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5749   +/-   ##
=======================================
  Coverage   55.57%   55.57%           
=======================================
  Files         476      476           
  Lines       66857    66859    +2     
=======================================
+ Hits        37156    37160    +4     
+ Misses      27177    27175    -2     
  Partials     2524     2524           
Files Coverage Δ
config/localTemplate.go 72.02% <ø> (ø)
ledger/ledger.go 69.04% <100.00%> (+0.15%) ⬆️

... and 8 files with indirect coverage changes

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

@cce cce changed the title Add MaxBlockHistoryLookback config: Add MaxBlockHistoryLookback option Sep 21, 2023
jannotti
jannotti previously approved these changes Sep 22, 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 good to me.

Do we ever complain about non-sensical combinations of config? If someone sets this to, say, 3000 but also has Archival set, they are probably confused.

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.

the code LGTM but please add a unit test and perform a manual test.

algorandskiy
algorandskiy previously approved these changes Sep 28, 2023
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.

Thank you for the change!

@ohill
Copy link
Contributor Author

ohill commented Sep 28, 2023

Manually tested after catchpoint restore & running a node for a little while with "MaxBlockHistoryLookback": 1500 set:

$ last_round=$(curl -s -H "X-Algo-API-Token: $(cat algod.token)" localhost:8080/v2/status | jq '.["last-round"]') && curl -s -o /dev/null -w "%{http_code}" -H "X-Algo-API-Token: $(cat algod.token)" "localhost:8080/v2/blocks/$((last_round - 1500))"
200
$ last_round=$(curl -s -H "X-Algo-API-Token: $(cat algod.token)" localhost:8080/v2/status | jq '.["last-round"]') && curl -s -o /dev/null -w "%{http_code}" -H "X-Algo-API-Token: $(cat algod.token)" "localhost:8080/v2/blocks/$((last_round - 1501))"
404

@winder winder self-requested a review September 28, 2023 20:06
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.

6 participants