fix(controller): avoid panic when producing a proposal before the mempool has cached one#455
Open
amathxbt wants to merge 2 commits into
Open
fix(controller): avoid panic when producing a proposal before the mempool has cached one#455amathxbt wants to merge 2 commits into
amathxbt wants to merge 2 commits into
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.
Bug
Controller.GetProposalBlockFromMempool()(incontroller/tx.go) does an unchecked type assertion on anatomic.Value:cachedProposalis anatomic.Value{}zero value untilCheckMempool()successfully stores the first*CachedProposal. CallingLoad()on anatomic.Valuethat has never been written returnsnil(the untyped zero value), and assertingnil.(*CachedProposal)without the, okform panics withinterface conversion: interface {} is nil, not *controller.CachedProposal.This is reachable from
Controller.ProduceProposal()(incontroller/block.go), which is invoked by the BFT module whenever this node is selected as the block proposer:If a node is elected proposer before its background
CheckMempool()loop has completed even one pass (e.g. right after startup, or whenLazyMempoolCheckFrequencySis configured such that the first tick hasn't fired yet), this panics the node instead of returning a handled error.Fix
GetProposalBlockFromMempool()now uses the safe two-value type assertion and returnsnilinstead of panicking when no proposal has been cached yet.ProduceProposal()now checks for anilresult immediately after callingGetProposalBlockFromMempool()and returnslib.ErrNilBlock()(an existing, semantically-appropriate error already used elsewhere in the codebase) instead of dereferencing a nil pointer a few lines later atp.Block.BlockHeader....Testing
Traced the only call site of
GetProposalBlockFromMempool()in the repository (controller/block.go:ProduceProposal) to confirm the nil case is now handled before any dereference ofp, and thaterris a pre-declared named return so no signature changes were required.