perf(blocksync): verify a commit once, not again as the next block's LastCommit - #1427
perf(blocksync): verify a commit once, not again as the next block's LastCommit#1427PastaPastaPasta wants to merge 1 commit into
Conversation
…LastCommit Block sync threshold-verifies the commit for block N in blockApplier.verify, and one height later the same commit comes back as block N+1's LastCommit and is verified a second time inside validateBlock. Every commit was verified twice, a block apart, at roughly 1.9 ms each. The applier now records what it verified through a new Executor.NoteVerifiedCommit and validateBlock takes a lastCommitVerified hint. The skip fires only on an exact match of every input ValidatorSet.verifyCommit reads: chain ID, height, block ID, quorum type, quorum hash, threshold public key, and a byte-identical marshalled commit. Marshalling rather than using Commit.Hash() matters, because that hash only covers ThresholdBlockSignature and would miss Round and the vote extensions. Anything short of a full match falls through to a real verification. The note is stored after ApplyBlock returns, not before: both places that would re-verify this commit run while the *next* block is applied, so writing it earlier would overwrite the entry they are still reading for the current block and the skip would never fire. The validator set is captured before ApplyBlock, which reassigns e.state. The consensus path never calls NoteVerifiedCommit, so the memo stays nil there and nothing changes. It is an atomic.Pointer written and read whole, never read-modify-write, and blockApplier.Apply is serialised under its own mutex. Replaying mainnet, the applier's verify stage goes from 3.61 ms to 1.84 ms a block.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔍 Review in progress — actively reviewing now (commit f30ba09) |
2 participants
Block sync threshold-verifies each commit twice.
blockApplier.verifyverifies the commit for block N againststate.Validators. One height later the identical commit arrives again as block N+1'sLastCommitandvalidateBlockverifies it a second time againststate.LastValidators. A BLS threshold verification costs about 1.9 ms, so every block pays for one it has already done.The applier now records what it verified through a new
Executor.NoteVerifiedCommit, andvalidateBlocktakes alastCommitVerifiedhint.Why the skip is safe
ValidatorSet.verifyCommitreads exactly:chainID,height(againstcommit.Height),blockID(againstcommit.BlockID),vals.QuorumType,vals.QuorumHash,vals.ThresholdPublicKey, and the commit itself — the canonical vote and the quorum signs are both derived from it.The memo pins every one of those, and the skip fires only on a full match: same chain ID, height, block ID, quorum type, quorum hash, threshold public key, and a byte-identical marshalled commit. Anything short of that falls through to a real verification.
Marshalling the commit rather than using
Commit.Hash()is deliberate: that hash only coversThresholdBlockSignature, so it would not notice a differentRoundor different vote extensions.Ordering
The note is stored after
ApplyBlockreturns, not before. Both places that would re-verify this commit run while the next block is applied, so writing it earlier would overwrite the entry they are still reading for the current block and the skip would never fire. The validator set is captured beforeApplyBlock, which reassignse.state.Consensus is unaffected
The consensus path never calls
NoteVerifiedCommit, so the memo stays nil there andvalidateBlockbehaves exactly as before. The memo is anatomic.Pointerwritten and read whole, never read-modify-write, andblockApplier.Applyis serialised under its own mutex.Measurements
Replaying mainnet history from a local peer, 3,000-block window at height 190k,
v1.7-devagainst this branch, back to back:verifystagev1.7-dev−1.91 ms off the verify stage, −19.5% off the block.
Numbers taken with tenderdash's per-block fsync removed, because Go's
File.Sync()on macOS issuesF_FULLFSYNC(6.78 ms here versus 0.23 ms for a plain fsync) and would otherwise swamp everything else. Linux nodes do not pay that.Tests
internal/state/...,internal/blocksync/...,internal/consensus,types/...all pass.🤖 Generated with Claude Code