fix: harden PushQC against stale QCs and unverified headers#2910
fix: harden PushQC against stale QCs and unverified headers#2910wen-coding wants to merge 4 commits intomainfrom
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2910 +/- ##
===========================================
- Coverage 57.35% 48.37% -8.98%
===========================================
Files 2095 671 -1424
Lines 172870 50611 -122259
===========================================
- Hits 99142 24484 -74658
+ Misses 64842 23980 -40862
+ Partials 8886 2147 -6739
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| func (s *State) PushQC(ctx context.Context, qc *types.FullCommitQC, blocks []*types.Block) error { | ||
| // Wait until QC is needed. | ||
| gr := qc.QC().GlobalRange() | ||
| needQC, err := func() (bool, error) { |
There was a problem hiding this comment.
if that's not too much trouble, I still think that it would be useful to skip verifying the QC if it is not needed. Then the fix would look as follows: if we already have the qc, then just drop the unverified qc and use the one we already have instead.
There was a problem hiding this comment.
I can do that, but why prefer skipping verification? Is QC verification very expensive?
wen-coding
force-pushed
the
wen/fix_commitqc_verify
branch
from
64f5001 to
03bcfb9
Compare
wen-coding
changed the title
wen-coding
force-pushed
the
wen/fix_commitqc_verify
branch
from
03bcfb9 to
b0b7d32
Compare
…ing state The insert section used `inner.nextQC < gr.Next` which allowed a stale overlapping QC (where gr.First < inner.nextQC < gr.Next) to advance nextQC even when verification was skipped. Replace with the existing needQC guard so that only the next expected QC is inserted. Also move ctrl.Updated() after the mutation rather than before. Co-authored-by: Cursor <cursoragent@cursor.com>
wen-coding
changed the title
wen-coding
force-pushed
the
wen/fix_commitqc_verify
branch
from
b0b7d32 to
bfe9965
Compare
|
|
||
| // Stale QC is silently ignored (verification is skipped, insert guard prevents | ||
| // advancing nextQC). No error is returned. | ||
| require.NoError(t, state.PushQC(ctx, maliciousQC, nil)) |
There was a problem hiding this comment.
the fact that the maliciousQC is ignored here is an implementation detail. Both error and no error are acceptable results. What you should test against is that the state did not change: call QC() on all previous heights - it should return the correct QC (and perhaps check the nextQC, since the api does not expose it currently in any way).
There was a problem hiding this comment.
good point, changed.
|
|
||
| // Verify state is not corrupted: the next valid QC should still be accepted. | ||
| qc2, blocks2 := TestCommitQC(rng, committee, keys, utils.Some(qc1.QC())) | ||
| require.NoError(t, state.PushQC(ctx, qc2, blocks2)) |
There was a problem hiding this comment.
in particular this call returning no error does not guarantee that the qc2 was not ignored.
pompon0
left a comment
pompon0
left a comment
There was a problem hiding this comment.
lgtm, added some comments wrt the test.
- Don't assert on malicious PushQC return value (implementation detail) - Verify all previously pushed QCs are intact via state.QC() - Check nextQC cursor did not advance - Verify qc2 is actually visible after push - Use correct leader key and OrPanic1 for merge compatibility Co-authored-by: Cursor <cursoragent@cursor.com>
When needQC is false (duplicate/stale QC), block header matching was using the unverified incoming QC's headers. Now uses per-block stored QC lookup with min(gr.Next, inner.nextQC) cap to prevent OOB access when the incoming range extends beyond stored QCs. Co-authored-by: Cursor <cursoragent@cursor.com>
wen-coding
changed the title
3 participants
Summary
ctrl.Updated()withneedQCto prevent stale QCs from advancingnextQCor triggering spurious updates.inner.nextQCto prevent nil-pointer dereference when a malicious QC's range extends beyond stored QCs.Test plan
TestPushQCStaleQCDoesNotCorruptState: pushes a stale malicious QC with blocks extending beyond stored range. Verifies no panic (min cap), no state corruption (QCs, blocks, nextQC intact), and subsequent valid QC is accepted.TestPushQCIgnoresBlocksMatchingUnverifiedHeaders: pushes a tampered QC (same range, different headers) with blocks matching the tampered headers. Verifies fake blocks are rejected and real blocks are accepted afterward.