-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
adds socket address for repair service over QUIC #32834
Conversation
51e296a
to
8eb88d0
Compare
Codecov Report
@@ Coverage Diff @@
## master #32834 +/- ##
=======================================
Coverage 82.0% 82.0%
=======================================
Files 785 785
Lines 211927 211948 +21
=======================================
+ Hits 173793 173847 +54
+ Misses 38134 38101 -33 |
Working towards migrating repair to QUIC.
8eb88d0
to
9a0a75b
Compare
@@ -271,7 +271,7 @@ pub fn make_accounts_hashes_message( | |||
pub(crate) type Ping = ping_pong::Ping<[u8; GOSSIP_PING_TOKEN_SIZE]>; | |||
|
|||
// TODO These messages should go through the gpu pipeline for spam filtering | |||
#[frozen_abi(digest = "4jtxvWyeFwfDQTTGh4yJLyukALzRNVJ9WNnCbFeJUmaS")] | |||
#[frozen_abi(digest = "6T2sn92PMrTijsgncH3bBZL4K5GUowb442cCw4y4DuwV")] |
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.
I wonder what cause the ABI change?
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.
The digest changes because the struct field name in LegacyContactInfo
has changed. But that is totally fine here because this structs are only (de)serialized using bincode
which is totally oblivious to struct field names.
@@ -214,7 +214,7 @@ pub(crate) type Ping = ping_pong::Ping<[u8; REPAIR_PING_TOKEN_SIZE]>; | |||
|
|||
/// Window protocol messages | |||
#[derive(Debug, AbiEnumVisitor, AbiExample, Deserialize, Serialize, strum_macros::Display)] | |||
#[frozen_abi(digest = "7vZyACjc13qQYWUsqWbdidLXR3uNXpmqUZaKeV3gKuY2")] | |||
#[frozen_abi(digest = "3VzVe3kMrG6ijkVPyCGeJVA9hQjWcFEZbAQPc5Zizrjm")] |
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.
Is this due to LegacyContactInfo change?
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.
Any concern on this "In general, once we add frozen_abi and its change is published in the stable release channel, its digest should never change. If such a change is needed, we should opt for defining a new struct like FooV1. And special release flow like hard forks should be approached." mentioned in https://docs.solana.com/implemented-proposals/abi-management?
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.
There is no backward compatibility issue here as described in #32834 (comment)
@@ -123,7 +124,7 @@ impl Default for LegacyContactInfo { | |||
gossip: socketaddr_any!(), | |||
tvu: socketaddr_any!(), | |||
tvu_quic: socketaddr_any!(), | |||
unused: socketaddr_any!(), | |||
serve_repair_quic: socketaddr_any!(), |
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.
ahh i see. ok replacing the old repair socket with a quic repair socket
Mode::ServeRepair => { | ||
Some((*node.pubkey(), node.serve_repair(Protocol::UDP).unwrap())) | ||
} |
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.
i assume the plan is to next implement repair over quic?
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.
yes, that is the plan
const SOCKET_TAG_RPC: u8 = 2; | ||
const SOCKET_TAG_RPC_PUBSUB: u8 = 3; | ||
const SOCKET_TAG_SERVE_REPAIR: u8 = 4; | ||
const SOCKET_TAG_SERVE_REPAIR_QUIC: u8 = 1; | ||
const SOCKET_TAG_TPU: u8 = 5; | ||
const SOCKET_TAG_TPU_FORWARDS: u8 = 6; | ||
const SOCKET_TAG_TPU_FORWARDS_QUIC: u8 = 7; |
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.
is there a concern this can get bloated and sparse over time as we add an remove hardcoded sockets? may not be a big issue but readability could suffer. just a thought
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.
if it becomes too sparse, we can use a hash-map instead of an array in the future.
Problem
Working towards migrating repair to QUIC.
Summary of Changes
added socket address for repair service over QUIC.