allow staked nodes weight override #26870

janlegner · 2022-08-01T15:35:21Z

Problem

As per issue #26407: The validator may want to prioritize QUIC traffic from a known whitelist of sources and apply its own stake to those transactions.

Summary of Changes

New parameter is added to the solana-validator program: --staked_nodes_overrides <PATH/URL>
A configuration file is periodically fetched from the given location (filesystem or HTTP)
The fetched configuration is applied to the staked nodes weight - effectively allowing whitelisting/prioritizing traffic from specified sources.

Config example

auto_reload: true
stake_map:
  1.1.1.1: 1000000000000000
  2.2.2.2: 4000000000000000

pgarg66 · 2022-08-01T19:18:55Z

It might be more useful to have overrides for pubkey -> stake mapping (instead of IP address -> stake). Most known connections will be from peer nodes that have active stake (e.g. previous leaders, or soon to be leaders). If the peer is staked, it's identity pubkey is being used to lookup its stake.

janlegner · 2022-08-02T14:16:36Z

It might be more useful to have overrides for pubkey -> stake mapping (instead of IP address -> stake). Most known connections will be from peer nodes that have active stake (e.g. previous leaders, or soon to be leaders). If the peer is staked, it's identity pubkey is being used to lookup its stake.

That is a good idea - but what about e.g. RPC nodes without vote pubkey? 🤔 Maybe allowing both IP and pubkey would be a good idea?

EDIT: or identity pubkey?

mvines · 2022-08-02T15:37:41Z

I don't like the idea of a new service within the validator that tries to keep this list updated. Instead we only need a couple primitives:

New solana-validator command-line argument to provide the data file
A new admin ipc method to signal a reload of the data file (cc:

solana/validator/src/admin_rpc_service.rs

Line 134 in ec36f0c

pub trait AdminRpc {

)

janlegner · 2022-08-03T07:43:41Z

I don't like the idea of a new service within the validator that tries to keep this list updated. Instead we only need a couple primitives:

New solana-validator command-line argument to provide the data file

A new admin ipc method to signal a reload of the data file (cc:

solana/validator/src/admin_rpc_service.rs

Line 134 in ec36f0c

pub trait AdminRpc {

)

Thanks for the suggestion, I did not think about using admin signals but that makes a lot of sense. Where do you think we should then move the logic loading the configuration from file/URL? Keeping it in core/src/... seems strange. I do not want to inflate validator/src/main.rs too much, so maybe validator/src/lib.rs?

mvines · 2022-08-03T15:29:50Z

I do not want to inflate validator/src/main.rs too much, so maybe validator/src/lib.rs?

Yep that seems great. Putting it in lib.rs will also make it easier to add to solana-test-validator if somebody wants the functionality added there as well one day

ochaloup · 2022-08-09T15:12:49Z

I did an update behalf of @janlegner on this PR. The yaml configuration is loaded via RPC admin command or as a configuration option at the start of the validator program. The configuration works with the node identities now (/cc @pgarg66) instead of the IPs.

@mvines would you be so kind and provide me a feedback on this PR?

mvines

nice progress!

mvines · 2022-08-10T15:30:31Z

validator/src/admin_rpc_service.rs

+    if let Ok(url) = Url::parse(path_or_url) {
+        return try_config_from_url(url);
+    }


My preference is to remove the URL support entirely. Local files only. It's trivial for the caller to wget the file, or use whatever other standard url fetching tooling they prefer.

+1, I changed for using only the path as you recommended.

mvines · 2022-08-10T15:32:14Z

validator/src/main.rs

+                .long("staked-nodes-overrides")
+                .value_name("FILE_OR_URL")
+                .takes_value(true)
+                .help("Provide file with custom overrides for stakes of specific IPs."),  // TODO: change IP to IP or identity


It'd be nice to include some more info in the .help() about what impact this overrides may have for the validator operationally, either as an inline or a call out to external documentation.

I added the more instructive description. I will be glad if you can comment on the way I did.

I searched for the documentation page that the description could be elaborated (not sure if it's the way to go now) but I haven't found any CLI reference guide where I could add that to. The solana cli has got the reference page that seems to be generated from the code (https://docs.solana.com/cli/usage#solana-cli). The solana-validator has got no reference guide. Should that added to some specific guide with even more details?

mvines · 2022-08-10T15:35:09Z

core/src/staked_nodes_updater_service.rs

@@ -21,18 +22,40 @@ pub struct StakedNodesUpdaterService {
    thread_hdl: JoinHandle<()>,
 }

+#[derive(Default, Deserialize, Clone)]
+pub struct StakedNodesOverrides {


I don't love how the placement of this struct here causes a number of extra downstream use of the staked_nodes_updater_service module, which really feels like it ought to be private to solana-core. Likewise, it necessitates a new serde_yaml dep for core.

Any way we can move the serde_yaml dep fully into validator/ instead?

Also are these files going to be large? It's been my experience that serde_yaml is very slow on large files (relative to serde_json)

The declaration was used to easily load various data from the struct. As it's the validator module that needs to provide the data to the core service I haven't thought an easy way here.

The struct could be defined without the serde_yaml dependency but then it still means multiple extra downstream use.
I haven't thought anything super clever and I eventually pass the HashMap as the shared structure for the data passing. I don't like the fact that any change of the format or adding types for overrides (e.g. the discussed IP) means a bit more changes.

Even though would be fine to do it the way I did?

About the serde_yaml. We like the yaml as it's easy to work with and the configuration file is not expected to be large. So I left there the loading from yaml for now. If you consider it should really be the json, I can change it.

ochaloup · 2022-08-11T13:18:12Z

@mvines thank you for the review. I hope I addressed most of the points (details in the comments above).

ochaloup · 2022-08-11T15:12:07Z

@mvines @pgarg66 As browsing through the code I would like to check with you one detail.

The goal of this PR is to override the stake amounts (as per pr#26407) to make a validator peer to be considered with some amount of the stake and then from that calculate QUIC streams and packet vote stage.
I did it now in way that the mapping is taken from the (https://github.com/solana-labs/solana/blob/v1.11.5/core/src/staked_nodes_updater_service.rs#L75) tvu_peers() and overrde is applied only if the validator identity is valid within the TVU ips. I think that this could be limiting. When the validator happens to be a valid TVU peer? For the QUIC stake overriding purpose of ip/id could I use rather the cluster_info.all_peers()?

Btw. could be two validators running with the same IP? Could be then the ip_to_stake map in clash in values (https://github.com/solana-labs/solana/blob/v1.11.5/core/src/staked_nodes_updater_service.rs#L82)?

pgarg66 · 2022-08-11T15:46:13Z

@mvines @pgarg66 As browsing through the code I would like to check with you one detail.

The goal of this PR is to override the stake amounts (as per pr#26407) to make a validator peer to be considered with some amount of the stake and then from that calculate QUIC streams and packet vote stage. I did it now in way that the mapping is taken from the (https://github.com/solana-labs/solana/blob/v1.11.5/core/src/staked_nodes_updater_service.rs#L75) tvu_peers() and overrde is applied only if the validator identity is valid within the TVU ips. I think that this could be limiting. When the validator happens to be a valid TVU peer? For the QUIC stake overriding purpose of ip/id could I use rather the cluster_info.all_peers()?

It makes sense to use all_peers(). It'll allow a validator to allocate more QUIC streams to a node that's not a staked validator (e.g. an RPC node, or a client).

Btw. could be two validators running with the same IP? Could be then the ip_to_stake map in clash in values (https://github.com/solana-labs/solana/blob/v1.11.5/core/src/staked_nodes_updater_service.rs#L82)?

It's technically possible for two validators, e.g. behind NAT, using the same IP address. For this reason, we are not using ip_stake_map in QUIC. We have a mechanism to get peer's identity pubkey during QUIC connection setup process, and that's used for looking up id_to_stake (pubkey_stake_map) table.

…re module

ochaloup · 2022-08-11T16:58:08Z

It makes sense to use all_peers(). It'll allow a validator to allocate more QUIC streams to a node that's not a staked validator (e.g. an RPC node, or a client).

I see. Thank you @pgarg66 for this great elaboration! I changed the code for all_peers() as suggested (if you can check it would be nice).

pgarg66 · 2022-08-11T17:03:13Z

It makes sense to use all_peers(). It'll allow a validator to allocate more QUIC streams to a node that's not a staked validator (e.g. an RPC node, or a client).

I see. Thank you @pgarg66 for this great elaboration! I changed the code for all_peers() as suggested (if you can check it would be nice).

Thanks for updating the PR. Looks good to me. Let's wait for @mvines to review it too, before we merge it.

mvines

lgtm, thanks!

* Allowed staked nodes weight override (solana-labs#26407) * Allowed staked nodes weight override, passing only HashMap over to core module Co-authored-by: Ondra Chaloupka <chalda@chainkeepers.io>

mergify bot added the community Community contribution label Aug 1, 2022

mergify bot requested a review from a team August 1, 2022 15:35

janlegner changed the title ~~Allowed staked nodes weight override~~ allow staked nodes weight override Aug 1, 2022

janlegner force-pushed the feat-staked-nodes-weight-override branch from fecbbc0 to 5a585f5 Compare August 1, 2022 17:40

ochaloup force-pushed the feat-staked-nodes-weight-override branch 5 times, most recently from 021ec77 to ab651bc Compare August 9, 2022 15:02

ochaloup force-pushed the feat-staked-nodes-weight-override branch from ab651bc to 5010b79 Compare August 10, 2022 06:35

mvines reviewed Aug 10, 2022

View reviewed changes

ochaloup force-pushed the feat-staked-nodes-weight-override branch from 5010b79 to d513e25 Compare August 11, 2022 12:44

ochaloup added 2 commits August 11, 2022 18:42

Allowed staked nodes weight override (solana-labs#26407)

e349407

Allowed staked nodes weight override, passing only HashMap over to co…

51d72de

…re module

ochaloup force-pushed the feat-staked-nodes-weight-override branch from d513e25 to 51d72de Compare August 11, 2022 16:58

mvines approved these changes Aug 11, 2022

View reviewed changes

pgarg66 merged commit fc6cee9 into solana-labs:master Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow staked nodes weight override #26870

allow staked nodes weight override #26870

janlegner commented Aug 1, 2022

pgarg66 commented Aug 1, 2022 •

edited

Loading

janlegner commented Aug 2, 2022 •

edited

Loading

mvines commented Aug 2, 2022

janlegner commented Aug 3, 2022

mvines commented Aug 3, 2022

ochaloup commented Aug 9, 2022

mvines left a comment

mvines Aug 10, 2022

ochaloup Aug 11, 2022

mvines Aug 10, 2022

ochaloup Aug 11, 2022

mvines Aug 10, 2022

ochaloup Aug 11, 2022 •

edited

Loading

ochaloup commented Aug 11, 2022

ochaloup commented Aug 11, 2022 •

edited

Loading

pgarg66 commented Aug 11, 2022

ochaloup commented Aug 11, 2022 •

edited

Loading

pgarg66 commented Aug 11, 2022

mvines left a comment

allow staked nodes weight override #26870

allow staked nodes weight override #26870

Conversation

janlegner commented Aug 1, 2022

Problem

Summary of Changes

Config example

pgarg66 commented Aug 1, 2022 • edited Loading

janlegner commented Aug 2, 2022 • edited Loading

mvines commented Aug 2, 2022

janlegner commented Aug 3, 2022

mvines commented Aug 3, 2022

ochaloup commented Aug 9, 2022

mvines left a comment

Choose a reason for hiding this comment

mvines Aug 10, 2022

Choose a reason for hiding this comment

ochaloup Aug 11, 2022

Choose a reason for hiding this comment

mvines Aug 10, 2022

Choose a reason for hiding this comment

ochaloup Aug 11, 2022

Choose a reason for hiding this comment

mvines Aug 10, 2022

Choose a reason for hiding this comment

ochaloup Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

ochaloup commented Aug 11, 2022

ochaloup commented Aug 11, 2022 • edited Loading

pgarg66 commented Aug 11, 2022

ochaloup commented Aug 11, 2022 • edited Loading

pgarg66 commented Aug 11, 2022

mvines left a comment

Choose a reason for hiding this comment

pgarg66 commented Aug 1, 2022 •

edited

Loading

janlegner commented Aug 2, 2022 •

edited

Loading

ochaloup Aug 11, 2022 •

edited

Loading

ochaloup commented Aug 11, 2022 •

edited

Loading

ochaloup commented Aug 11, 2022 •

edited

Loading