Avoid mutating commit bitmap during certificate comparison - #5096
Open
Frozen wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
There is a potential order-dependent race between two valid commit certificates for the same block:
COMMITTEDmessage arrives with more signer slots;The existing comparison reuses the mutable bitmap decoded from the incoming message. To count signers in the stored certificate, it calls
SetMaskon that same bitmap object. This can overwrite the active incoming bitmap with the older stored bitmap while the aggregate signature still belongs to the incoming certificate.For example:
7/15signer slots;11/15signer slots;11/15certificate;11/15, but bitmap changed back to7/15during comparison.This is a logical state race caused by update ordering and shared mutable state, rather than a Go race-detector data race.
Fix
This PR makes certificate selection pure and monotonic:
After this change, the comparison itself cannot roll the active bitmap back to the older certificate, and the persisted certificate can only move to a more complete value.
The incoming certificate is still cryptographically and quorum-verified by the existing
VerifyHeaderSignaturepath before this comparison runs.Scope
This is a focused correctness fix based directly on
dev. It does not change consensus quorum rules, BLS verification, wire formats, block timing, leader rotation, retry behavior, or feature activation epochs.[Test]
make testgo test ./consensus -run '^TestIsMoreCompleteCommitPayload$' -count=1git diff --check