gossip: ignore retransmitter signatures when comparing duplicate shreds#2673
Conversation
AshwinSekar
force-pushed
the
duplicate-proof-ignore-retransmitter-sig
branch
2 times, most recently
from
b0eaa56 to
d132a18
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_retransmitter_signature_invalid() { |
There was a problem hiding this comment.
Verified that this test fails with the previous payload comparison logic
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
AshwinSekar
force-pushed
the
duplicate-proof-ignore-retransmitter-sig
branch
from
d132a18 to
43de34f
Compare
| } | ||
|
|
||
| fn retransmitter_signature_offset(&self) -> Result<usize, Error> { | ||
| pub(crate) fn retransmitter_signature_offset(&self) -> Result<usize, Error> { |
| /// Returns true if the other shred has the same ShredId, i.e. (slot, index, | ||
| /// shred-type), but different payload. | ||
| /// Retransmitter's signature is ignored when comparing payloads. | ||
| pub fn is_shred_duplicate(&self, other: &Shred) -> bool { |
There was a problem hiding this comment.
Probably need some unit tests for this function too.
But it can be done later if we want to release and fix testnet first.
There was a problem hiding this comment.
Got it, i'll add a unit test in a follow up PR #2697.
Will aim to get this backported for tomorrow's testnet release.
| } | ||
| } | ||
|
|
||
| #[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] |
There was a problem hiding this comment.
The point of avoiding pub is that we can change this function api or behavior without the external code breaking.
If some tests in another crate is going to break then it is already a lost cause.
So I don't see much value in using dev-context here; you can just make the function pub.
AshwinSekar
added
the
automerge
…e shreds (backport of #2673) (#2699) * gossip: ignore retransmitter signatures when comparing duplicate shreds (#2673) * gossip: ignore retransmitter signatures when comparing duplicate shreds * pr feedback: compare rest of payload instead of setting sig * pr feedback: remove dcou, pub(super) (cherry picked from commit ff87ed9) # Conflicts: # ledger/src/shred.rs * fix conflicts --------- Co-authored-by: Ashwin Sekar <ashwin@anza.xyz> Co-authored-by: Ashwin Sekar <ashwin@solana.com>
…ds (anza-xyz#2673) * gossip: ignore retransmitter signatures when comparing duplicate shreds * pr feedback: compare rest of payload instead of setting sig * pr feedback: remove dcou, pub(super)
…duplicate shreds (backport of anza-xyz#2673) (anza-xyz#2699)"
…mparing duplicate shreds (backport of anza-xyz#2673) (anza-xyz#2699)"" This reverts commit 260f39c.
2 participants
Problem
#1271 ignores the retransmitter signature when comparing shreds in order to generate a duplicate proof.
We need to do the same when verifying a duplicate proof received from gossip.
Summary of Changes
Ignore retransmitter signature when verifying a duplicate proof received through gossip