uses SipHasher24 for gossip ping tokens

[behzadnouri]

Problem

More efficient ping tokens in gossip ping/pong protocol.

Summary of Changes

The commit uses SipHasher24 for gossip ping tokens where keys are refreshed every 1 minute.

[behzadnouri]

@ryoqun the frozen-abi tests are failing here. I tried manually doing

impl<const N:usize> solana_frozen_abi::abi_example::AbiExample for Ping<N> {
  // ...
}

as well but that didn't work either. Do you mind taking a look? Thank you,

[behzadnouri]

@ryoqun surprisingly this is also making the AbiExample implementation for solana_packet::Packet to fail as well even though I am not making any changes there. I am guessing the specializations somehow interfere with each other. 🤷‍♂️

Tried a few things, but none worked. I have for now removed failing frozen-abi tests to unblock this.
Happy to take any suggestion how to fix this. Thanks.

$ RUST_BACKTRACE=1 cargo test --features frozen-abi --lib -- TimedTracedEvent_frozen_abi --nocapture
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.36s
     Running unittests src/lib.rs (/home/acer/solana/target/debug/deps/solana_core-78548d94c0ee7b01)

running 1 test
thread 'banking_trace::TimedTracedEvent_frozen_abi::test_abi_digest' panicked at /home/acer/solana/sdk/frozen-abi/src/abi_example.rs:279:13:
derive or implement AbiExample/AbiEnumVisitor for solana_packet::Packet

[gregcusack]

generally looks good. have you tested this against v2 to make sure there are not backwards compatibility issues?

[gregcusack]

is there any reason why REPAIR_PING_TOKEN_SIZE may differ from GOSSIP_PING_TOKEN_SIZE? And would it make sense to just turn it into one variable called PING_TOKEN_SIZE and make it public in gossip/src/protocol.rs?

[behzadnouri]

It is a legacy thing. I have commented in the ping_pong.rs file that the new code should only use [u8; 8]. Old code was unnecessarily generic to allow different protocols specify different ping tokens.
I guess that could have been useful to distinguish types but with the new siphasher it no longer is applicable.

[gregcusack]

debug_assert!(N >= std::mem::size_of::<u64>());

is this the only safeguard if someone accidentally defines N < 8? is it worth making sure someone can't define N < 8 for non debug builds?

[behzadnouri]

Yes, I have commented this out where struct Ping is defined.

is it worth making sure someone can't define N < 8 for non debug builds?

how?

[gregcusack]

could we just change that line to:

const_assert!(N >= std::mem::size_of::<u64>());

This would fail at compile time and wouldn't add runtime overhead.

[behzadnouri]

I tried that before, but const_assert wouldn't work because N is const generic parameter, and apparently static_assertions cannot handle it.

[gregcusack]

ahh ok not ideal but ya not sure exactly how this could be enforced at compile time. and runtime check probably doesn't make sense. happy to stick with how you've documented the requirements for N

[behzadnouri]

have you tested this against v2 to make sure there are not backwards compatibility issues?

yes, I have a node running on mainnet with this code and it looks fine.

[gregcusack]

lgtm!

[ryoqun]

@behzadnouri oh, it's interesting... I'll take a look at the frozen abi failure later.

[behzadnouri]

@behzadnouri oh, it's interesting... I'll take a look at the frozen abi failure later.

Thanks @ryoqun ,
I had to remove the abi test to unblock this (see the last commit on this PR),
But happy to add them back if there is a way to make AbiExample to work. Thank you,