From 60d330aa7e3ddd6e797126722c231b9388c663d9 Mon Sep 17 00:00:00 2001 From: Youssef Azzaoui Date: Wed, 20 Dec 2023 18:21:12 +0100 Subject: [PATCH] Fix proposal handling flow issue Signed-off-by: Youssef Azzaoui --- cmd/utils/flags.go | 1 + consensus/tendermint/core/constants/error.go | 2 ++ consensus/tendermint/core/handler.go | 2 ++ consensus/tendermint/core/propose.go | 25 ++++++++++++++------ miner/worker_test.go | 2 +- 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 3850165dd6..4de08f0f19 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -1603,6 +1603,7 @@ func RegisterEthService(stack *node.Node, cfg *ethconfig.Config) (ethapi.Backend return backend.ApiBackend, nil } backend, err := eth.New(stack, cfg) + if err != nil { Fatalf("Failed to register the Autonity service: %v", err) } diff --git a/consensus/tendermint/core/constants/error.go b/consensus/tendermint/core/constants/error.go index f2110a142e..1b2ec88b7b 100644 --- a/consensus/tendermint/core/constants/error.go +++ b/consensus/tendermint/core/constants/error.go @@ -6,6 +6,8 @@ var ( // ErrNotFromProposer is returned when received message is supposed to be from // proposer. ErrNotFromProposer = errors.New("message does not come from proposer") + // ErrAlreadyProcessed is returned when a received proposal was already processed in a specific round. + ErrAlreadyProcessed = errors.New("proposal was already processed") // ErrFutureHeightMessage is returned when curRoundMessages view is earlier than the // view of the received message. ErrFutureHeightMessage = errors.New("future height message") diff --git a/consensus/tendermint/core/handler.go b/consensus/tendermint/core/handler.go index a625c81c05..5912f7eff7 100644 --- a/consensus/tendermint/core/handler.go +++ b/consensus/tendermint/core/handler.go @@ -89,6 +89,8 @@ func shouldDisconnectSender(err error) bool { case errors.Is(err, constants.ErrNilPrecommitSent): fallthrough case errors.Is(err, constants.ErrMovedToNewRound): + fallthrough + case errors.Is(err, constants.ErrAlreadyProcessed): return false case errors.Is(err, ErrValidatorJailed): // this one is tricky. Ideally yes, we want to disconnect the sender but we can't diff --git a/consensus/tendermint/core/propose.go b/consensus/tendermint/core/propose.go index 92744d89e4..28f85dfdc7 100644 --- a/consensus/tendermint/core/propose.go +++ b/consensus/tendermint/core/propose.go @@ -42,17 +42,17 @@ func (c *Proposer) HandleProposal(ctx context.Context, proposal *message.Propose // message to the backlog is fine. if errors.Is(err, constants.ErrOldRoundMessage) { roundMessages := c.messages.GetOrCreate(proposal.R()) - // if we already have a proposal then it must be different than the current one - // it can't happen unless someone's byzantine. + // if we already have a proposal for this old round - ignore + // the first proposal sent by the sender in a round is always the only one we consider. if roundMessages.Proposal() != nil { - return err // do not gossip, TODO: accountability + return constants.ErrAlreadyProcessed } if !c.IsFromProposer(proposal.R(), proposal.Sender()) { c.logger.Warn("Ignoring proposal from non-proposer") return constants.ErrNotFromProposer } - // We do not verify the proposal in this case. + // Save it, but do not verify the proposal yet unless we have enough precommits for it. roundMessages.SetProposal(proposal, false) if roundMessages.PrecommitsPower(proposal.Block().Hash()).Cmp(c.CommitteeSet().Quorum()) >= 0 { if _, err2 := c.backend.VerifyProposal(proposal.Block()); err2 != nil { @@ -65,6 +65,12 @@ func (c *Proposer) HandleProposal(ctx context.Context, proposal *message.Propose } return err } + // if we already have processed a proposal in this round we ignore. + if c.curRoundMessages.Proposal() != nil { + return constants.ErrAlreadyProcessed + } + // At this point the local round matches the message round and the current step + // could be either Proposal, Prevote, or Precommit. // Check if the message comes from curRoundMessages proposer if !c.IsFromProposer(c.Round(), proposal.Sender()) { @@ -104,9 +110,14 @@ func (c *Proposer) HandleProposal(ctx context.Context, proposal *message.Propose }) return err } - c.prevoter.SendPrevote(ctx, true) - // do not to accept another proposal in current round - c.SetStep(Prevote) + // Proposal is invalid here, we need to prevote nil. + // However, we may have already sent a prevote nil in the past without having processed the proposal + // because of a timeout, so we need to check if we are still in the Propose step. + if c.step == Propose { + c.prevoter.SendPrevote(ctx, true) + // do not to accept another proposal in current round + c.SetStep(Prevote) + } c.logger.Warn("Failed to verify proposal", "err", err, "duration", duration) diff --git a/miner/worker_test.go b/miner/worker_test.go index 70afe9cbe4..e08fa8aa8e 100644 --- a/miner/worker_test.go +++ b/miner/worker_test.go @@ -282,7 +282,7 @@ func testGenerateBlockAndImport(t *testing.T, isTendermint bool) { if _, err := chain.InsertChain([]*types.Block{block}); err != nil { t.Fatalf("failed to insert new mined block %d: %v", block.NumberU64(), err) } - case <-time.After(3 * time.Second): // Worker needs 1s to include new changes. + case <-time.After(5 * time.Second): // Worker needs 1s to include new changes. t.Fatalf("timeout") } }