Add remote_pubkey to Packet Meta #436
Conversation
lijunwangs
requested review from
alexpyattaev,
bw-solana,
gregcusack,
joncinque,
sakridge,
tao-stones and
wen-coding
|
Overall LGTM. |
You meant instead of Option, just store Pubkey? |
Yes, and set it to all zeros when no Pubkey is provided. Have a getter and setter that operate on Option of course. Saves 8 bytes per packet - not sure it is worth the effort though. |
imo, worth it. |
Done |
alexpyattaev
left a comment
alexpyattaev
left a comment
There was a problem hiding this comment.
LGTM (please wait for SDK SME approval)
|
|
||
| #[inline] | ||
| pub fn remote_pubkey(&self) -> Option<Pubkey> { | ||
| if self.remote_pubkey == Pubkey::default() { |
There was a problem hiding this comment.
In such cases it may be preferable to explicitly compare with array of zeros rather than relying on the Default impl staying stable. Unlikely to be a problem for us of course.
|
@joncinque can you help review from sdk side? |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Just a couple of small things on my side, looks great otherwise!
| pub addr: IpAddr, | ||
| pub port: u16, | ||
| pub flags: PacketFlags, | ||
| remote_pubkey: Pubkey, |
There was a problem hiding this comment.
Flagging that this is technically a breaking change for anyone who builds a Meta without using Default in some way, ie:
let m = Meta { size, addr, port, flags };
It's not a problem, but probably requires a breaking release
There was a problem hiding this comment.
I think they can still use struct Meta, by putting the new flag with default
Met {
size,
add,
port,
flags,
..Meta::default()
}
There was a problem hiding this comment.
Correct, but that does still constitute a breaking change since old code will no longer compile.
There was a problem hiding this comment.
True, whenever we introduce a field it will break as we lack a constructor.
This is related to anza-xyz/agave#8836 for anza-xyz/agave#8420
Alpenglow requires sending the pubkey of the client for QUIC connection to the consumer to support censoring a client using the key if the consumer determines the client is misbehaving.
We considered alternatives of adding the information to PacketBatch, BytesPacket, and we think having it in Meta is cleanest.