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

Spins up AccountsHashVerifier in ledger-tool #31358

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Apr 26, 2023

Problem

When creating a snapshot with ledger-tool, and when the Epoch Accounts Hash feature-gate is enabled (#27539), ledger-tool will hang if it replays a slot that needs to save an EAH.

This is because the bank at that (☝️) slot needs to wait for the EAH request to be handled and for the EAH to be calculated.

AccountsHashVerifier is where EAH requests are handled, but AHV is not spun up for ledger-tool. This means that all ledger-tool commands that replay slots (like verify and create-snapshot) that cross an EAH boundary will hang waiting for a valid EAH that will never be.

Summary of Changes

Spin up AHV in ledger-tool.

Fixes #31351

@brooksprumo brooksprumo added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 26, 2023
@brooksprumo brooksprumo self-assigned this Apr 26, 2023
@brooksprumo brooksprumo force-pushed the eah/ledger-tool-ahv branch from 2396d62 to c13a1ac Compare April 26, 2023 15:19
@brooksprumo brooksprumo removed the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 26, 2023
@brooksprumo brooksprumo marked this pull request as ready for review April 26, 2023 15:46
Comment on lines +1271 to +1276
let node_id = Arc::new(Keypair::new());
let cluster_info = Arc::new(ClusterInfo::new(
ContactInfo::new_localhost(&node_id.pubkey(), timestamp()),
Arc::clone(&node_id),
SocketAddrSpace::Unspecified,
));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AccountsHashVerifier needs a ClusterInfo to push the accounts hashes to gossip. But since we don't actually care about gossip in ledger-tool, just use default/dummy values.

None,
false,
None,
SnapshotConfig::new_load_only(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, we could use the snapshot_config that was created above, but we don't want to create and snapshot archives with AHV here; it's only used to service EAH requests.

Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #31358 (c13a1ac) into master (466c3c3) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #31358     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         734      734             
  Lines      207153   207153             
=========================================
- Hits       168954   168948      -6     
- Misses      38199    38205      +6     

@brooksprumo brooksprumo merged commit c73b654 into solana-labs:master Apr 26, 2023
@brooksprumo brooksprumo deleted the eah/ledger-tool-ahv branch April 26, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

EpochAccountsHash doesn't play nicely when the full snapshot interval is greater than the slots per epoch
2 participants