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: fix catchpoint pending hashes locking #5534

Merged
merged 1 commit into from Jul 10, 2023

Conversation

icorderi
Copy link
Contributor

Summary

#5451 introduced a bug, the catchpoint merkle tree generation needs to be kept on a separate readonly connection for it to make forward progress.

This PR add the required method to the Reader interface for the store, so it becomes available via the Snapshot scope.
There is an implicit contract that the snapshot is going through a separate read-only handle to the sqlite db.
If this were ever to change, this will break again.

Test Plan

@icorderi icorderi force-pushed the fix/reader-writer-catchpoint branch from b56a599 to 3a254c3 Compare July 10, 2023 12:29
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Merging #5534 (3a254c3) into master (e215f6d) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #5534      +/-   ##
==========================================
- Coverage   55.83%   55.82%   -0.02%     
==========================================
  Files         446      446              
  Lines       63262    63262              
==========================================
- Hits        35323    35313      -10     
- Misses      25566    25583      +17     
+ Partials     2373     2366       -7     
Impacted Files Coverage Δ
...edger/store/trackerdb/sqlitedriver/sqlitedriver.go 0.00% <0.00%> (ø)
ledger/catchupaccessor.go 64.79% <100.00%> (ø)

... and 15 files with indirect coverage changes

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

Copy link
Contributor

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice to setup a regression test to guard against this same bug in the future.

@algorandskiy
Copy link
Contributor

Agree, a test would be nice but it appears fairy hard to repro on small DB. I think it is worth investigating if the MT could write data into a temporal DB since it is not needed after finishing the FC process (well, unless one want to produce catchpoints but it is handled on startup - MT will be regenerated if the config wants to).

@algorandskiy algorandskiy merged commit a1569e5 into algorand:master Jul 10, 2023
17 checks passed
@algorandskiy algorandskiy changed the title fix: catchpoint pending hashes locking ledger: fix catchpoint pending hashes locking Jul 10, 2023
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

3 participants