adds new contact-info with forward compatible sockets #29596

behzadnouri · 2023-01-09T21:01:24Z

Problem

Need to add/remove sockets from ContactInfo in a backward and forward compatible way.
Current sockets specification in LegacyContactInfo is inefficient and unnecessarily duplicates IP addresses.

Summary of Changes

Implemented a new ContactInfo where

Ports and IP addresses are specified separately so that unique IP addresses can only be specified once.
Different sockets (tvu, tpu, etc) are specified by opaque u8 tags so that adding and removing sockets is backward and forward compatible.
solana_version::Version is also embedded in so that it won't need to be gossiped separately.
In order to identify duplicate running instances, the struct also includes when the instance was first created and so the separate NodeInstance crds-value will become redundant.

Update plan:

Once the cluster is able to ingest the new type (i.e. this patch), a 2nd patch will start gossiping the new ContactInfo along with the LegacyContactInfo.
Once all nodes in the cluster gossip the new ContactInfo, a 3rd patch will start solely using the new ContactInfo while still gossiping the old LegacyContactInfo.
Once all nodes in the cluster solely use the new ContactInfo, a 4th patch will stop gossiping the old LegacyContactInfo.

ryoqun · 2023-01-11T07:18:15Z

@behzadnouri i pushed commits to fix the frozen abi issue with Cow: 28a0c38 and 552d13d

ref: rust-lang/rust#106710

behzadnouri · 2023-01-11T13:32:45Z

@behzadnouri i pushed commits to fix the frozen abi issue with Cow: 28a0c38 and 552d13d

ref: rust-lang/rust#106710

@ryoqun Thank you

gossip/src/contact_info.rs

t-nelson · 2023-01-11T22:17:02Z

gossip/src/contact_info.rs

+const SOCKET_TAG_GOSSIP: u8 = 0;
+const SOCKET_TAG_REPAIR: u8 = 1;
+const SOCKET_TAG_RPC: u8 = 2;
+const SOCKET_TAG_RPC_PUBSUB: u8 = 3;
+const SOCKET_TAG_SERVE_REPAIR: u8 = 4;
+const SOCKET_TAG_TPU: u8 = 5;
+const SOCKET_TAG_TPU_QUIC: u8 = 6;
+const SOCKET_TAG_TPU_FORWARDS: u8 = 7;
+const SOCKET_TAG_TPU_FORWARDS_QUIC: u8 = 8;
+const SOCKET_TAG_TPU_VOTE: u8 = 9;
+const SOCKET_TAG_TVU: u8 = 10;
+const SOCKET_TAG_TVU_FORWARDS: u8 = 11;


consider an enum or type-alias/newtype u8 here

The opaque u8 here is intentional so that adding new sockets is backward compatible.

An enum will fail deserialization if there is an unknown tag/enum-variant; as a result you cannot add a new socket in a backward compatible way because nodes not recognizing the enum-variant would fail deserializing the whole contact-info.

You can work around that by adding an Unknown(u8) variant to the enum but that would also require manual serde implementation which would be counterproductive.

gossip/src/contact_info.rs

t-nelson · 2023-01-11T23:25:48Z

gossip/src/contact_info.rs

+        // TODO: sanity check on the fields.
+        // TODO: verify port offsets don't overflow.


these seem important?

t-nelson · 2023-01-11T23:31:23Z

gossip/src/contact_info.rs

+    fn new_rand_addr<R: Rng>(rng: &mut R) -> IpAddr {
+        if rng.gen() {
+            let addr = Ipv4Addr::new(rng.gen(), rng.gen(), rng.gen(), rng.gen());
+            IpAddr::V4(addr)
+        } else {
+            let addr = Ipv6Addr::new(
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+            );
+            IpAddr::V6(addr)
+        }
+    }
+
+    fn new_rand_port<R: Rng>(rng: &mut R) -> u16 {
+        let port = rng.gen::<u16>();
+        let bits = u16::BITS - port.leading_zeros();
+        let shift = rng.gen_range(0u32, bits + 1u32);
+        port.checked_shr(shift).unwrap_or_default()
+    }


these will occasionally draw values that we'd reject on sanitize, right? probably less flaky to generate the deterministically ala Pubkey::new_unique

I don't understand what change you are suggesting.
This occasionally creates invalid socket addresses which is intentional to have test coverage over paths which fail sanitize_socket.

If we want to make the test deterministic we can use a SeedableRng and a seeded random number generator.
Pubkey::new_unique approach isn't good because:
https://discord.com/channels/428295358100013066/439194979856809985/881943239823880222

I don't understand what change you are suggesting. This occasionally creates invalid socket addresses which is intentional to have test coverage over paths which fail sanitize_socket.

we shouldn't mix testing and fuzzing like that. tests should be deterministic to avoid flake. if we need to start fuzzing stuff, that's another conversation

If we want to make the test deterministic we can use a SeedableRng and a seeded random number generator. Pubkey::new_unique approach isn't good because: https://discord.com/channels/428295358100013066/439194979856809985/881943239823880222

seedable rng would be fine (would be fine to reimplement Pubkey::new_unique over one as well)

Even if all corner cases are identified, it is impossible to cover all their various combinations using deterministic tests. Using unseeded random number generator here, we are getting a much larger test coverage.

Flaky tests by themselves are not a problem, as long as they are easily reproducible locally; in which case they do identify a legit bug or corner case.

Given that this is a single threaded test and there is no other dependence on runtime environment here, do you see any reason that if this test becomes flaky it won't be locally reproducible, by say running the test 1000 times in a loop?

how about switch to deterministic rng, seed with time and log the seed? so we can immediately reproduce a failure with trivial modification

you didn't answer my question

gossip/src/contact_info.rs

t-nelson · 2023-01-12T00:02:19Z

gossip/src/contact_info.rs

+        let &(_, index, port) = self.sockets.iter().find(|(k, _, _)| *k == tag)?;
+        let addr = self.addrs.get(usize::from(index))?;
+        let socket = SocketAddr::new(*addr, port);
+        let _ = sanitize_socket(&socket).ok()?;


seems like this should've been done on construction/push?

set_socket does sanitize_socket.
But I am thinking if we are deserializing another node's contact-info, we probably don't want to throw the entire contact-info out if one of the sockets doesn't sanitize. It would be just enough to filter out that specific socket when needed.

gossip/src/contact_info.rs

ryleung-solana · 2023-01-12T13:13:01Z

gossip/src/contact_info.rs

+    thiserror::Error,
+};
+
+const SOCKET_TAG_GOSSIP: u8 = 0;


Would it be possible to add a gossip quic entry as well? We want to add Quic Gossip support. CC @lijunwangs on the related Quic repair changes for if we need a port allocated for that as well.

Instead of adding unused ports now, we can add new ports once the code using those ports is almost ready (could possibly be in the same patch). Even if we add that now it cannot be used yet until LegacyContactInfo => ContactInfo migration is done.

lijunwangs · 2023-01-12T18:19:37Z

gossip/src/contact_info.rs

+    #[serde(with = "short_vec")]
+    addrs: Cow<'a, [IpAddr]>,
+    #[serde(with = "short_vec")]
+    sockets: Cow<'a, [(u8, u8, u16)]>,


Maybe we can define a struct to clarify what these three members means. Would this make the sockets dynamic to add additional sockets? Like quic tpu and etc?

#29596 (comment)

lijunwangs · 2023-01-12T18:25:39Z

gossip/src/contact_info.rs

+    }
+
+    fn get_socket(&self, tag: u8) -> Option<SocketAddr> {
+        let &(_, index, port) = self.sockets.iter().find(|(k, _, _)| *k == tag)?;


Maybe put it into a map for quicker lookup?

the list will be short. probably faster to linear probe than do the hashing for a map

The actual socket lookup is done by self.cache which is an array.

pub struct ContactInfo { // ... #[serde(skip_serializing)] cache: [SocketAddr; SOCKET_CACHE_SIZE], }

gossip/src/contact_info.rs

behzadnouri · 2023-01-12T19:02:42Z

@lijunwangs @t-nelson
This is not ready for code review; It is marked as "Draft" and you see things like #![allow(dead_code)], TODO etc.

The intention of discord message was just to discuss the new ContactInfo struct design-wise:
https://discord.com/channels/428295358100013066/439194979856809985/1062804586118795315

gossip/src/contact_info.rs

mvines · 2023-01-12T23:46:47Z

Should we roll NodeInstance into this as well? Really that's separate data item only because we couldn't extend ContactInfo:

solana/gossip/src/crds_value.rs

Lines 414 to 419 in 164c929

    
           pub struct NodeInstance { 
        
               from: Pubkey, 
        
               wallclock: u64, 
        
               timestamp: u64, // Timestamp when the instance was created. 
        
               token: u64,     // Randomly generated value at node instantiation. 
        
           }

behzadnouri · 2023-01-13T18:37:55Z

Should we roll NodeInstance into this as well? Really that's separate data item only because we couldn't extend ContactInfo:

solana/gossip/src/crds_value.rs

Lines 414 to 419 in 164c929

pub struct NodeInstance {

from: Pubkey,

wallclock: u64,

timestamp: u64, // Timestamp when the instance was created.

token: u64, // Randomly generated value at node instantiation.

}

yup, doing that as well.

gossip/src/crds_gossip_pull.rs

t-nelson · 2023-01-19T01:29:41Z

gossip/src/crds_gossip_push.rs

@@ -189,6 +189,10 @@ impl CrdsGossipPush {
        let crds = crds.read().unwrap();
        let entries = crds
            .get_entries(crds_cursor.deref_mut())
+            .filter(|entry| {
+                // Exclude the new ContactInfo until the cluster has upgraded.
+                !matches!(&entry.value.data, CrdsData::ContactInfo(_))


#29596 (comment)

gossip/src/contact_info.rs

t-nelson · 2023-01-19T04:13:22Z

gossip/src/contact_info.rs

+            }
+        }
+    }
+    // Verify that all sockets have unique key.


can you elaborate this section in the comment? i understand what's going on here, but not how it relates to the rest of this changeset

The key should be unique otherwise with multiple entries for the same key, the socket specification would be ambiguous as which one it is. This is already commented in the struct definition:

pub struct ContactInfo { // ... // All sockets have a unique key and a valid IP address index. #[serde(with = "short_vec")] sockets: Vec<SocketEntry>, // ... }

here I am verifying "that all sockets have unique key".

What needs to be clarified?

t-nelson · 2023-01-19T04:48:42Z

gossip/src/contact_info.rs

+    fn new_rand_addr<R: Rng>(rng: &mut R) -> IpAddr {
+        if rng.gen() {
+            let addr = Ipv4Addr::new(rng.gen(), rng.gen(), rng.gen(), rng.gen());
+            IpAddr::V4(addr)
+        } else {
+            let addr = Ipv6Addr::new(
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+                rng.gen(),
+            );
+            IpAddr::V6(addr)
+        }
+    }
+
+    fn new_rand_port<R: Rng>(rng: &mut R) -> u16 {
+        let port = rng.gen::<u16>();
+        let bits = u16::BITS - port.leading_zeros();
+        let shift = rng.gen_range(0u32, bits + 1u32);
+        port.checked_shr(shift).unwrap_or_default()
+    }


I don't understand what change you are suggesting. This occasionally creates invalid socket addresses which is intentional to have test coverage over paths which fail sanitize_socket.

we shouldn't mix testing and fuzzing like that. tests should be deterministic to avoid flake. if we need to start fuzzing stuff, that's another conversation

If we want to make the test deterministic we can use a SeedableRng and a seeded random number generator. Pubkey::new_unique approach isn't good because: https://discord.com/channels/428295358100013066/439194979856809985/881943239823880222

seedable rng would be fine (would be fine to reimplement Pubkey::new_unique over one as well)

t-nelson · 2023-01-19T05:52:15Z

gossip/src/contact_info.rs

+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, AbiExample, Deserialize, Serialize)]
+struct SocketEntry {
+    key: u8,   // Protocol identifier, e.g. tvu, tpu, etc


can we normalize on key or tag?

It is key everywhere. There is no tag.
..._TAG_... is only used for constants (because those are consts not a key variable).

right, but we assign a *_TAG_* constant to a key field. which i find inconsistent

I took me a while to realize the "key" in this context is the one of the tags defined as well. I think it is clearer to standardize on one only

behzadnouri · 2023-01-24T14:15:33Z

@t-nelson were you going to look at this again or are you ok to merge it?

The commit implement new ContactInfo where * Ports and IP addresses are specified separately so that unique IP addresses can only be specified once. * Different sockets (tvu, tpu, etc) are specified by opaque u8 tags so that adding and removing sockets is backward and forward compatible. * solana_version::Version is also embedded in so that it won't need to be gossiped separately. * NodeInstance is also rolled in by adding a field identifying when the instance was first created so that it won't need to be gossiped separately. Update plan: * Once the cluster is able to ingest the new type (i.e. this patch), a 2nd patch will start gossiping the new ContactInfo along with the LegacyContactInfo. * Once all nodes in the cluster gossip the new ContactInfo, a 3rd patch will start solely using the new ContactInfo while still gossiping the old LegacyContactInfo. * Once all nodes in the cluster solely use the new ContactInfo, a 4th patch will stop gossiping the old LegacyContactInfo.

t-nelson · 2023-01-24T17:41:36Z

yes. i'll take another pass today

lijunwangs · 2023-01-25T03:33:45Z

gossip/src/contact_info.rs

+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, AbiExample, Deserialize, Serialize)]
+struct SocketEntry {
+    key: u8,   // Protocol identifier, e.g. tvu, tpu, etc


I took me a while to realize the "key" in this context is the one of the tags defined as well. I think it is clearer to standardize on one only

lijunwangs · 2023-01-25T23:20:34Z

gossip/src/contact_info.rs

+    key: u8,   // Protocol identifier, e.g. tvu, tpu, etc
+    index: u8, // IpAddr index in the accompanying addrs vector.
+    #[serde(with = "serde_varint")]
+    offset: u16, // Port offset with respect to the previous entry.


I think the offset is not totally accurate, for the first entry it is the actual port -- so better to reflect it in the comment at least. I wonder what is the benefit of storing the offset than simply the port number itself as most complexity in this code is related to the offset arithmetic.

Ports tend to be close to each other, so the offsets can be stored in one byte.
For gossip we are consuming a lot of bandwidth so anything which helps in the front can be useful.
Any added complexity is encapsulated inside this struct, and this is not exposed in the public api.

New ContactInfo implemented in solana-labs#29596 specifies sockets for QUIC protocols explicitly. The commit adds tpu_quic field to RpcContactInfo in preparation of LegacyContactInfo => ContactInfo migration.

Working towards migrating from legacy contact-info to the new contact-info: solana-labs#29596

Working towards migrating from legacy contact-info to the new contact-info: #29596

Working towards migrating from legacy contact-info to the new contact-info: #29596 (cherry picked from commit ba0a49b)

…4092) (#34095) propagates the new contact-info through gossip (#34092) Working towards migrating from legacy contact-info to the new contact-info: #29596 (cherry picked from commit ba0a49b) Co-authored-by: behzad nouri <[email protected]>

Working towards migrating from legacy contact-info to the new contact-info: solana-labs/solana#29596

behzadnouri force-pushed the contact-info-packed branch 3 times, most recently from b74c948 to 50269f6 Compare January 10, 2023 14:59

ryoqun mentioned this pull request Jan 11, 2023

Cow<'a, T> isn't specializable while FooCow<'a, T> is rust-lang/rust#106710

Open

behzadnouri marked this pull request as draft January 11, 2023 18:47

behzadnouri force-pushed the contact-info-packed branch from 552d13d to 30980d1 Compare January 11, 2023 18:47

t-nelson reviewed Jan 12, 2023

View reviewed changes

ryleung-solana reviewed Jan 12, 2023

View reviewed changes

behzadnouri force-pushed the contact-info-packed branch from c9a8be3 to e52dabb Compare January 12, 2023 16:45

lijunwangs reviewed Jan 12, 2023

View reviewed changes

t-nelson reviewed Jan 12, 2023

View reviewed changes

gossip/src/contact_info.rs Outdated Show resolved Hide resolved

behzadnouri force-pushed the contact-info-packed branch 2 times, most recently from d86d437 to 6b690c7 Compare January 18, 2023 15:05

behzadnouri marked this pull request as ready for review January 18, 2023 15:39

behzadnouri force-pushed the contact-info-packed branch 2 times, most recently from 754599e to 8102031 Compare January 18, 2023 16:23

behzadnouri requested review from t-nelson, mvines and sakridge January 18, 2023 17:29

behzadnouri force-pushed the contact-info-packed branch from 8102031 to dfb521c Compare January 18, 2023 17:46

t-nelson reviewed Jan 19, 2023

View reviewed changes

behzadnouri force-pushed the contact-info-packed branch from 7d62da2 to 7f7b8b6 Compare January 19, 2023 14:56

behzadnouri requested a review from t-nelson January 23, 2023 12:54

behzadnouri force-pushed the contact-info-packed branch from 7f7b8b6 to 0d35756 Compare January 24, 2023 16:59

behzadnouri mentioned this pull request Jan 25, 2023

adds new solana_version::Version with ClientId (backport #29649) #29910

Closed

lijunwangs reviewed Jan 25, 2023

View reviewed changes

behzadnouri merged commit d6fbf3f into solana-labs:master Jan 26, 2023

behzadnouri deleted the contact-info-packed branch January 26, 2023 17:02

behzadnouri mentioned this pull request Mar 16, 2023

Bootstrap cannot differentiate between "incremental snapshots don't exist" and "incremental snapshots haven't been seen yet" #30759

Closed

brooksprumo mentioned this pull request Apr 3, 2023

Uses AccountsHashes type for AccountsHashes CrdsData variant #31003

Merged

behzadnouri mentioned this pull request Apr 21, 2023

adds tpu_quic field to RpcContactInfo #31310

Merged

behzadnouri added a commit to behzadnouri/solana that referenced this pull request Nov 15, 2023

propagates the new contact-info through gossip

213f625

Working towards migrating from legacy contact-info to the new contact-info: solana-labs#29596

behzadnouri mentioned this pull request Nov 15, 2023

propagates the new contact-info through gossip #34092

Merged

behzadnouri added a commit that referenced this pull request Nov 15, 2023

propagates the new contact-info through gossip (#34092)

ba0a49b

Working towards migrating from legacy contact-info to the new contact-info: #29596

mergify bot pushed a commit that referenced this pull request Nov 15, 2023

propagates the new contact-info through gossip (#34092)

a0d28b2

Working towards migrating from legacy contact-info to the new contact-info: #29596 (cherry picked from commit ba0a49b)

crossdev24 pushed a commit to crossdev24/solana that referenced this pull request Jun 24, 2024

propagates the new contact-info through gossip (#34092)

38f2c77

Working towards migrating from legacy contact-info to the new contact-info: solana-labs/solana#29596

		// TODO: sanity check on the fields.
		// TODO: verify port offsets don't overflow.

adds new contact-info with forward compatible sockets #29596

adds new contact-info with forward compatible sockets #29596

Conversation

behzadnouri commented Jan 9, 2023 • edited Loading

Problem

Summary of Changes

ryoqun commented Jan 11, 2023 • edited Loading

behzadnouri commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri commented Jan 12, 2023

mvines commented Jan 12, 2023 • edited Loading

behzadnouri commented Jan 13, 2023

Choose a reason for hiding this comment

behzadnouri Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri commented Jan 24, 2023

t-nelson commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri commented Jan 9, 2023 •

edited

Loading

ryoqun commented Jan 11, 2023 •

edited

Loading

mvines commented Jan 12, 2023 •

edited

Loading

behzadnouri Jan 19, 2023 •

edited

Loading