Skip to content

fix(finality-grandpa): release the voter lock on processIncoming's early returns - #4853

Open
dimartiro wants to merge 1 commit into
developmentfrom
diego/finality-grandpa-catchup-unlock
Open

dimartiro wants to merge 1 commit into
developmentfrom
diego/finality-grandpa-catchup-unlock

Conversation

@dimartiro

Copy link
Copy Markdown
Contributor

Changes

processIncoming takes v.inner and had two early returns that left it held:

  • Rejected catch-up. When validateCatchUp returns nil, the outcome callback ran and the function returned without unlocking. That covers a catch-up at or below the best round, one short of the threshold, and one that fails to import or isn't completable. The next v.inner.Lock() (pruneBackgroundRounds on the following poll) then blocks forever, and so does every VoterState().Get(): the voter stops for good, and stopping it can only time out. It now unlocks right after the rejection, so the Bad callback also runs outside the lock.
  • ValidateCommit error. The commit path returned the error with the lock held. The voter ends on that error anyway, but callers of VoterState().Get() then block forever. It now unlocks first, as the FinalizeBlock error path a few lines below already does.

In the Rust crate both returns release the lock when the guard drops.

Tests

go test -race -run TestVoter_StaysLiveAfterRejectedCatchUp ./pkg/finality-grandpa

TestVoter_StaysLiveAfterRejectedCatchUp sends a valid catch-up, the same one again (now stale), one short of the threshold, and a valid one for a later round. It checks each outcome and that the voter ends at round 8. Without the fix it fails at the third: the voter never processes it.

The ValidateCommit path has no test. Its error needs an environment whose Ancestry fails for a block IsEqualOrDescendantOf accepted.

go test -race ./pkg/finality-grandpa passes. TestVoter_SkipsToLatestRoundAfterCatchUp is already flaky on development, about 1 run in 40 without this change: it asserts round 5 is not yet a background round right after sending the catch-up, which races the voter.

Issues

Surfaced by ChainSafe/go-jam#850, whose catch-up fan-out can hand the voter more than one answer for the same round: the second one wedged it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants