fix: ignore epoch finality from state sync#5750
Conversation
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved. I verified the state-sync change at head ffb9c50435aa81062a8351610ff355f1678748dc.
Validation run:
git diff --check origin/main...HEAD -- node/rustchain_p2p_gossip.py node/tests/test_p2p_state_epoch_sync.py
python3 -B -m py_compile node/rustchain_p2p_gossip.py node/tests/test_p2p_state_epoch_sync.py
RC_P2P_SECRET=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa PYTHONPATH=node python3 -B -m pytest -q node/tests/test_p2p_state_epoch_sync.py node/tests/test_p2p_hardening_phase2.py --tb=short
# 11 passed in 0.22sI also ran a direct origin/main-vs-PR probe with a valid signed STATE message from peer1 carrying epochs: [999] plus fake finality metadata. On origin/main, the message verified and _handle_state() merged the epoch (contains_999=true, metadata_keys=["999"]). On this PR, the same verified message returned status=ok but logged ignoring epoch finality data in STATE sync and left contains_999=false, metadata_keys=[].
That matches the intended boundary: a peer signature authenticates the sender of a state snapshot, but it is not quorum evidence for epoch finality. Keeping finality on the EPOCH_COMMIT path while preserving the existing attestation and balance state-sync handling is the right fix for this issue.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
|
Good catch. Addressed in the latest commit. The fix now includes an explicit The commit handler validates quorum-shaped metadata before marking the epoch finalized:
Validation: |
kevinyan911
left a comment
There was a problem hiding this comment.
Code Review — PR #5750
Reviewer: @kevinyan911
Wallet: RTCcd1dd903b3cbbfca24c30bd98973931a4af53302
What this PR does
Adds EPOCH_COMMIT message handler with quorum validation, and explicitly stops importing epoch finality from generic STATE sync. Previously _handle_state() would merge epochs from any verified peer signature — but a peer's signature only authenticates who sent the snapshot, not that quorum actually committed those epochs. This PR draws the correct security boundary.
Code quality
_handle_epoch_commit()validatesaccept_countand voter set against quorum before adding toepoch_crdt— correct._handle_state()now only logs a warning when epochs appear in STATE sync rather than merging them — the intent comment makes the reasoning explicit.- Regression test creates a valid signed STATE with forged epoch 999 and asserts
epoch_crdt.contains(999) == False— directly proves the attack vector is closed. py_compileclean,git diff --checkclean, 11 pytests pass.
APPROVED — scoped security fix with clear test coverage.
Code review bounty claim submitted to rustchain-bounties
akaalholdings
left a comment
There was a problem hiding this comment.
Requesting changes because the new EPOCH_COMMIT path still lets one peer inject epoch finality by claiming quorum-shaped metadata without proving the listed voters actually signed or sent votes.
The PR correctly stops importing STATE[\"epochs\"], but _handle_epoch_commit() currently trusts these sender-provided fields:
accept_count = payload.get("accept_count", 0)
voters = payload.get("voters", [])
...
if accept_count < quorum or len(voter_set) < quorum:
return {"status": "error", "reason": "insufficient_quorum"}
self.epoch_crdt.add(epoch, ...)A valid message signature only authenticates the committer peer. It does not authenticate peer2 or peer3 as voters. I verified this against PR head with a one-off probe: peer1 alone creates an EPOCH_COMMIT for epoch 31337, lists peer1, peer2, and peer3 as voters, and the victim accepts it.
signature_valid= True
result= {'status': 'committed', 'epoch': 31337, 'accept_count': 3}
contains_31337= True
metadata= {'proposal_hash': 'forged-by-one-peer', 'finalized': True, 'accept_count': 3, 'voters': ['peer1', 'peer2', 'peer3'], 'committer': 'peer1'}
The included regression suite also currently codifies that behavior:
RC_P2P_SECRET=$(printf 'a%.0s' {1..64}) PYTHONPATH=node /tmp/rustchain-review-venv/bin/python -B -m pytest -q node/tests/test_p2p_state_epoch_sync.py --tb=short -p no:cacheprovider
# 3 passedThis reintroduces the same security boundary problem through a different message type: peer signature plus self-reported quorum metadata is still not quorum evidence. Please require verifiable voter evidence before accepting an external commit, for example signed vote records from the listed voters, or only accept EPOCH_COMMIT when it matches locally stored validated votes/quorum state.
|
Addressed in commit fed6fd6. I tightened Regression coverage now includes both sides of the boundary:
Local validation: python3 -B -m py_compile node/rustchain_p2p_gossip.py node/tests/test_p2p_state_epoch_sync.py
RC_P2P_SECRET=<64-byte test secret> PYTHONPATH=node python3 -B -m pytest -q node/tests/test_p2p_state_epoch_sync.py node/tests/test_p2p_hardening_phase2.py --tb=short -p no:cacheprovider
# 14 passed
git diff --check |
|
Merged. This completes the file-by-file review of your full 8-PR batch (#5732 + #5740–#5752). Review outcome — all 8 APPROVED
Every PR: clean 2–3 file scope, real exploit-demonstrating or concurrency tests, VM-zero rule preserved, no smuggled hunks. This is exceptionally strong security work — and filing the paired bug issues (#5743–#5751) before the fixes is exactly the right practice. On #5750 specifically: merged as-is. The offline-node catch-up tradeoff is tracked in follow-up issue #5950 — closing the forgery hole now is the right call; the validated catch-up path gets designed separately. To get paid — post your walletI have no RTC wallet on file for you. Reply here (or on any of your PRs) with your wallet — format Also: you're clearly a serious security contributor. If you want, the standing bounty queue is at https://github.com/Scottcjn/rustchain-bounties/issues — and consensus/settlement hardening like this is the highest-value work there. — Sophia |
BossChaos
left a comment
There was a problem hiding this comment.
PR #5750 Review - Epoch commit handler with strict type validation
Security Analysis - Consensus Security
This PR adds a new EPOCH_COMMIT message handler with comprehensive type validation.
Key improvements:
- _handle_epoch_commit(): New handler for finalized epoch commits
- Strict type checking: Validates epoch (int), proposal_hash (str), accept_count (int >= 0), voters (list)
- Known voters validation: Verifies all listed voters are known nodes
- Commit voter filtering: Only includes accept voters in the commit message broadcast, not reject voters
- Type safety: Returns specific error reasons for each validation failure
Note: Also fixes _handle_epoch_vote() to filter voters to only those who voted "accept" when broadcasting the commit message.
Recommendation: Merge - important consensus security hardening.
|
Payout address: RTC71d6976297ed35377b867f13ed962f54020ef434 |
Summary
Fixes #5749.
_handle_state()verified the signedSTATEmessage, then mergedstate["epochs"]directly intoepoch_crdt. A peer signature authenticates the snapshot sender, but it is not quorum/commit evidence that those epochs finalized.This PR stops importing epoch finality from generic state sync. Epoch finality should enter
epoch_crdtvia theEPOCH_COMMITpath after local validation, not via additive CRDT snapshot merge.The regression test creates a valid signed
STATEmessage from a peer containing forged epoch999, verifies the signature is accepted, and asserts the victim does not add epoch999toepoch_crdt.Validation
Note: I also ran adjacent P2P tests;
node/tests/test_p2p_entropy_score_downgrade.pypasses, whilenode/tests/test_p2p_vote_spoofing.py::test_vote_spoofing_finds_quorumappears to be an older reproduction test that now expects a previously fixed vulnerability to still reproduce, so I did not include it as blocking validation for this scoped fix.