-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat!: fix horizon sync after smt upgrade #6006
feat!: fix horizon sync after smt upgrade #6006
Conversation
1fa90d3
to
0a22fb3
Compare
Test Results (CI)1 268 tests 1 268 ✅ 11m 9s ⏱️ Results for commit 658679f. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files 11 suites 15m 41s ⏱️ For more details on these failures, see this check. Results for commit 658679f. ♻️ This comment has been updated with latest results. |
cc526fa
to
9bae457
Compare
a5b7550
to
1c005e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
WIP notice: The system-level use needs some love, specifically cleaning out the output tables after a failed attempt. Edit:: System level tests pass doing horizon sync from scratch and form an old or new archival node. |
79d8591
to
7401d33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just some comments
base_layer/core/src/base_node/state_machine_service/states/horizon_state_sync.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/state_machine_service/states/sync_decide.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs
Outdated
Show resolved
Hide resolved
7401d33
to
15a6c18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can fix this in a next PR or this one.
base_layer/core/src/base_node/state_machine_service/states/horizon_state_sync.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/state_machine_service/states/horizon_state_sync.rs
Outdated
Show resolved
Hide resolved
f7e7136
to
9931495
Compare
- Added integration-level horizon sync unit tests. - Fixed horizon sync: initial, re-sync, re-sync after being offline. - Added logic to detect genesys block outputs being spend. - Fixed an issue where a tip block body could not be inserted due to the input being a compact input. - Added integration-level block sync unit tests to verufy ^^.
9931495
to
658679f
Compare
Description
Renamed some struct members and functions related to these changes to depict their use better.fn fetch_outputs_in_block_with_spend_state(...)
whereby it did not filter out outputs with a spent state at the target header. Updated integration level unit testtest_horizon_sync_from_archival_node_happy_path()
to verify this behaviour.fn prune_outputs_spent_at_hash(..)
whereby it used the wrong key(s) to try and prune outputs.Note: Initial prune node sync can still be optimized if we can allow it to happen from another prune node, as this PR restricts initial prune node sync from an archival node. That is left for another PR.
Motivation and Context
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Code walk-through.
Run the integration-level horizon sync and block sync unit tests.
Selected output of
test_horizon_sync_from_archival_node_happy_path()
with trace logs is added here to assist reviewers (pr_#6006.log):Breaking Changes
BREAKING CHANGE: Sync nodes can only sync from base nodes running the same or later software version