From 08965372a4425dac6e7625ade02e1795fe501503 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 11 Dec 2024 10:01:30 +0100 Subject: [PATCH 1/4] fix(consensus): stopped abci client is not recoverable --- abci/client/socket_client.go | 6 +++++- internal/consensus/state_try_add_commit.go | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/abci/client/socket_client.go b/abci/client/socket_client.go index 0726bb85f9..25b77f55c1 100644 --- a/abci/client/socket_client.go +++ b/abci/client/socket_client.go @@ -38,6 +38,10 @@ type socketClient struct { var _ Client = (*socketClient)(nil) +var ( + ErrClientStopped = errors.New("client has stopped") +) + // NewSocketClient creates a new socket client, which connects to a given // address. If mustConnect is true, the client will return an error upon start // if it fails to connect. @@ -234,7 +238,7 @@ func (cli *socketClient) didRecvResponse(res *types.Response) error { func (cli *socketClient) doRequest(ctx context.Context, req *types.Request) (*types.Response, error) { if !cli.IsRunning() { - return nil, errors.New("client has stopped") + return nil, ErrClientStopped } reqres := makeReqRes(ctx, req) diff --git a/internal/consensus/state_try_add_commit.go b/internal/consensus/state_try_add_commit.go index 4537d37a2d..ba1964aa6b 100644 --- a/internal/consensus/state_try_add_commit.go +++ b/internal/consensus/state_try_add_commit.go @@ -2,8 +2,10 @@ package consensus import ( "context" + "errors" "fmt" + abciclient "github.com/dashpay/tenderdash/abci/client" "github.com/dashpay/tenderdash/dash" cstypes "github.com/dashpay/tenderdash/internal/consensus/types" "github.com/dashpay/tenderdash/libs/log" @@ -108,6 +110,10 @@ func (cs *TryAddCommitAction) verifyCommit(ctx context.Context, stateData *State // We have a correct block, let's process it before applying the commit err = cs.blockExec.ensureProcess(ctx, &stateData.RoundState, commit.Round) if err != nil { + if errors.Is(err, abciclient.ErrClientStopped) { + // this is a non-recoverable error in current architecture + panic(fmt.Errorf("ABCI client stopped, Tenderdash needs to be restarted: %w", err)) + } return false, fmt.Errorf("unable to process proposal: %w", err) } err = cs.blockExec.validate(ctx, stateData) From 8b6f737a7d704ebba0f4cbc4327278c5cd324dcd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 11 Dec 2024 11:21:05 +0100 Subject: [PATCH 2/4] test(consensus): ensure client stopped error causes panic --- internal/consensus/state_test.go | 80 ++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/internal/consensus/state_test.go b/internal/consensus/state_test.go index 5604c0cd44..b3895cd798 100644 --- a/internal/consensus/state_test.go +++ b/internal/consensus/state_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + abciclient "github.com/dashpay/tenderdash/abci/client" + clientmocks "github.com/dashpay/tenderdash/abci/client/mocks" "github.com/dashpay/tenderdash/abci/example/kvstore" abci "github.com/dashpay/tenderdash/abci/types" abcimocks "github.com/dashpay/tenderdash/abci/types/mocks" @@ -22,6 +24,7 @@ import ( "github.com/dashpay/tenderdash/internal/mempool" tmpubsub "github.com/dashpay/tenderdash/internal/pubsub" tmquery "github.com/dashpay/tenderdash/internal/pubsub/query" + sm "github.com/dashpay/tenderdash/internal/state" sf "github.com/dashpay/tenderdash/internal/state/test/factory" "github.com/dashpay/tenderdash/internal/test/factory" tmbytes "github.com/dashpay/tenderdash/libs/bytes" @@ -3272,6 +3275,83 @@ func TestStateTryAddCommitCallsProcessProposal(t *testing.T) { assert.NoError(t, err) } +// TestStateTryAddCommitPanicsOnClientError ensures that +// given ABCI client that errors on ProcessProposal, +// when new block is about to be processed, +// then the TryAddCommitEvent causes a panic. +func TestStateTryAddCommitPanicsOnClientError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + config := configSetup(t) + t.Helper() + + // setup some node and commit + genDoc, privVals := factory.RandGenesisDoc(1, factory.ConsensusParams()) + logger := consensusLogger(t) + + state, err := sm.MakeGenesisState(genDoc) + require.NoError(t, err) + + // Create a panicking app + app := clientmocks.NewClient(t) + app.On("ProcessProposal", mock.Anything, mock.Anything). + Return(&abci.ResponseProcessProposal{}, abciclient.ErrClientStopped). + Once() + + // create a new consensus state + proTxHash, _ := privVals[0].GetProTxHash(ctx) + ctx = dash.ContextWithProTxHash(ctx, proTxHash) + consensusState := newStateWithConfig(ctx, t, logger, config, state, privVals[0], app) + + stateData := consensusState.GetStateData() + + // create proposal and a block to be processed + block, err := sf.MakeBlock(state, 1, &types.Commit{}, kvstore.ProtocolVersion) + require.NoError(t, err) + require.NotZero(t, block.Version.App) + block.CoreChainLockedHeight = 1 + + commit, err := factory.MakeCommit( + ctx, + block.BlockID(nil), + block.Height, + 0, + stateData.Votes.Precommits(0), + state.Validators, + privVals, + ) + require.NoError(t, err) + + proposal := types.NewProposal( + block.Height, + block.CoreChainLockedHeight, + 0, + -1, + commit.BlockID, + block.Time) + + parts, err := block.MakePartSet(999999999) + require.NoError(t, err) + + // emulate that the node has received proposal and block + peerID := stateData.Validators.Proposer().NodeAddress.NodeID + stateData.Proposal = proposal + stateData.ProposalBlock = block + stateData.ProposalBlockParts = parts + stateData.updateRoundStep(commit.Round, cstypes.RoundStepPrevote) + + // invoke the TryAddCommitEvent to see if it will panic + ctx = msgInfoWithCtx(ctx, msgInfo{ + Msg: &CommitMessage{commit}, + PeerID: peerID, + ReceiveTime: time.Time{}, + }) + + assert.Panics(t, func() { + _ = consensusState.ctrl.Dispatch(ctx, &TryAddCommitEvent{Commit: commit, PeerID: peerID}, &stateData) + }) +} + // TestStateTimestamp_ProposalMatch tests that a validator prevotes a // proposed block if the timestamp in the block matches the timestamp in the // corresponding proposal message. From 47c8da0d37b662f03664f9b544577b730ec64742 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 11 Dec 2024 11:26:21 +0100 Subject: [PATCH 3/4] doc(abciclient): document ErrClientStopped --- abci/client/socket_client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/abci/client/socket_client.go b/abci/client/socket_client.go index 25b77f55c1..fb046187d8 100644 --- a/abci/client/socket_client.go +++ b/abci/client/socket_client.go @@ -39,6 +39,7 @@ type socketClient struct { var _ Client = (*socketClient)(nil) var ( + // ErrClientStopped is returned when client wasn't started yet or it was terminated with an error. ErrClientStopped = errors.New("client has stopped") ) From b1c915de4387acbbc4296f506e29fcafd6994f9e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 11 Dec 2024 11:42:06 +0100 Subject: [PATCH 4/4] test(consensus): apply rabbit feedback --- internal/consensus/state_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/consensus/state_test.go b/internal/consensus/state_test.go index b3895cd798..6aa41f283f 100644 --- a/internal/consensus/state_test.go +++ b/internal/consensus/state_test.go @@ -3283,7 +3283,6 @@ func TestStateTryAddCommitPanicsOnClientError(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() config := configSetup(t) - t.Helper() // setup some node and commit genDoc, privVals := factory.RandGenesisDoc(1, factory.ConsensusParams()) @@ -3299,7 +3298,8 @@ func TestStateTryAddCommitPanicsOnClientError(t *testing.T) { Once() // create a new consensus state - proTxHash, _ := privVals[0].GetProTxHash(ctx) + proTxHash, err := privVals[0].GetProTxHash(ctx) + require.NoError(t, err) ctx = dash.ContextWithProTxHash(ctx, proTxHash) consensusState := newStateWithConfig(ctx, t, logger, config, state, privVals[0], app) @@ -3346,10 +3346,11 @@ func TestStateTryAddCommitPanicsOnClientError(t *testing.T) { PeerID: peerID, ReceiveTime: time.Time{}, }) - - assert.Panics(t, func() { - _ = consensusState.ctrl.Dispatch(ctx, &TryAddCommitEvent{Commit: commit, PeerID: peerID}, &stateData) - }) + assert.PanicsWithError(t, + "ABCI client stopped, Tenderdash needs to be restarted: ProcessProposal abci method: client has stopped", + func() { + _ = consensusState.ctrl.Dispatch(ctx, &TryAddCommitEvent{Commit: commit, PeerID: peerID}, &stateData) + }) } // TestStateTimestamp_ProposalMatch tests that a validator prevotes a