fix: reject BatchProofQueue ingress collisions and fix commit accounting - #406
fix: reject BatchProofQueue ingress collisions and fix commit accounting#4060xIcarus wants to merge 1 commit into
Conversation
|
could you next time include the upstream in the your PR description? if there is none, mention None. It would make the review much easier |
| // time, even if a colliding BatchInfo reaches this path. | ||
| let stored_num_txns = item.info.num_txns(); | ||
| let stored_author = item.info.author(); | ||
| self.dec_remaining_proofs(&stored_author, stored_num_txns); |
There was a problem hiding this comment.
Blocking: This fixes the counter decrement, but the colliding commit still leaves the queued item's BatchInfo and expiry indexes authoritative. I reproduced A(expiry 10) → colliding B(expiry 20) committed → advance to 10 → retransmit B; A's stale expiry removes the committed tombstone and B is accepted (remaining_proofs=1). Could we replace or synchronize item.info, author_to_batches, and expirations with the committed batch (or otherwise ignore stale expirations), and add this lifecycle regression? The current helper forces u64::MAX, so the existing tests cannot catch it.
Description
BatchProofQueuekeys slots on(author, batch_id)without the content digest. Two batches sharing the same key but different metadata can corrupt a slot thus grafting a proof onto a mismatched summary or vice versa and cause au64underflow inmark_committedwhennum_txnsdiverges.How Has This Been Tested?
4 regression tests in
batch_proof_queue_test.rscovering: summary-first collision, proof-first collision, colliding summary on an existing proof, and themark_committedunderflow case. All 11 tests in the module pass.Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.