gossip: process duplicate proofs for merkle root conflicts #34066
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34066 +/- ##
==========================================
Coverage 81.8% 81.9%
==========================================
Files 766 816 +50
Lines 209077 220010 +10933
==========================================
+ Hits 171130 180262 +9132
- Misses 37947 39748 +1801 |
AshwinSekar
force-pushed
the
gossip-merkle-dup-proof
branch
from
730ceb5
to
691c6d0
Compare
|
Now that we have many different scenarios for duplicate proof (and more on the way with #33037), it might be worthwhile to add an enum here: solana/gossip/src/crds_value.rs Line 94 in 6a5b8e8 Don't think it's strictly necessary, but might be cleaner for debugging / logging |
gossip/src/duplicate_shred.rs
Outdated
| let mr1 = shred1.merkle_root().ok(); | ||
| let mr2 = shred2.merkle_root().ok(); | ||
|
|
||
| if (mr1.is_some() && mr2.is_none()) || (mr2.is_some() && mr1.is_none()) { | ||
| // A mixture of legacy and merkle shreds in the same fec set | ||
| // constitutes a valid duplicate proof | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some((mr1, mr2)) = mr1.zip(mr2) { | ||
| if mr1 != mr2 { | ||
| // Conflicting merkle roots for the same fec set is a | ||
| // valid duplicate proof | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this just below?
if shred1.merkle_root().ok() != shred2.merkle_root().ok() {
return Ok(());
}
gossip/src/duplicate_shred.rs
Outdated
|
|
||
| if shred1.shred_type() != shred2.shred_type() { | ||
| // Only valid proof here is a merkle conflict which was checked above | ||
| return Err(Error::InvalidMerkleRootConflict); |
There was a problem hiding this comment.
The return error value is going to be very confusing if not wrong when other types of duplicates mentioned in #33979 (comment) are included here, because those also can be different shred-type but are not merkle-root conflict.
Can we keep the error ShredTypeMismatch? because here we are checking for shreds type to be matching and for the rest of this function we expect the shreds types to be the same.
Gossip is oblivious to what constructs a duplicate block proof anyways; we also need to send multiple chunks for each duplicate proof which already makes this code a bit hacky. |
AshwinSekar
force-pushed
the
gossip-merkle-dup-proof
branch
from
691c6d0
to
85f29cd
Compare
|
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. |
* gossip: process duplicate proofs for merkle root conflicts * pr comments + abi (cherry picked from commit ca6ab08) # Conflicts: # gossip/src/cluster_info.rs
* gossip: process duplicate proofs for merkle root conflicts * pr comments + abi (cherry picked from commit ca6ab08) # Conflicts: # gossip/src/cluster_info.rs
* gossip: process duplicate proofs for merkle root conflicts * pr comments + abi (cherry picked from commit ca6ab08) # Conflicts: # gossip/src/cluster_info.rs
* gossip: process duplicate proofs for merkle root conflicts * pr comments + abi (cherry picked from commit ca6ab08) # Conflicts: # gossip/src/cluster_info.rs
* gossip: process duplicate proofs for merkle root conflicts * pr comments + abi (cherry picked from commit ca6ab08) # Conflicts: # gossip/src/cluster_info.rs
Problem
With the addition of merkle shreds, there is a new class of duplicate proofs available to be gossiped.
Summary of Changes
Add a check for merkle root conflict proofs:
Additionally this means we can now have a duplicate proof with 1 data shred and 1 coding shred,
so the
shred_typefield on duplicate proofs is unnecessary.Split from #33889
Contributes to #33644