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

Sub function getting vetted RPC nodes #30946

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

bw-solana
Copy link
Contributor

Problem

rpc_bootstrap function is pretty unwieldy. Sub-functioning the process to get vetted RPC nodes improves readability. This also sets the stage for decoupling parts of the full/incremental snapshot download

Summary of Changes

Add function for getting vetted RPC nodes

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #30946 (ceab5a8) into master (b72be0f) will increase coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #30946   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         728      728           
  Lines      205475   205480    +5     
=======================================
+ Hits       167560   167578   +18     
+ Misses      37915    37902   -13     

@bw-solana bw-solana marked this pull request as ready for review March 29, 2023 02:32
@bw-solana bw-solana requested a review from brooksprumo March 29, 2023 02:32
validator/src/bootstrap.rs Outdated Show resolved Hide resolved
gossip: &Option<(Arc<ClusterInfo>, Arc<AtomicBool>, GossipService)>,
cluster_entrypoints: &[ContactInfo],
validator_config: &ValidatorConfig,
blacklisted_rpc_nodes: &RwLock<HashSet<Pubkey>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Well... this is unfortunate... I wish get_rpc_node() and fail_rpc_node() didn't modify blacklisted_rpc_nodes. I think for this PR the refactoring here is fine. Maybe future PRs can help clean this up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked into this. Unfortunately, there is a place we might clear blacklisted_rpc_nodes that is deep in the depths of the call stack. Going to be really difficult to allow blacklisted_rpc_nodes to be immutable at this level.

Tried to improve things slightly by stripping the RwLock from blacklisted_rpc_nodes here: #30987

// Populates `vetted_rpc_nodes` with a list of RPC nodes that are ready to be
// used for downloading latest snapshots and/or the genesis block. Guaranteed to
// find at least one viable node or terminate the process.
fn get_vetted_rpc_nodes(
Copy link
Contributor

Choose a reason for hiding this comment

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

And similarly to the previous comment, it'd be nice if this function returned the vetted rpc nodes, instead of mutating them internally. Nothing to do for this PR (maybe future ones though)!

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@bw-solana bw-solana added the automerge Merge this Pull Request automatically once CI passes label Mar 29, 2023
@mergify mergify bot merged commit 9f7e041 into solana-labs:master Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants