diff --git a/dash/quorum/validator_conn_executor_test.go b/dash/quorum/validator_conn_executor_test.go index 47c5d88fe..4d6e715b6 100644 --- a/dash/quorum/validator_conn_executor_test.go +++ b/dash/quorum/validator_conn_executor_test.go @@ -424,10 +424,10 @@ func TestFinalizeBlock(t *testing.T) { require.NoError(t, err) block.NextValidatorsHash = newVals.Hash() const round = int32(0) - candidateState, err := blockExec.ProcessProposal(ctx, block, round, state, true) + candidateState, err := blockExec.ProcessProposal(ctx, block, round, state, true, types.VerifiedCommit{}) require.NoError(t, err) - state, err = blockExec.FinalizeBlock(ctx, state, candidateState, blockID, block, new(types.Commit)) + state, err = blockExec.FinalizeBlock(ctx, state, candidateState, blockID, block, new(types.Commit), types.VerifiedCommit{}) require.NoError(t, err) // test new validator was added to NextValidators diff --git a/internal/blocksync/applier.go b/internal/blocksync/applier.go index 9c92f7ee3..e139bb941 100644 --- a/internal/blocksync/applier.go +++ b/internal/blocksync/applier.go @@ -23,6 +23,12 @@ type ( state sm.State metrics *consensus.Metrics stats applyStats + // lastCommit is the commit of the block most recently applied onto state, + // with the proof of its verification. The next block carries that commit + // as its LastCommit, so this is offered back when that block is validated + // and applied, sparing a second threshold verification of the same commit. + // Guarded by mtx, like state. + lastCommit types.VerifiedCommit // lastDone is when the previous Apply returned, so the time the applier // sits idle waiting for the next block can be measured lastDone time.Time @@ -85,7 +91,7 @@ func (e *blockApplier) Apply(ctx context.Context, block *types.Block, commit *ty partSetTime := e.observeSince("partset", start) start = time.Now() - err = e.verify(ctx, blockID, block, commit) + verified, err := e.verify(ctx, blockID, block, commit) if err != nil { return err } @@ -94,7 +100,7 @@ func (e *blockApplier) Apply(ctx context.Context, block *types.Block, commit *ty // Validate the app response before persisting; save before FinalizeBlock so // crash recovery never finds the block store behind the application. start = time.Now() - uncommittedState, err := e.blockExec.ProcessProposal(ctx, block, commit.Round, e.state, true) + uncommittedState, err := e.blockExec.ProcessProposal(ctx, block, commit.Round, e.state, true, e.lastCommit) if err != nil { panic(fmt.Sprintf("failed to process committed block (%d:%X): %v", block.Height, block.Hash(), err)) } @@ -105,10 +111,12 @@ func (e *blockApplier) Apply(ctx context.Context, block *types.Block, commit *ty saveTime := e.observeSince("save", start) start = time.Now() - e.state, err = e.blockExec.FinalizeBlock(ctx, e.state, uncommittedState, blockID, block, commit) + e.state, err = e.blockExec.FinalizeBlock(ctx, e.state, uncommittedState, blockID, block, commit, e.lastCommit) if err != nil { panic(fmt.Sprintf("failed to finalize committed block (%d:%X): %v", block.Height, block.Hash(), err)) } + // commit comes back as the next block's LastCommit + e.lastCommit = verified execTime := processTime + time.Since(start) e.metrics.ObserveBlockSyncStage("exec", execTime) @@ -138,14 +146,22 @@ func (e *blockApplier) UpdateState(newState sm.State) { e.mtx.Lock() defer e.mtx.Unlock() e.state = newState + // the commit lastCommit verified was applied onto the replaced state, not onto + // newState + e.lastCommit = types.VerifiedCommit{} } -func (e *blockApplier) verify(ctx context.Context, blockID types.BlockID, block *types.Block, commit *types.Commit) error { - // The two checks are timed separately: the commit check is a BLS threshold - // signature verification and is nearly the whole stage, block validation is - // free. +// verify checks commit and then block against the current state, before the +// block is persisted. It returns commit with the proof of its verification; +// the next block carries commit as its LastCommit. +func (e *blockApplier) verify( + ctx context.Context, + blockID types.BlockID, + block *types.Block, + commit *types.Commit, +) (types.VerifiedCommit, error) { start := time.Now() - err := e.state.Validators.VerifyCommit(e.state.ChainID, blockID, block.Height, commit) + verified, err := e.blockExec.VerifyCommit(e.state, blockID, block.Height, commit) e.observeSince("verify_commit", start) // If either of the checks failed we log the error and request for a new block @@ -157,11 +173,11 @@ func (e *blockApplier) verify(ctx context.Context, blockID types.BlockID, block "block_id", blockID, "height", block.Height, ) - return err + return types.VerifiedCommit{}, err } // validate the block before we persist it start = time.Now() - err = e.blockExec.ValidateBlock(ctx, e.state, block) + err = e.blockExec.ValidateBlock(ctx, e.state, block, e.lastCommit) e.observeSince("verify_block", start) if err != nil { err = fmt.Errorf("invalid block: %w", err) @@ -170,9 +186,9 @@ func (e *blockApplier) verify(ctx context.Context, blockID types.BlockID, block "block_id", blockID, "height", block.Height, ) - return err + return types.VerifiedCommit{}, err } - return nil + return verified, nil } // observeSince records the time since start under stage and returns it, so the diff --git a/internal/blocksync/applier_test.go b/internal/blocksync/applier_test.go index bccafafc1..3a3c0cc7a 100644 --- a/internal/blocksync/applier_test.go +++ b/internal/blocksync/applier_test.go @@ -15,6 +15,9 @@ import ( abci "github.com/dashpay/tenderdash/abci/types" "github.com/dashpay/tenderdash/crypto" "github.com/dashpay/tenderdash/internal/consensus" + "github.com/dashpay/tenderdash/internal/eventbus" + mpmocks "github.com/dashpay/tenderdash/internal/mempool/mocks" + "github.com/dashpay/tenderdash/internal/proxy" sm "github.com/dashpay/tenderdash/internal/state" "github.com/dashpay/tenderdash/internal/state/mocks" statefactory "github.com/dashpay/tenderdash/internal/state/test/factory" @@ -93,25 +96,46 @@ func TestBlockApplierApply(t *testing.T) { mockFn: func() { mockBlockStore.On("SaveBlock", blockH1, blockH1Parts, commitH1).Once() mockBlockExec. - On("ValidateBlock", mock.Anything, initialState, blockH1). + On("VerifyCommit", initialState, blockH1ID, blockH1.Height, commitH1). + Once(). + Return(types.VerifiedCommit{}, nil) + mockBlockExec. + On("ValidateBlock", mock.Anything, initialState, blockH1, types.VerifiedCommit{}). Once(). Return(nil) mockBlockExec. - On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true). + On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true, types.VerifiedCommit{}). Once(). Return(sm.CurrentRoundState{}, nil) mockBlockExec. - On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, blockH1ID, blockH1, commitH1). + On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, blockH1ID, blockH1, commitH1, types.VerifiedCommit{}). Once(). Return(state, nil) }, }, + { + // a commit the executor rejects stops the block before it is validated, + // saved or applied + block: blockH1, + commit: commitH1, + mockFn: func() { + mockBlockExec. + On("VerifyCommit", initialState, blockH1ID, blockH1.Height, commitH1). + Once(). + Return(types.VerifiedCommit{}, errors.New("bad signature")) + }, + wantErr: "invalid a commit: bad signature", + }, { block: blockH1, commit: commitH1, mockFn: func() { mockBlockExec. - On("ValidateBlock", mock.Anything, initialState, blockH1). + On("VerifyCommit", initialState, blockH1ID, blockH1.Height, commitH1). + Once(). + Return(types.VerifiedCommit{}, nil) + mockBlockExec. + On("ValidateBlock", mock.Anything, initialState, blockH1, types.VerifiedCommit{}). Once(). Return(errors.New("invalid block")) }, @@ -123,15 +147,19 @@ func TestBlockApplierApply(t *testing.T) { mockFn: func() { mockBlockStore.On("SaveBlock", blockH1, blockH1Parts, commitH1).Once() mockBlockExec. - On("ValidateBlock", mock.Anything, initialState, blockH1). + On("VerifyCommit", initialState, blockH1ID, blockH1.Height, commitH1). + Once(). + Return(types.VerifiedCommit{}, nil) + mockBlockExec. + On("ValidateBlock", mock.Anything, initialState, blockH1, types.VerifiedCommit{}). Once(). Return(nil) mockBlockExec. - On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true). + On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true, types.VerifiedCommit{}). Once(). Return(sm.CurrentRoundState{}, nil) mockBlockExec. - On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, blockH1ID, blockH1, commitH1). + On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, blockH1ID, blockH1, commitH1, types.VerifiedCommit{}). Once(). Return(state, errors.New("eeeeeeeee")) }, @@ -225,11 +253,15 @@ func TestBlockApplierDoesNotSaveBlockRejectedByApp(t *testing.T) { block, commit := blocks[0], blocks[1].LastCommit mockBlockExec. - On("ValidateBlock", mock.Anything, initialState, block). + On("VerifyCommit", initialState, block.BlockID(nil), block.Height, commit). + Once(). + Return(types.VerifiedCommit{}, nil) + mockBlockExec. + On("ValidateBlock", mock.Anything, initialState, block, types.VerifiedCommit{}). Once(). Return(nil) mockBlockExec. - On("ProcessProposal", mock.Anything, block, commit.Round, initialState, true). + On("ProcessProposal", mock.Anything, block, commit.Round, initialState, true, types.VerifiedCommit{}). Once(). Return(sm.CurrentRoundState{}, errors.New("app rejected the block")) @@ -256,11 +288,15 @@ func TestBlockApplierSavesBlockBeforeFinalize(t *testing.T) { var calls []string mockBlockExec. - On("ValidateBlock", mock.Anything, initialState, block). + On("VerifyCommit", initialState, block.BlockID(nil), block.Height, commit). + Once(). + Return(types.VerifiedCommit{}, nil) + mockBlockExec. + On("ValidateBlock", mock.Anything, initialState, block, types.VerifiedCommit{}). Once(). Return(nil) mockBlockExec. - On("ProcessProposal", mock.Anything, block, commit.Round, initialState, true). + On("ProcessProposal", mock.Anything, block, commit.Round, initialState, true, types.VerifiedCommit{}). Once(). Run(func(mock.Arguments) { calls = append(calls, "process") }). Return(sm.CurrentRoundState{}, nil) @@ -269,7 +305,7 @@ func TestBlockApplierSavesBlockBeforeFinalize(t *testing.T) { Once(). Run(func(mock.Arguments) { calls = append(calls, "save") }) mockBlockExec. - On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, block.BlockID(blockParts), block, commit). + On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, block.BlockID(blockParts), block, commit, types.VerifiedCommit{}). Once(). Run(func(mock.Arguments) { calls = append(calls, "finalize") }). Return(state, nil) @@ -295,11 +331,14 @@ func TestBlockApplierRecordsStageMetrics(t *testing.T) { blockH1 := blocks[0] commitH1 := blocks[1].LastCommit + mockBlockExec.On("VerifyCommit", initialState, blockH1.BlockID(nil), blockH1.Height, commitH1).Twice().Return(types.VerifiedCommit{}, nil) + mockBlockExec.On("VerifyCommit", initialState, blockH1.BlockID(nil), blockH1.Height, new(types.Commit)). + Once().Return(types.VerifiedCommit{}, errors.New("bad signature")) mockBlockStore.On("SaveBlock", blockH1, mock.Anything, commitH1).Twice() - mockBlockExec.On("ValidateBlock", mock.Anything, mock.Anything, blockH1).Twice().Return(nil) - mockBlockExec.On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true). + mockBlockExec.On("ValidateBlock", mock.Anything, mock.Anything, blockH1, types.VerifiedCommit{}).Twice().Return(nil) + mockBlockExec.On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, initialState, true, types.VerifiedCommit{}). Twice().Return(sm.CurrentRoundState{}, nil) - mockBlockExec.On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, mock.Anything, blockH1, commitH1). + mockBlockExec.On("FinalizeBlock", mock.Anything, initialState, sm.CurrentRoundState{}, mock.Anything, blockH1, commitH1, types.VerifiedCommit{}). Twice().Return(initialState, nil) hist := metricspy.NewHistogram("stage") @@ -356,7 +395,9 @@ func TestBlockApplierVerifyFailureTimesOnlyTheCheckThatRan(t *testing.T) { applier := newBlockApplier(mockBlockExec, mockBlockStore, applierWithState(initialState), applierWithMetrics(m)) - // an empty commit fails the signature check before ValidateBlock is reached + // A rejected commit must stop before ValidateBlock is reached. + mockBlockExec.On("VerifyCommit", initialState, blockH1.BlockID(nil), blockH1.Height, new(types.Commit)). + Once().Return(types.VerifiedCommit{}, errors.New("bad signature")) require.Error(t, applier.Apply(ctx, blockH1, new(types.Commit))) require.Len(t, hist.Samples["partset"], 1) @@ -365,3 +406,133 @@ func TestBlockApplierVerifyFailureTimesOnlyTheCheckThatRan(t *testing.T) { require.Empty(t, hist.Samples["save"]) require.Empty(t, hist.Samples["exec"]) } + +// TestBlockApplierOffersTheVerifiedCommitForward checks that the verified +// commit the executor returns for a block's commit is what the applier offers +// when it validates and applies the next block, whose LastCommit is that +// commit. The first block has none to offer, a rejected commit does not replace +// it, and replacing the state discards it. +func TestBlockApplierOffersTheVerifiedCommitForward(t *testing.T) { + ctx := context.Background() + valSet, privVals := factory.MockValidatorSet() + initialState := fakeInitialState(valSet) + state := initialState.Copy() + blocks := statefactory.MakeBlocks(ctx, t, 3, &state, privVals, 1) + blockH1, blockH2 := blocks[0], blocks[1] + commitH1, commitH2 := blocks[1].LastCommit, blocks[2].LastCommit + blockH1ID, blockH2ID := blockH1.BlockID(nil), blockH2.BlockID(nil) + + // a genuine verification, so the expectations can tell it from the zero value + verifiedH1, err := types.VerifyCommitSignatures(initialState.Validators, initialState.ChainID, + blockH1ID, blockH1.Height, commitH1, nil) + require.NoError(t, err) + require.NotEqual(t, types.VerifiedCommit{}, verifiedH1) + none := types.VerifiedCommit{} + + // applyH1 returns an applier that has applied block 1, offering no + // verification because there is no previous commit + applyH1 := func(t *testing.T) (*blockApplier, *mocks.Executor) { + blockExec := mocks.NewExecutor(t) + blockStore := mocks.NewBlockStore(t) + blockStore.On("SaveBlock", mock.Anything, mock.Anything, mock.Anything).Maybe() + blockExec.On("VerifyCommit", mock.Anything, blockH1ID, blockH1.Height, commitH1).Once().Return(verifiedH1, nil) + blockExec.On("ValidateBlock", mock.Anything, mock.Anything, blockH1, none).Once().Return(nil) + blockExec.On("ProcessProposal", mock.Anything, blockH1, commitH1.Round, mock.Anything, true, none). + Once().Return(sm.CurrentRoundState{}, nil) + blockExec.On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, blockH1ID, blockH1, commitH1, none). + Once().Return(initialState, nil) + applier := newBlockApplier(blockExec, blockStore, applierWithState(initialState)) + require.NoError(t, applier.Apply(ctx, blockH1, commitH1)) + return applier, blockExec + } + // expectH2 expects block 2 to be validated and applied offering lastCommit + expectH2 := func(blockExec *mocks.Executor, lastCommit types.VerifiedCommit) { + blockExec.On("VerifyCommit", mock.Anything, blockH2ID, blockH2.Height, commitH2).Once().Return(none, nil) + blockExec.On("ValidateBlock", mock.Anything, mock.Anything, blockH2, lastCommit).Once().Return(nil) + blockExec.On("ProcessProposal", mock.Anything, blockH2, commitH2.Round, mock.Anything, true, lastCommit). + Once().Return(sm.CurrentRoundState{}, nil) + blockExec.On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, blockH2ID, blockH2, commitH2, lastCommit). + Once().Return(initialState, nil) + } + + t.Run("the next block is offered the previous commit's verification", func(t *testing.T) { + applier, blockExec := applyH1(t) + expectH2(blockExec, verifiedH1) + require.NoError(t, applier.Apply(ctx, blockH2, commitH2)) + }) + + t.Run("a rejected commit does not replace the verification", func(t *testing.T) { + applier, blockExec := applyH1(t) + bad := new(types.Commit) + blockExec.On("VerifyCommit", mock.Anything, blockH2ID, blockH2.Height, bad). + Once().Return(none, errors.New("bad signature")) + require.Error(t, applier.Apply(ctx, blockH2, bad)) + + expectH2(blockExec, verifiedH1) + require.NoError(t, applier.Apply(ctx, blockH2, commitH2)) + }) + + t.Run("replacing the state discards the verification", func(t *testing.T) { + applier, blockExec := applyH1(t) + applier.UpdateState(initialState) + expectH2(blockExec, none) + require.NoError(t, applier.Apply(ctx, blockH2, commitH2)) + }) +} + +// TestBlockApplierSkipsTheLastCommitItVerified runs consecutive blocks through a +// real executor and checks that every block after the first is validated and +// applied without threshold-verifying its LastCommit again: the applier +// verified that commit when it applied the previous block, and the proof is +// handed forward with it. Every other test either mocks the executor or offers +// the proof by hand, so a broken hand-over would leave them all green while +// block sync silently verified each commit twice more. +func TestBlockApplierSkipsTheLastCommitItVerified(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + logger := log.NewNopLogger() + + genDoc, privVals := factory.RandGenesisDoc(1, factory.ConsensusParams()) + state, err := sm.MakeGenesisState(genDoc) + require.NoError(t, err) + stateStore := sm.NewStore(dbm.NewMemDB()) + require.NoError(t, stateStore.Save(state)) + blockStore := store.NewBlockStore(dbm.NewMemDB()) + + app := proxy.New(abciclient.NewLocalClient(logger, &abci.BaseApplication{}), logger, proxy.NopMetrics()) + require.NoError(t, app.Start(ctx)) + eventBus := eventbus.NewDefault(logger) + require.NoError(t, eventBus.Start(ctx)) + mp := &mpmocks.Mempool{} + mp.On("Lock").Return() + mp.On("Unlock").Return() + mp.On("FlushAppConn", mock.Anything).Return(nil) + mp.On("Update", mock.Anything, mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + + skipped := metricspy.NewCounter() + execMetrics := sm.NopMetrics() + execMetrics.LastCommitVerificationSkipped = skipped + blockExec := sm.NewBlockExecutor(stateStore, app, mp, sm.EmptyEvidencePool{}, blockStore, eventBus, + sm.BockExecWithMetrics(execMetrics)) + applier := newBlockApplier(blockExec, blockStore, applierWithState(state)) + + // Height 1 has no LastCommit to verify. Every later height skips its + // threshold verification three times, once per site handed the proof: the + // applier's ValidateBlock before the block is saved, ProcessProposal's check + // of the app response, and FinalizeBlock's. The latter two find the block in + // the executor's per-block cache but still each run + // ValidateBlockWithRoundState's explicit LastCommit check, so any site handed + // a zero proof verifies in full and drops the count. + commit := types.NewCommit(0, 0, types.BlockID{}, nil, nil) + for _, step := range []struct { + height int64 + wantSkipped float64 + }{{1, 0}, {2, 3}, {3, 6}} { + block, _, _, seenCommit := makeNextBlock(ctx, t, applier.State(), privVals[0], step.height, commit) + require.NoError(t, applier.Apply(ctx, block, seenCommit)) + require.Equal(t, step.wantSkipped, skipped.Value(), + "LastCommit threshold verifications skipped after applying height %d", step.height) + commit = seenCommit + } +} diff --git a/internal/blocksync/reactor_test.go b/internal/blocksync/reactor_test.go index e3aacd811..d0f565848 100644 --- a/internal/blocksync/reactor_test.go +++ b/internal/blocksync/reactor_test.go @@ -199,7 +199,7 @@ func (rts *reactorTestSuite) addNode( require.NoError(t, err) for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ { block, blockID, partSet, seenCommit := makeNextBlock(ctx, t, state, privVal, blockHeight, commit) - state, err = reactor.blockExec.ApplyBlock(ctx, state, blockID, block, seenCommit) + state, err = reactor.blockExec.ApplyBlock(ctx, state, blockID, block, seenCommit, types.VerifiedCommit{}) require.NoError(t, err) reactor.store.SaveBlock(block, partSet, seenCommit) commit = seenCommit diff --git a/internal/blocksync/synchronizer_test.go b/internal/blocksync/synchronizer_test.go index 4677059af..c48846de4 100644 --- a/internal/blocksync/synchronizer_test.go +++ b/internal/blocksync/synchronizer_test.go @@ -81,9 +81,13 @@ func (suite *SynchronizerTestSuite) TestBasic() { On("SaveBlock", mock.Anything, mock.Anything, mock.Anything). Maybe() suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything). + On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Maybe(). Return(nil) + suite.blockExec. + On("VerifyCommit", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Maybe(). + Return(types.VerifiedCommit{}, nil) expectApply(suite.blockExec, func(call *mock.Call) { call.Maybe() }) suite.client. On("GetBlock", mock.Anything, mock.Anything, mock.Anything). @@ -195,15 +199,19 @@ func (suite *SynchronizerTestSuite) TestConsumeJobResult() { Once(). Return(nil) suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, respH1.Block). + On("ValidateBlock", mock.Anything, mock.Anything, respH1.Block, mock.Anything). Once(). Return(nil) suite.blockExec. - On("ProcessProposal", mock.Anything, respH1.Block, mock.Anything, mock.Anything, true). + On("VerifyCommit", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Maybe(). + Return(types.VerifiedCommit{}, nil) + suite.blockExec. + On("ProcessProposal", mock.Anything, respH1.Block, mock.Anything, mock.Anything, true, mock.Anything). Once(). Return(sm.CurrentRoundState{}, nil) suite.blockExec. - On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything, respH1.Block, respH1.Commit). + On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything, respH1.Block, respH1.Commit, mock.Anything). Once(). Return(sm.State{}, nil) }, @@ -238,8 +246,10 @@ func (suite *SynchronizerTestSuite) TestConsumeJobResult() { wantPushBack: []int64{1, 2}, mockFn: func(pool *Synchronizer) { pool.pendingToApply[2] = BlockResponse{PeerID: "peer 1", Block: respH2.Block} + // VerifyCommit is covered by the Maybe expectation the earlier case + // registered on this shared mock suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, respH1.Block). + On("ValidateBlock", mock.Anything, mock.Anything, respH1.Block, mock.Anything). Once(). Return(errors.New("invalid error")) suite.client. @@ -415,9 +425,13 @@ func (suite *SynchronizerTestSuite) TestConsumeDuplicateThenDrain() { On("SaveBlock", mock.Anything, mock.Anything, mock.Anything). Twice() suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything). + On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Twice(). Return(nil) + suite.blockExec. + On("VerifyCommit", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Maybe(). + Return(types.VerifiedCommit{}, nil) expectApply(suite.blockExec, func(call *mock.Call) { call.Twice() }) resultCh <- workerpool.Result{Value: respH1} @@ -481,7 +495,11 @@ func (suite *SynchronizerTestSuite) TestApplyFailurePunishesSupplyingPeer() { pool.pendingToApply[poisonH1.Block.Height] = *poisonH1 suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, poisonH1.Block). + On("VerifyCommit", mock.Anything, mock.Anything, poisonH1.Block.Height, poisonH1.Commit). + Once(). + Return(types.VerifiedCommit{}, nil) + suite.blockExec. + On("ValidateBlock", mock.Anything, mock.Anything, poisonH1.Block, mock.Anything). Once(). Return(errors.New("invalid block")) suite.client. @@ -1285,9 +1303,13 @@ func (suite *SynchronizerTestSuite) newBacklogHarness() *backlogHarness { On("SaveBlock", mock.Anything, mock.Anything, mock.Anything). Maybe() suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything). + On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Maybe(). Return(nil) + suite.blockExec. + On("VerifyCommit", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Maybe(). + Return(types.VerifiedCommit{}, nil) expectApply(suite.blockExec, func(call *mock.Call) { call.Maybe() }) jobCh := make(chan *workerpool.Job, 1) @@ -1730,9 +1752,13 @@ func (suite *SynchronizerTestSuite) TestClientTimeoutUnwedgesAFullWindow() { On("SaveBlock", mock.Anything, mock.Anything, mock.Anything). Maybe() suite.blockExec. - On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything). + On("ValidateBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Maybe(). Return(nil) + suite.blockExec. + On("VerifyCommit", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Maybe(). + Return(types.VerifiedCommit{}, nil) expectApply(suite.blockExec, func(call *mock.Call) { call.Maybe() }) applier := newBlockApplier(suite.blockExec, suite.store, applierWithState(suite.initialState)) @@ -1807,11 +1833,11 @@ func (suite *SynchronizerTestSuite) TestConsumeJobResultKeepsPeerOnTransientFail // unchanged. expect applies the same cardinality to both. func expectApply(exec *mocks.Executor, expect func(*mock.Call)) { expect(exec. - On("ProcessProposal", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + On("ProcessProposal", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(sm.CurrentRoundState{}, nil)) expect(exec. - On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(func(_ context.Context, state sm.State, _ sm.CurrentRoundState, _ types.BlockID, _ *types.Block, _ *types.Commit) sm.State { + On("FinalizeBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(func(_ context.Context, state sm.State, _ sm.CurrentRoundState, _ types.BlockID, _ *types.Block, _ *types.Commit, _ types.VerifiedCommit) sm.State { return state }, nil)) } diff --git a/internal/consensus/block_executor.go b/internal/consensus/block_executor.go index 3234c6397..5e828146d 100644 --- a/internal/consensus/block_executor.go +++ b/internal/consensus/block_executor.go @@ -70,7 +70,10 @@ func (c *blockExecutor) ensureProcess(ctx context.Context, rs *cstypes.RoundStat crs := rs.CurrentRoundState if crs.Params.Source != sm.ProcessProposalSource || !crs.MatchesBlock(block.Header, round) { c.logger.Trace("CurrentRoundState is outdated, executing ProcessProposal", "crs", crs) - uncommittedState, err := c.blockExec.ProcessProposal(ctx, block, round, c.committedState, true) + // consensus holds no proof for the block's LastCommit, so it is verified + // in full + uncommittedState, err := c.blockExec.ProcessProposal(ctx, block, round, c.committedState, true, + types.VerifiedCommit{}) if err != nil { return fmt.Errorf("ProcessProposal abci method: %w", err) } @@ -100,12 +103,17 @@ func (c *blockExecutor) finalize(ctx context.Context, stateData *StateData, comm }, block, commit, + // consensus holds no proof for the block's LastCommit, so it is verified + // in full + types.VerifiedCommit{}, ) } func (c *blockExecutor) validate(ctx context.Context, stateData *StateData) error { - // Validate the block. - err := c.blockExec.ValidateBlockWithRoundState(ctx, stateData.state, stateData.CurrentRoundState, stateData.ProposalBlock) + // Validate the block. Consensus holds no proof for its LastCommit, so it is + // verified in full. + err := c.blockExec.ValidateBlockWithRoundState(ctx, stateData.state, stateData.CurrentRoundState, + stateData.ProposalBlock, types.VerifiedCommit{}) if err != nil { step := stateData.Step.String() return fmt.Errorf("invalid block %X (step %s): %w", step, stateData.CurrentRoundState.AppHash, err) diff --git a/internal/consensus/block_executor_test.go b/internal/consensus/block_executor_test.go index 9fa00938e..62045833a 100644 --- a/internal/consensus/block_executor_test.go +++ b/internal/consensus/block_executor_test.go @@ -229,6 +229,7 @@ func (suite *BlockExecutorTestSuite) TestProcess() { round, stateData.state, true, + types.VerifiedCommit{}, ). Once(). Return(tc.wantCRS, wantErr) diff --git a/internal/consensus/byzantine_test.go b/internal/consensus/byzantine_test.go index 7f39833de..2b94eff88 100644 --- a/internal/consensus/byzantine_test.go +++ b/internal/consensus/byzantine_test.go @@ -232,6 +232,7 @@ func (p *byzantinePrevoter) Do(ctx context.Context, stateData *StateData) error stateData.Round, stateData.state, true, + types.VerifiedCommit{}, ) require.NoError(p.t, err) assert.NotZero(p.t, uncommittedState) diff --git a/internal/consensus/replay_test.go b/internal/consensus/replay_test.go index db9a6dc7d..5e3d7aa8c 100644 --- a/internal/consensus/replay_test.go +++ b/internal/consensus/replay_test.go @@ -9,6 +9,7 @@ import ( "os" "runtime" "strings" + "sync" "testing" "time" @@ -839,7 +840,7 @@ func applyBlock( bps, err := blk.MakePartSet(testPartSize) require.NoError(t, err) blkID := blk.BlockID(bps) - newState, err := blockExec.ApplyBlock(ctx, st, blkID, blk, commit) + newState, err := blockExec.ApplyBlock(ctx, st, blkID, blk, commit, types.VerifiedCommit{}) require.NoError(t, err) return newState } @@ -1396,7 +1397,36 @@ func TestHandshakeInitialCoreLockHeight(t *testing.T) { assert.Equal(t, InitialCoreHeight, state.LastCoreChainLockedBlockHeight) } +// walRoundsSkipperProposeTimeout replaces the test genesis' 30ms propose +// timeout on the node that generates the WAL for testWALRoundsSkipper. That +// node is the only validator, so it proposes every round and its propose +// timeout can only race its own proposal: there is no absent proposer to wait +// out, and a round moves on as soon as the proposal is complete. At 30ms a +// loaded machine lets the timeout win round maxRound, which heights 3 to +// chainLen have to commit in. The override is node-local, not a consensus +// parameter, so no block changes. +const walRoundsSkipperProposeTimeout = 10 * time.Second + func TestWALRoundsSkipper(t *testing.T) { + testWALRoundsSkipper(t, false) +} + +// TestWALRoundsSkipperSlowProposer delays the node's own proposal at round +// maxRound past the propose timeout the test genesis sets for that round, as a +// loaded CI runner does. The WAL generator must still commit every height at +// round maxRound. +func TestWALRoundsSkipperSlowProposer(t *testing.T) { + testWALRoundsSkipper(t, true) +} + +// testWALRoundsSkipper generates a WAL in which heights 3 to chainLen prevote +// nil for rounds 0 to maxRound-1 and commit at round maxRound, then checks that +// a node replaying it with WalSkipRoundsToLast carries on to the next height. +// With slowProposer, the generating node's PrepareProposal at round maxRound +// outlasts the test genesis' propose timeout. +func testWALRoundsSkipper(t *testing.T, slowProposer bool) { + // first, so that it is muted only after every later cleanup has run + logger := newTeardownSafeLogger(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() const ( @@ -1405,7 +1435,7 @@ func TestWALRoundsSkipper(t *testing.T) { ) cfg := getConfig(t) cfg.Consensus.WalSkipRoundsToLast = true - logger := log.NewTestingLogger(t) + cfg.Consensus.UnsafeProposeTimeoutOverride = walRoundsSkipperProposeTimeout ng := nodeGen{ cfg: cfg, logger: logger, @@ -1414,6 +1444,9 @@ func TestWALRoundsSkipper(t *testing.T) { stopConsensusAtHeight(chainLen, maxRound+1), )}, } + if slowProposer { + ng.app = newSlowProposerApp(t, cfg, maxRound) + } node := ng.Generate(ctx, t) withReplayPrevoter(node.csState) @@ -1462,7 +1495,7 @@ func TestWALRoundsSkipper(t *testing.T) { require.NoError(t, err) ctx = dash.ContextWithProTxHash(ctx, proTxHash) - cs := newStateWithConfigAndBlockStore(ctx, t, log.NewTestingLogger(t), cfg, state, privVal, app, blockStore) + cs := newStateWithConfigAndBlockStore(ctx, t, logger, cfg, state, privVal, app, blockStore) commit := blockStore.commits[len(blockStore.commits)-1] require.Equal(t, int64(4), commit.Height) @@ -1533,6 +1566,91 @@ func newBlockReplayer( ) } +// slowProposerApp is a kvstore application whose PrepareProposal takes delay +// longer at round, so a node proposing at that round completes its proposal +// only after delay has passed. +type slowProposerApp struct { + *kvstore.Application + round int32 + delay time.Duration +} + +// newSlowProposerApp returns a slowProposerApp whose delay at round is three +// times the propose timeout cfg's genesis sets for round, yet still well below +// walRoundsSkipperProposeTimeout. +func newSlowProposerApp(t *testing.T, cfg *config.Config, round int32) *slowProposerApp { + t.Helper() + genDoc, err := types.GenesisDocFromFile(cfg.GenesisFile()) + require.NoError(t, err) + require.NotNil(t, genDoc.ConsensusParams, "test genesis sets no consensus params") + delay := 3 * genDoc.ConsensusParams.Timeout.TimeoutParamsOrDefaults().ProposeTimeout(round) + require.Less(t, delay, walRoundsSkipperProposeTimeout/10, + "the delay must stay far below the propose timeout override") + + app, err := kvstore.NewMemoryApp() + require.NoError(t, err) + return &slowProposerApp{Application: app, round: round, delay: delay} +} + +func (app *slowProposerApp) PrepareProposal( + ctx context.Context, + req *abci.RequestPrepareProposal, +) (*abci.ResponsePrepareProposal, error) { + if req.Round == app.round { + select { + case <-time.After(app.delay): + case <-ctx.Done(): + return nil, ctx.Err() + } + } + return app.Application.PrepareProposal(ctx, req) +} + +// teardownSafeLogWriter writes to t's log until it is muted and discards +// everything after. A consensus State outlives its test: Stop and Wait return +// without waiting for its receiveRoutine, which exits only once it notices its +// context is done, nor for the goroutines that stop its WAL and other services +// on that context. Any of them logging after t has completed panics the whole +// test binary. +type teardownSafeLogWriter struct { + mtx sync.Mutex // held across t.Log, so muting waits out a write in flight + t testing.TB + muted bool +} + +func (w *teardownSafeLogWriter) Write(p []byte) (int, error) { + w.mtx.Lock() + defer w.mtx.Unlock() + if !w.muted { + w.t.Log(string(p)) + } + return len(p), nil +} + +func (w *teardownSafeLogWriter) mute() { + w.mtx.Lock() + defer w.mtx.Unlock() + w.muted = true +} + +// newTeardownSafeLogger returns a logger at NewTestingLogger's level that +// writes through a teardownSafeLogWriter muted by a cleanup of t. Cleanups run +// last-in first-out, so create it before anything else registers one: the +// writer is then muted only after every other cleanup has run, and nothing is +// logged once t has completed. +func newTeardownSafeLogger(t *testing.T) log.Logger { + t.Helper() + w := &teardownSafeLogWriter{t: t} + t.Cleanup(w.mute) + level := log.LogLevelError + if testing.Verbose() { + level = log.LogLevelDebug + } + logger, err := log.NewLogger(level, w) + require.NoError(t, err) + return logger +} + type replayPrevoter struct { voteSigner *voteSigner prevoter Prevoter diff --git a/internal/consensus/replayer.go b/internal/consensus/replayer.go index d2a32c258..300db0063 100644 --- a/internal/consensus/replayer.go +++ b/internal/consensus/replayer.go @@ -275,7 +275,9 @@ func (r *BlockReplayer) replayBlock( ) (sm.CurrentRoundState, error) { r.logger.Info("Replay: applying block", "height", height) // Extra check to ensure the app was not changed in a way it shouldn't have. - ucState, err := r.blockExec.ProcessProposal(ctx, block, commit.Round, state, false) + // The replayer holds no proof for block.LastCommit. + ucState, err := r.blockExec.ProcessProposal(ctx, block, commit.Round, state, false, + types.VerifiedCommit{}) if err != nil { return sm.CurrentRoundState{}, fmt.Errorf("blockReplayer process proposal: %w", err) } @@ -306,8 +308,10 @@ func (r *BlockReplayer) syncStateAt( meta := r.store.LoadBlockMeta(height) seenCommit := r.store.LoadSeenCommitAt(height) // Use stubs for both mempool and evidence pool since no transactions nor - // evidence are needed here - block already exists. - state, err := blockExec.ApplyBlock(ctx, state, meta.BlockID, block, seenCommit) + // evidence are needed here - block already exists. The replayer holds no + // proof for block.LastCommit, so it verifies every block in full. + state, err := blockExec.ApplyBlock(ctx, state, meta.BlockID, block, seenCommit, + types.VerifiedCommit{}) if err != nil { return sm.State{}, err } diff --git a/internal/consensus/state_prevoter_test.go b/internal/consensus/state_prevoter_test.go index efd8f7ee0..35b9d9c33 100644 --- a/internal/consensus/state_prevoter_test.go +++ b/internal/consensus/state_prevoter_test.go @@ -125,13 +125,13 @@ func (suite *PrevoterTestSuite) TestDo() { } if tc.mockProcessProposal { suite.mockExecutor. - On("ProcessProposal", ctx, mock.Anything, int32(0), mock.Anything, true). + On("ProcessProposal", ctx, mock.Anything, int32(0), mock.Anything, true, types.VerifiedCommit{}). Once(). Return(currState, tc.ppErr) } if tc.mockValidateBlock { suite.mockExecutor. - On("ValidateBlockWithRoundState", ctx, mock.Anything, mock.Anything, mock.Anything). + On("ValidateBlockWithRoundState", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Once(). Return(nil) } diff --git a/internal/state/execution.go b/internal/state/execution.go index 526b6152c..cb8d11660 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -47,15 +47,36 @@ type Executor interface { round int32, state State, verify bool, + lastCommit types.VerifiedCommit, ) (CurrentRoundState, error) - ValidateBlock(ctx context.Context, state State, block *types.Block) error + // ValidateBlock, ValidateBlockWithRoundState, FinalizeBlock, ApplyBlock and + // ProcessProposal (used only when verify is set) take lastCommit, + // block.LastCommit with the proof of its verification if + // the caller holds one. The proof spares the threshold verification only + // when it covers exactly the verification the block's validation would run; + // anything else, including a VerifiedCommit without proof, is verified in + // full. block.LastCommit is always what is verified: the commit lastCommit + // holds is never read in its place. + ValidateBlock(ctx context.Context, state State, block *types.Block, lastCommit types.VerifiedCommit) error + + // VerifyCommit checks that commit is state.Validators' commit for the block + // blockID at height. On success it returns commit with the proof of that + // verification, which the caller can offer back as lastCommit when commit + // reappears as the next block's LastCommit. + VerifyCommit( + state State, + blockID types.BlockID, + height int64, + commit *types.Commit, + ) (types.VerifiedCommit, error) ValidateBlockWithRoundState( ctx context.Context, state State, uncommittedState CurrentRoundState, block *types.Block, + lastCommit types.VerifiedCommit, ) error FinalizeBlock( @@ -65,6 +86,7 @@ type Executor interface { blockID types.BlockID, block *types.Block, commit *types.Commit, + lastCommit types.VerifiedCommit, ) (State, error) ApplyBlock( @@ -73,6 +95,7 @@ type Executor interface { blockID types.BlockID, block *types.Block, commit *types.Commit, + lastCommit types.VerifiedCommit, ) (State, error) VerifyVoteExtension(ctx context.Context, vote *types.Vote) error @@ -159,7 +182,7 @@ func NewBlockExecutor( return blockExec } -// Copy returns a new instance of BlockExecutor and applies option functions +// Copy returns a new instance of BlockExecutor and applies option functions. func (blockExec *BlockExecutor) Copy(opts ...func(e *BlockExecutor)) *BlockExecutor { copied := &BlockExecutor{ eventPublisher: blockExec.eventPublisher, @@ -334,6 +357,7 @@ func (blockExec *BlockExecutor) ProcessProposal( round int32, state State, verify bool, + lastCommit types.VerifiedCommit, ) (CurrentRoundState, error) { version := block.Version.ToProto() stages := blockExec.metrics.startStages() @@ -394,7 +418,7 @@ func (blockExec *BlockExecutor) ProcessProposal( // Here we check if the ProcessProposal response matches // block received from proposer, eg. if `uncommittedState` // fields are the same as `block` fields - err = blockExec.ValidateBlockWithRoundState(ctx, state, stateChanges, block) + err = blockExec.ValidateBlockWithRoundState(ctx, state, stateChanges, block, lastCommit) if err != nil { return stateChanges, ErrInvalidBlock{err} } @@ -408,13 +432,18 @@ func (blockExec *BlockExecutor) ProcessProposal( // If the block is invalid, it returns an error. // Validation does not mutate state, but does require historical information from the stateDB, // ie. to verify evidence from a validator at an old height. -func (blockExec *BlockExecutor) ValidateBlock(ctx context.Context, state State, block *types.Block) error { +func (blockExec *BlockExecutor) ValidateBlock( + ctx context.Context, + state State, + block *types.Block, + lastCommit types.VerifiedCommit, +) error { hash := block.Hash() if _, ok := blockExec.cache[hash.String()]; ok { return nil } - err := validateBlock(state, block) + err := validateBlock(state, block, lastCommit, blockExec.metrics) if err != nil { return err } @@ -428,13 +457,27 @@ func (blockExec *BlockExecutor) ValidateBlock(ctx context.Context, state State, return nil } +// VerifyCommit verifies commit against state.Validators as the commit for the +// block blockID at height. On success it returns commit with the proof of that +// verification, which the caller offers back as lastCommit when the same commit +// comes back one height later as the next block's LastCommit. +func (blockExec *BlockExecutor) VerifyCommit( + state State, + blockID types.BlockID, + height int64, + commit *types.Commit, +) (types.VerifiedCommit, error) { + return types.VerifyCommitSignatures(state.Validators, state.ChainID, blockID, height, commit, nil) +} + func (blockExec *BlockExecutor) ValidateBlockWithRoundState( ctx context.Context, state State, uncommittedState CurrentRoundState, block *types.Block, + lastCommit types.VerifiedCommit, ) error { - err := blockExec.ValidateBlock(ctx, state, block) + err := blockExec.ValidateBlock(ctx, state, block, lastCommit) if err != nil { return err } @@ -456,10 +499,14 @@ func (blockExec *BlockExecutor) ValidateBlockWithRoundState( ) } + // ValidateBlock above normally verifies block.LastCommit, but it short-circuits + // on its per-height cache, which is keyed on the block hash alone and so says + // nothing about the state the earlier validation ran against. That is why this + // second verification exists. lastCommit's proof is safe to skip on instead: + // it names the verified data itself, not a block hash. if block.Height > state.InitialHeight { - if err := state.LastValidators.VerifyCommit( - state.ChainID, state.LastBlockID, block.Height-1, block.LastCommit); err != nil { - return fmt.Errorf("error validating block: %w", err) + if err := verifyLastCommit(state, block, lastCommit, blockExec.metrics); err != nil { + return err } } if !bytes.Equal(block.NextValidatorsHash, uncommittedState.NextValidators.Hash()) { @@ -493,6 +540,7 @@ func (blockExec *BlockExecutor) FinalizeBlock( blockID types.BlockID, block *types.Block, commit *types.Commit, + lastCommit types.VerifiedCommit, ) (State, error) { // This is the only ValidateBlockWithRoundState on the ApplyBlock path, so it // must not be removed: ApplyBlock deliberately calls ProcessProposal with @@ -504,7 +552,7 @@ func (blockExec *BlockExecutor) FinalizeBlock( // app by ProcessProposal and written by blockApplier.Apply; what this guards // is the state and ABCI responses written below. stages := blockExec.metrics.startStages() - if err := blockExec.ValidateBlockWithRoundState(ctx, state, uncommittedState, block); err != nil { + if err := blockExec.ValidateBlockWithRoundState(ctx, state, uncommittedState, block, lastCommit); err != nil { return state, ErrInvalidBlock{err} } stages.done("finalize_validate") @@ -579,16 +627,17 @@ func (blockExec *BlockExecutor) ApplyBlock( blockID types.BlockID, block *types.Block, commit *types.Commit, + lastCommit types.VerifiedCommit, ) (State, error) { // verify is false because FinalizeBlock below runs ValidateBlockWithRoundState // with the same arguments and wraps failures in the same ErrInvalidBlock. // Verifying here as well would validate every block twice, and each pass costs // a threshold signature verification of block.LastCommit. - uncommittedState, err := blockExec.ProcessProposal(ctx, block, commit.Round, state, false) + uncommittedState, err := blockExec.ProcessProposal(ctx, block, commit.Round, state, false, lastCommit) if err != nil { return state, err } - return blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, commit) + return blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, commit, lastCommit) } // ExtendVote gets vote-extensions from ABCI and updates vote.VoteExtensions with this value diff --git a/internal/state/execution_test.go b/internal/state/execution_test.go index e7477d62e..0313bebd4 100644 --- a/internal/state/execution_test.go +++ b/internal/state/execution_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "slices" "testing" "time" @@ -30,6 +31,7 @@ import ( sf "github.com/dashpay/tenderdash/internal/state/test/factory" "github.com/dashpay/tenderdash/internal/store" "github.com/dashpay/tenderdash/internal/test/factory" + "github.com/dashpay/tenderdash/internal/test/metricspy" "github.com/dashpay/tenderdash/libs/log" "github.com/dashpay/tenderdash/libs/rand" tmtypes "github.com/dashpay/tenderdash/proto/tendermint/types" @@ -94,7 +96,7 @@ func TestApplyBlock(t *testing.T) { require.NoError(t, err) blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: bps.Header()} - state, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit)) + state, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit), types.VerifiedCommit{}) require.NoError(t, err) // State for next block @@ -190,7 +192,7 @@ func TestFinalizeBlockByzantineValidators(t *testing.T) { blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: bps.Header()} - _, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit)) + _, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit), types.VerifiedCommit{}) require.NoError(t, err) // TODO check state and mempool @@ -274,7 +276,7 @@ func TestProcessProposal(t *testing.T) { TxResults: txResults, Status: abci.ResponseProcessProposal_ACCEPT, }, nil) - uncommittedState, err := blockExec.ProcessProposal(ctx, block1, round, state, true) + uncommittedState, err := blockExec.ProcessProposal(ctx, block1, round, state, true, types.VerifiedCommit{}) require.NoError(t, err) assert.NotZero(t, uncommittedState) app.AssertExpectations(t) @@ -344,7 +346,7 @@ func TestUpdateConsensusParams(t *testing.T) { Status: abci.ResponseProcessProposal_ACCEPT, ConsensusParamUpdates: &tmtypes.ConsensusParams{Block: &tmtypes.BlockParams{MaxBytes: 1024 * 1024}}, }, nil).Once() - uncommittedState, err := blockExec.ProcessProposal(ctx, block1, round, state, true) + uncommittedState, err := blockExec.ProcessProposal(ctx, block1, round, state, true, types.VerifiedCommit{}) require.NoError(t, err) assert.Equal(t, block1.NextConsensusHash, uncommittedState.NextConsensusParams.HashConsensusParams()) @@ -421,7 +423,7 @@ func TestOverrideAppVersion(t *testing.T) { Status: abci.ResponseProcessProposal_ACCEPT, }, nil).Once() - _, err = blockExec.ProcessProposal(ctx, block1, round, state, true) + _, err = blockExec.ProcessProposal(ctx, block1, round, state, true, types.VerifiedCommit{}) require.NoError(t, err) assert.EqualValues(t, appVersion, block1.Version.App, "App version should be overridden by PrepareProposal") @@ -686,7 +688,7 @@ func TestFinalizeBlockValidatorUpdates(t *testing.T) { require.NoError(t, err) blockID := block.BlockID(nil) require.NoError(t, err) - state, err = blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, new(types.Commit)) + state, err = blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, new(types.Commit), types.VerifiedCommit{}) require.NoError(t, err) require.Nil(t, err) @@ -768,7 +770,7 @@ func TestFinalizeBlockValidatorUpdatesResultingInEmptySet(t *testing.T) { } assert.NotPanics(t, func() { - state, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit)) + state, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit), types.VerifiedCommit{}) }) assert.NotNil(t, err) assert.NotEmpty(t, state.Validators.Validators) @@ -1358,7 +1360,7 @@ func TestApplyBlockValidatesBlock(t *testing.T) { require.NoError(t, err) blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: bps.Header()} - _, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit)) + _, err = blockExec.ApplyBlock(ctx, state, blockID, block, new(types.Commit), types.VerifiedCommit{}) require.Error(t, err, "ApplyBlock must reject a block that fails validation") require.ErrorAs(t, err, &sm.ErrInvalidBlock{}) @@ -1368,3 +1370,236 @@ func TestApplyBlockValidatesBlock(t *testing.T) { require.Equal(t, state.LastBlockHeight, loaded.LastBlockHeight, "an invalid block must not advance the state store") } + +// badCommitSignature is what ValidatorSet.VerifyCommit reports for a forged threshold +// signature. ErrInvalidBlock does not unwrap, so tests match it by message. +const badCommitSignature = "invalid commit signatures for quorum" + +// verifiedCommitFixture drives a real BlockExecutor through height 1 so height 2 +// carries a genuine BLS LastCommit, then returns everything a test of the +// commit verification flow needs. +type verifiedCommitFixture struct { + ctx context.Context + blockExec *sm.BlockExecutor + // skipped counts the LastCommit verifications the executor skipped + skipped *metricspy.Counter + // state after height 1 is applied; the state height 2 validates against + state sm.State + // block ID and commit for height 1, as block sync would hand to the applier + blockID types.BlockID + commit *types.Commit + // height 2 block whose LastCommit is commit + block *types.Block + // state height 1 was verified against (Validators is the signing set) + verifiedAgainst sm.State + privVals map[string]types.PrivValidator +} + +func newVerifiedCommitFixture(t *testing.T) verifiedCommitFixture { + t.Helper() + app := &testApp{} + logger := log.NewNopLogger() + proxyApp := proxy.New(abciclient.NewLocalClient(logger, app), logger, proxy.NopMetrics()) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + require.NoError(t, proxyApp.Start(ctx)) + + eventBus := eventbus.NewDefault(logger) + require.NoError(t, eventBus.Start(ctx)) + + state, stateDB, privVals := makeState(t, 2, 1) + stateStore := sm.NewStore(stateDB) + ctx = dash.ContextWithProTxHash(ctx, state.Validators.Validators[0].ProTxHash) + app.ValidatorSetUpdate = state.Validators.ABCIEquivalentValidatorUpdates() + + mp := &mpmocks.Mempool{} + mp.On("Lock").Return() + mp.On("Unlock").Return() + mp.On("FlushAppConn", mock.Anything).Return(nil) + mp.On("Update", mock.Anything, mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + skipped := metricspy.NewCounter() + execMetrics := sm.NopMetrics() + execMetrics.LastCommitVerificationSkipped = skipped + blockExec := sm.NewBlockExecutor(stateStore, proxyApp, mp, sm.EmptyEvidencePool{}, + store.NewBlockStore(dbm.NewMemDB()), eventBus, sm.BockExecWithMetrics(execMetrics)) + + genesis := state + proposer := state.GetProposerFromState(1, 0).ProTxHash + // makeAndCommitGoodBlock validates through this executor and populates its + // per-height cache; FinalizeBlock resets it, so height 2 validates for real. + state, blockID, commit := makeAndCommitGoodBlock(ctx, t, state, 1, new(types.Commit), proposer, blockExec, privVals, nil, 0) + + f := verifiedCommitFixture{ + ctx: ctx, + blockExec: blockExec, + skipped: skipped, + state: state, + blockID: blockID, + commit: commit, + verifiedAgainst: genesis, + privVals: privVals, + } + f.block = f.blockWith(commit) + return f +} + +// verify runs commit through the executor as block sync does when it applies +// the height 1 block, and returns the verification the executor hands back. +func (f verifiedCommitFixture) verify(commit *types.Commit) (types.VerifiedCommit, error) { + return f.blockExec.VerifyCommit(f.verifiedAgainst, f.blockID, 1, commit) +} + +// blockWith builds the height 2 block carrying lastCommit, ready for ApplyBlock. +func (f verifiedCommitFixture) blockWith(lastCommit *types.Commit) *types.Block { + proposer := f.state.GetProposerFromState(2, 0).ProTxHash + block := f.state.MakeBlock(2, factory.MakeNTxs(2, 10), lastCommit, nil, proposer, 0) + block.ResultsHash, _ = abci.TxResultsHash(factory.ExecTxResults(block.Txs)) + return block +} + +// applyNext runs the height 2 block carrying lastCommit through ApplyBlock, +// offering lastCommitVerified as the verification of that LastCommit, and +// returns the resulting error. +func (f verifiedCommitFixture) applyNext( + t *testing.T, + lastCommit *types.Commit, + lastCommitVerified types.VerifiedCommit, +) error { + t.Helper() + block := f.blockWith(lastCommit) + bps, err := block.MakePartSet(testPartSize) + require.NoError(t, err) + _, err = f.blockExec.ApplyBlock(f.ctx, f.state, block.BlockID(bps), block, new(types.Commit), lastCommitVerified) + return err +} + +// genuineCommit signs a commit for an unrelated block at height with the +// fixture's validators: one that passes verification but is not the commit +// under test. +func (f verifiedCommitFixture) genuineCommit(t *testing.T, height int64) (types.BlockID, *types.Commit) { + t.Helper() + blockID := factory.MakeBlockID() + commit, _ := makeValidCommit(f.ctx, t, height, blockID, f.verifiedAgainst.Validators, f.privVals) + return blockID, commit +} + +// cloneCommit returns a commit with the same fields and no cached hash, so a +// test can mutate it without touching the fixture's copy. +func cloneCommit(c *types.Commit) *types.Commit { + return &types.Commit{ + Height: c.Height, + Round: c.Round, + BlockID: c.BlockID.Copy(), + QuorumHash: bytes.Clone(c.QuorumHash), + ThresholdBlockSignature: bytes.Clone(c.ThresholdBlockSignature), + ThresholdVoteExtensions: slices.Clone(c.ThresholdVoteExtensions), + } +} + +// forgedCommit returns a copy of commit with a corrupted threshold signature. +func forgedCommit(commit *types.Commit) *types.Commit { + c := cloneCommit(commit) + c.ThresholdBlockSignature[0] ^= 0xff + return c +} + +// TestVerifyCommitReturnsVerification checks that the executor hands back a +// verification only for a commit it accepted, and that validating the next +// block skips the LastCommit threshold verification only when the verification +// it is offered covers that exact commit. What "exactly" means input by input +// is pinned in the types package; this checks the executor honors it. +func TestVerifyCommitReturnsVerification(t *testing.T) { + t.Run("state without validators cannot verify", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + noVals := f.verifiedAgainst.Copy() + noVals.Validators = nil + verified, err := f.blockExec.VerifyCommit(noVals, f.blockID, 1, f.commit) + require.Error(t, err) + require.Equal(t, types.VerifiedCommit{}, verified) + }) + + t.Run("rejected commit yields no verification", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + verified, err := f.verify(forgedCommit(f.commit)) + require.ErrorContains(t, err, badCommitSignature) + require.Equal(t, types.VerifiedCommit{}, verified) + }) + + t.Run("covering verification skips the LastCommit check", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + verified, err := f.verify(f.commit) + require.NoError(t, err) + // verified against height 1's Validators, validated against height 2's + // LastValidators: the same quorum held by a different object + require.NotSame(t, f.verifiedAgainst.Validators, f.state.LastValidators) + require.NoError(t, f.blockExec.ValidateBlock(f.ctx, f.state, f.block, verified)) + require.Equal(t, 1.0, f.skipped.Value()) + }) + + t.Run("zero verification verifies in full", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + require.NoError(t, f.blockExec.ValidateBlock(f.ctx, f.state, f.block, types.VerifiedCommit{})) + require.Equal(t, 0.0, f.skipped.Value(), "nothing was verified, so nothing may be skipped") + }) + + t.Run("verification of another commit verifies in full", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + otherBlockID, otherCommit := f.genuineCommit(t, 1) + other, err := f.blockExec.VerifyCommit(f.verifiedAgainst, otherBlockID, 1, otherCommit) + require.NoError(t, err) + require.NoError(t, f.blockExec.ValidateBlock(f.ctx, f.state, f.block, other)) + require.Equal(t, 0.0, f.skipped.Value()) + }) + + t.Run("verification does not cover a forged LastCommit", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + verified, err := f.verify(f.commit) + require.NoError(t, err) + err = f.blockExec.ValidateBlock(f.ctx, f.state, f.blockWith(forgedCommit(f.commit)), verified) + require.ErrorContains(t, err, badCommitSignature) + require.Equal(t, 0.0, f.skipped.Value()) + }) +} + +// TestApplyBlockSkipsVerifiedLastCommit checks the ApplyBlock flow: with the +// verification of the commit it just verified, ApplyBlock for the next block +// does not threshold-verify that commit again, while a LastCommit the +// verification does not cover is verified — and rejected when forged. Block +// sync does not call ApplyBlock; TestBlockApplierSkipsTheLastCommitItVerified +// pins its flow. +func TestApplyBlockSkipsVerifiedLastCommit(t *testing.T) { + t.Run("verified commit is not re-verified", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + verified, err := f.verify(f.commit) + require.NoError(t, err) + require.NoError(t, f.applyNext(t, f.commit, verified)) + // once in validateBlock and once in ValidateBlockWithRoundState: the two + // threshold verifications ApplyBlock would otherwise run on LastCommit + require.Equal(t, 2.0, f.skipped.Value(), + "a commit the verification covers must not be verified again") + }) + + t.Run("genuine commit passes without a verification", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + require.NoError(t, f.applyNext(t, f.commit, types.VerifiedCommit{})) + require.Equal(t, 0.0, f.skipped.Value(), "nothing was verified, so nothing may be skipped") + }) + + t.Run("unverified forged commit is rejected", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + err := f.applyNext(t, forgedCommit(f.commit), types.VerifiedCommit{}) + require.ErrorAs(t, err, &sm.ErrInvalidBlock{}) + require.ErrorContains(t, err, badCommitSignature) + require.Equal(t, 0.0, f.skipped.Value()) + }) + + t.Run("verification of the genuine commit does not cover a forged one", func(t *testing.T) { + f := newVerifiedCommitFixture(t) + verified, err := f.verify(f.commit) + require.NoError(t, err) + require.ErrorContains(t, f.applyNext(t, forgedCommit(f.commit), verified), badCommitSignature) + require.Equal(t, 0.0, f.skipped.Value()) + }) +} diff --git a/internal/state/helpers_test.go b/internal/state/helpers_test.go index 37082794e..d8978474d 100644 --- a/internal/state/helpers_test.go +++ b/internal/state/helpers_test.go @@ -47,16 +47,16 @@ func makeAndCommitGoodBlock( // A good block passes state, blockID, block := makeAndApplyGoodBlock(t, state, height, lastCommit, proposerProTxHash, evidence, proposedAppVersion) - require.NoError(t, blockExec.ValidateBlock(ctx, state, block)) + require.NoError(t, blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{})) txResults := factory.ExecTxResults(block.Txs) block.ResultsHash, err = abci.TxResultsHash(txResults) require.NoError(t, err) - uncommittedState, err := blockExec.ProcessProposal(ctx, block, 0, state, true) + uncommittedState, err := blockExec.ProcessProposal(ctx, block, 0, state, true, types.VerifiedCommit{}) require.NoError(t, err) // Simulate a lastCommit for this block from all validators for the next height commit, _ := makeValidCommit(ctx, t, height, blockID, state.Validators, privVals) - state, err = blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, commit) + state, err = blockExec.FinalizeBlock(ctx, state, uncommittedState, blockID, block, commit, types.VerifiedCommit{}) require.NoError(t, err) return state, blockID, commit diff --git a/internal/state/metrics.gen.go b/internal/state/metrics.gen.go index eaf56ab6f..121dfc1b5 100644 --- a/internal/state/metrics.gen.go +++ b/internal/state/metrics.gen.go @@ -34,6 +34,12 @@ func PrometheusMetrics(namespace string, labelsAndValues ...string) *Metrics { Name: "validator_set_updates", Help: "Number of validator set updates returned by the application since process start.", }, labels).With(labelsAndValues...), + LastCommitVerificationSkipped: prometheus.NewCounterFrom(stdprometheus.CounterOpts{ + Namespace: namespace, + Subsystem: MetricsSubsystem, + Name: "last_commit_verification_skipped", + Help: "Number of LastCommit threshold verifications skipped because the same commit was already verified.", + }, labels).With(labelsAndValues...), BlockApplyStageDuration: prometheus.NewHistogramFrom(stdprometheus.HistogramOpts{ Namespace: namespace, Subsystem: MetricsSubsystem, @@ -47,9 +53,10 @@ func PrometheusMetrics(namespace string, labelsAndValues ...string) *Metrics { func NopMetrics() *Metrics { return &Metrics{ - BlockProcessingTime: discard.NewHistogram(), - ConsensusParamUpdates: discard.NewCounter(), - ValidatorSetUpdates: discard.NewCounter(), - BlockApplyStageDuration: discard.NewHistogram(), + BlockProcessingTime: discard.NewHistogram(), + ConsensusParamUpdates: discard.NewCounter(), + ValidatorSetUpdates: discard.NewCounter(), + LastCommitVerificationSkipped: discard.NewCounter(), + BlockApplyStageDuration: discard.NewHistogram(), } } diff --git a/internal/state/metrics.go b/internal/state/metrics.go index 45db85de1..9ffcef8d4 100644 --- a/internal/state/metrics.go +++ b/internal/state/metrics.go @@ -29,6 +29,13 @@ type Metrics struct { //metrics:Number of validator set updates returned by the application since process start. ValidatorSetUpdates metrics.Counter + // LastCommitVerificationSkipped counts the LastCommit threshold verifications + // that were skipped because VerifyCommit had already verified the identical + // commit against the identical inputs. During block sync this should track + // the block rate; in consensus it stays flat. + //metrics:Number of LastCommit threshold verifications skipped because the same commit was already verified. + LastCommitVerificationSkipped metrics.Counter + // BlockApplyStageDuration is the wall-clock cost of each stage a committed // block goes through in ProcessProposal and FinalizeBlock. During block sync // those stages are most of a block; this says which one a slow sync is in. diff --git a/internal/state/mocks/executor.go b/internal/state/mocks/executor.go index 2d7c07605..74aed6073 100644 --- a/internal/state/mocks/executor.go +++ b/internal/state/mocks/executor.go @@ -40,8 +40,8 @@ func (_m *Executor) EXPECT() *Executor_Expecter { } // ApplyBlock provides a mock function for the type Executor -func (_mock *Executor) ApplyBlock(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit) (state.State, error) { - ret := _mock.Called(ctx, state1, blockID, block, commit) +func (_mock *Executor) ApplyBlock(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit) (state.State, error) { + ret := _mock.Called(ctx, state1, blockID, block, commit, lastCommit) if len(ret) == 0 { panic("no return value specified for ApplyBlock") @@ -49,16 +49,16 @@ func (_mock *Executor) ApplyBlock(ctx context.Context, state1 state.State, block var r0 state.State var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit) (state.State, error)); ok { - return returnFunc(ctx, state1, blockID, block, commit) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) (state.State, error)); ok { + return returnFunc(ctx, state1, blockID, block, commit, lastCommit) } - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit) state.State); ok { - r0 = returnFunc(ctx, state1, blockID, block, commit) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) state.State); ok { + r0 = returnFunc(ctx, state1, blockID, block, commit, lastCommit) } else { r0 = ret.Get(0).(state.State) } - if returnFunc, ok := ret.Get(1).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit) error); ok { - r1 = returnFunc(ctx, state1, blockID, block, commit) + if returnFunc, ok := ret.Get(1).(func(context.Context, state.State, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) error); ok { + r1 = returnFunc(ctx, state1, blockID, block, commit, lastCommit) } else { r1 = ret.Error(1) } @@ -76,11 +76,12 @@ type Executor_ApplyBlock_Call struct { // - blockID types.BlockID // - block *types.Block // - commit *types.Commit -func (_e *Executor_Expecter) ApplyBlock(ctx any, state1 any, blockID any, block any, commit any) *Executor_ApplyBlock_Call { - return &Executor_ApplyBlock_Call{Call: _e.mock.On("ApplyBlock", ctx, state1, blockID, block, commit)} +// - lastCommit types.VerifiedCommit +func (_e *Executor_Expecter) ApplyBlock(ctx any, state1 any, blockID any, block any, commit any, lastCommit any) *Executor_ApplyBlock_Call { + return &Executor_ApplyBlock_Call{Call: _e.mock.On("ApplyBlock", ctx, state1, blockID, block, commit, lastCommit)} } -func (_c *Executor_ApplyBlock_Call) Run(run func(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit)) *Executor_ApplyBlock_Call { +func (_c *Executor_ApplyBlock_Call) Run(run func(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit)) *Executor_ApplyBlock_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -102,12 +103,17 @@ func (_c *Executor_ApplyBlock_Call) Run(run func(ctx context.Context, state1 sta if args[4] != nil { arg4 = args[4].(*types.Commit) } + var arg5 types.VerifiedCommit + if args[5] != nil { + arg5 = args[5].(types.VerifiedCommit) + } run( arg0, arg1, arg2, arg3, arg4, + arg5, ) }) return _c @@ -118,7 +124,7 @@ func (_c *Executor_ApplyBlock_Call) Return(state11 state.State, err error) *Exec return _c } -func (_c *Executor_ApplyBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit) (state.State, error)) *Executor_ApplyBlock_Call { +func (_c *Executor_ApplyBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit) (state.State, error)) *Executor_ApplyBlock_Call { _c.Call.Return(run) return _c } @@ -274,8 +280,8 @@ func (_c *Executor_ExtendVote_Call) RunAndReturn(run func(ctx context.Context, v } // FinalizeBlock provides a mock function for the type Executor -func (_mock *Executor) FinalizeBlock(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit) (state.State, error) { - ret := _mock.Called(ctx, state1, uncommittedState, blockID, block, commit) +func (_mock *Executor) FinalizeBlock(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit) (state.State, error) { + ret := _mock.Called(ctx, state1, uncommittedState, blockID, block, commit, lastCommit) if len(ret) == 0 { panic("no return value specified for FinalizeBlock") @@ -283,16 +289,16 @@ func (_mock *Executor) FinalizeBlock(ctx context.Context, state1 state.State, un var r0 state.State var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit) (state.State, error)); ok { - return returnFunc(ctx, state1, uncommittedState, blockID, block, commit) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) (state.State, error)); ok { + return returnFunc(ctx, state1, uncommittedState, blockID, block, commit, lastCommit) } - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit) state.State); ok { - r0 = returnFunc(ctx, state1, uncommittedState, blockID, block, commit) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) state.State); ok { + r0 = returnFunc(ctx, state1, uncommittedState, blockID, block, commit, lastCommit) } else { r0 = ret.Get(0).(state.State) } - if returnFunc, ok := ret.Get(1).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit) error); ok { - r1 = returnFunc(ctx, state1, uncommittedState, blockID, block, commit) + if returnFunc, ok := ret.Get(1).(func(context.Context, state.State, state.CurrentRoundState, types.BlockID, *types.Block, *types.Commit, types.VerifiedCommit) error); ok { + r1 = returnFunc(ctx, state1, uncommittedState, blockID, block, commit, lastCommit) } else { r1 = ret.Error(1) } @@ -311,11 +317,12 @@ type Executor_FinalizeBlock_Call struct { // - blockID types.BlockID // - block *types.Block // - commit *types.Commit -func (_e *Executor_Expecter) FinalizeBlock(ctx any, state1 any, uncommittedState any, blockID any, block any, commit any) *Executor_FinalizeBlock_Call { - return &Executor_FinalizeBlock_Call{Call: _e.mock.On("FinalizeBlock", ctx, state1, uncommittedState, blockID, block, commit)} +// - lastCommit types.VerifiedCommit +func (_e *Executor_Expecter) FinalizeBlock(ctx any, state1 any, uncommittedState any, blockID any, block any, commit any, lastCommit any) *Executor_FinalizeBlock_Call { + return &Executor_FinalizeBlock_Call{Call: _e.mock.On("FinalizeBlock", ctx, state1, uncommittedState, blockID, block, commit, lastCommit)} } -func (_c *Executor_FinalizeBlock_Call) Run(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit)) *Executor_FinalizeBlock_Call { +func (_c *Executor_FinalizeBlock_Call) Run(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit)) *Executor_FinalizeBlock_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -341,6 +348,10 @@ func (_c *Executor_FinalizeBlock_Call) Run(run func(ctx context.Context, state1 if args[5] != nil { arg5 = args[5].(*types.Commit) } + var arg6 types.VerifiedCommit + if args[6] != nil { + arg6 = args[6].(types.VerifiedCommit) + } run( arg0, arg1, @@ -348,6 +359,7 @@ func (_c *Executor_FinalizeBlock_Call) Run(run func(ctx context.Context, state1 arg3, arg4, arg5, + arg6, ) }) return _c @@ -358,14 +370,14 @@ func (_c *Executor_FinalizeBlock_Call) Return(state11 state.State, err error) *E return _c } -func (_c *Executor_FinalizeBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit) (state.State, error)) *Executor_FinalizeBlock_Call { +func (_c *Executor_FinalizeBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, blockID types.BlockID, block *types.Block, commit *types.Commit, lastCommit types.VerifiedCommit) (state.State, error)) *Executor_FinalizeBlock_Call { _c.Call.Return(run) return _c } // ProcessProposal provides a mock function for the type Executor -func (_mock *Executor) ProcessProposal(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool) (state.CurrentRoundState, error) { - ret := _mock.Called(ctx, block, round, state1, verify) +func (_mock *Executor) ProcessProposal(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool, lastCommit types.VerifiedCommit) (state.CurrentRoundState, error) { + ret := _mock.Called(ctx, block, round, state1, verify, lastCommit) if len(ret) == 0 { panic("no return value specified for ProcessProposal") @@ -373,16 +385,16 @@ func (_mock *Executor) ProcessProposal(ctx context.Context, block *types.Block, var r0 state.CurrentRoundState var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, *types.Block, int32, state.State, bool) (state.CurrentRoundState, error)); ok { - return returnFunc(ctx, block, round, state1, verify) + if returnFunc, ok := ret.Get(0).(func(context.Context, *types.Block, int32, state.State, bool, types.VerifiedCommit) (state.CurrentRoundState, error)); ok { + return returnFunc(ctx, block, round, state1, verify, lastCommit) } - if returnFunc, ok := ret.Get(0).(func(context.Context, *types.Block, int32, state.State, bool) state.CurrentRoundState); ok { - r0 = returnFunc(ctx, block, round, state1, verify) + if returnFunc, ok := ret.Get(0).(func(context.Context, *types.Block, int32, state.State, bool, types.VerifiedCommit) state.CurrentRoundState); ok { + r0 = returnFunc(ctx, block, round, state1, verify, lastCommit) } else { r0 = ret.Get(0).(state.CurrentRoundState) } - if returnFunc, ok := ret.Get(1).(func(context.Context, *types.Block, int32, state.State, bool) error); ok { - r1 = returnFunc(ctx, block, round, state1, verify) + if returnFunc, ok := ret.Get(1).(func(context.Context, *types.Block, int32, state.State, bool, types.VerifiedCommit) error); ok { + r1 = returnFunc(ctx, block, round, state1, verify, lastCommit) } else { r1 = ret.Error(1) } @@ -400,11 +412,12 @@ type Executor_ProcessProposal_Call struct { // - round int32 // - state1 state.State // - verify bool -func (_e *Executor_Expecter) ProcessProposal(ctx any, block any, round any, state1 any, verify any) *Executor_ProcessProposal_Call { - return &Executor_ProcessProposal_Call{Call: _e.mock.On("ProcessProposal", ctx, block, round, state1, verify)} +// - lastCommit types.VerifiedCommit +func (_e *Executor_Expecter) ProcessProposal(ctx any, block any, round any, state1 any, verify any, lastCommit any) *Executor_ProcessProposal_Call { + return &Executor_ProcessProposal_Call{Call: _e.mock.On("ProcessProposal", ctx, block, round, state1, verify, lastCommit)} } -func (_c *Executor_ProcessProposal_Call) Run(run func(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool)) *Executor_ProcessProposal_Call { +func (_c *Executor_ProcessProposal_Call) Run(run func(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool, lastCommit types.VerifiedCommit)) *Executor_ProcessProposal_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -426,12 +439,17 @@ func (_c *Executor_ProcessProposal_Call) Run(run func(ctx context.Context, block if args[4] != nil { arg4 = args[4].(bool) } + var arg5 types.VerifiedCommit + if args[5] != nil { + arg5 = args[5].(types.VerifiedCommit) + } run( arg0, arg1, arg2, arg3, arg4, + arg5, ) }) return _c @@ -442,22 +460,22 @@ func (_c *Executor_ProcessProposal_Call) Return(currentRoundState state.CurrentR return _c } -func (_c *Executor_ProcessProposal_Call) RunAndReturn(run func(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool) (state.CurrentRoundState, error)) *Executor_ProcessProposal_Call { +func (_c *Executor_ProcessProposal_Call) RunAndReturn(run func(ctx context.Context, block *types.Block, round int32, state1 state.State, verify bool, lastCommit types.VerifiedCommit) (state.CurrentRoundState, error)) *Executor_ProcessProposal_Call { _c.Call.Return(run) return _c } // ValidateBlock provides a mock function for the type Executor -func (_mock *Executor) ValidateBlock(ctx context.Context, state1 state.State, block *types.Block) error { - ret := _mock.Called(ctx, state1, block) +func (_mock *Executor) ValidateBlock(ctx context.Context, state1 state.State, block *types.Block, lastCommit types.VerifiedCommit) error { + ret := _mock.Called(ctx, state1, block, lastCommit) if len(ret) == 0 { panic("no return value specified for ValidateBlock") } var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, *types.Block) error); ok { - r0 = returnFunc(ctx, state1, block) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, *types.Block, types.VerifiedCommit) error); ok { + r0 = returnFunc(ctx, state1, block, lastCommit) } else { r0 = ret.Error(0) } @@ -473,11 +491,12 @@ type Executor_ValidateBlock_Call struct { // - ctx context.Context // - state1 state.State // - block *types.Block -func (_e *Executor_Expecter) ValidateBlock(ctx any, state1 any, block any) *Executor_ValidateBlock_Call { - return &Executor_ValidateBlock_Call{Call: _e.mock.On("ValidateBlock", ctx, state1, block)} +// - lastCommit types.VerifiedCommit +func (_e *Executor_Expecter) ValidateBlock(ctx any, state1 any, block any, lastCommit any) *Executor_ValidateBlock_Call { + return &Executor_ValidateBlock_Call{Call: _e.mock.On("ValidateBlock", ctx, state1, block, lastCommit)} } -func (_c *Executor_ValidateBlock_Call) Run(run func(ctx context.Context, state1 state.State, block *types.Block)) *Executor_ValidateBlock_Call { +func (_c *Executor_ValidateBlock_Call) Run(run func(ctx context.Context, state1 state.State, block *types.Block, lastCommit types.VerifiedCommit)) *Executor_ValidateBlock_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -491,10 +510,15 @@ func (_c *Executor_ValidateBlock_Call) Run(run func(ctx context.Context, state1 if args[2] != nil { arg2 = args[2].(*types.Block) } + var arg3 types.VerifiedCommit + if args[3] != nil { + arg3 = args[3].(types.VerifiedCommit) + } run( arg0, arg1, arg2, + arg3, ) }) return _c @@ -505,22 +529,22 @@ func (_c *Executor_ValidateBlock_Call) Return(err error) *Executor_ValidateBlock return _c } -func (_c *Executor_ValidateBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, block *types.Block) error) *Executor_ValidateBlock_Call { +func (_c *Executor_ValidateBlock_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, block *types.Block, lastCommit types.VerifiedCommit) error) *Executor_ValidateBlock_Call { _c.Call.Return(run) return _c } // ValidateBlockWithRoundState provides a mock function for the type Executor -func (_mock *Executor) ValidateBlockWithRoundState(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block) error { - ret := _mock.Called(ctx, state1, uncommittedState, block) +func (_mock *Executor) ValidateBlockWithRoundState(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block, lastCommit types.VerifiedCommit) error { + ret := _mock.Called(ctx, state1, uncommittedState, block, lastCommit) if len(ret) == 0 { panic("no return value specified for ValidateBlockWithRoundState") } var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, *types.Block) error); ok { - r0 = returnFunc(ctx, state1, uncommittedState, block) + if returnFunc, ok := ret.Get(0).(func(context.Context, state.State, state.CurrentRoundState, *types.Block, types.VerifiedCommit) error); ok { + r0 = returnFunc(ctx, state1, uncommittedState, block, lastCommit) } else { r0 = ret.Error(0) } @@ -537,11 +561,12 @@ type Executor_ValidateBlockWithRoundState_Call struct { // - state1 state.State // - uncommittedState state.CurrentRoundState // - block *types.Block -func (_e *Executor_Expecter) ValidateBlockWithRoundState(ctx any, state1 any, uncommittedState any, block any) *Executor_ValidateBlockWithRoundState_Call { - return &Executor_ValidateBlockWithRoundState_Call{Call: _e.mock.On("ValidateBlockWithRoundState", ctx, state1, uncommittedState, block)} +// - lastCommit types.VerifiedCommit +func (_e *Executor_Expecter) ValidateBlockWithRoundState(ctx any, state1 any, uncommittedState any, block any, lastCommit any) *Executor_ValidateBlockWithRoundState_Call { + return &Executor_ValidateBlockWithRoundState_Call{Call: _e.mock.On("ValidateBlockWithRoundState", ctx, state1, uncommittedState, block, lastCommit)} } -func (_c *Executor_ValidateBlockWithRoundState_Call) Run(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block)) *Executor_ValidateBlockWithRoundState_Call { +func (_c *Executor_ValidateBlockWithRoundState_Call) Run(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block, lastCommit types.VerifiedCommit)) *Executor_ValidateBlockWithRoundState_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -559,11 +584,16 @@ func (_c *Executor_ValidateBlockWithRoundState_Call) Run(run func(ctx context.Co if args[3] != nil { arg3 = args[3].(*types.Block) } + var arg4 types.VerifiedCommit + if args[4] != nil { + arg4 = args[4].(types.VerifiedCommit) + } run( arg0, arg1, arg2, arg3, + arg4, ) }) return _c @@ -574,7 +604,85 @@ func (_c *Executor_ValidateBlockWithRoundState_Call) Return(err error) *Executor return _c } -func (_c *Executor_ValidateBlockWithRoundState_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block) error) *Executor_ValidateBlockWithRoundState_Call { +func (_c *Executor_ValidateBlockWithRoundState_Call) RunAndReturn(run func(ctx context.Context, state1 state.State, uncommittedState state.CurrentRoundState, block *types.Block, lastCommit types.VerifiedCommit) error) *Executor_ValidateBlockWithRoundState_Call { + _c.Call.Return(run) + return _c +} + +// VerifyCommit provides a mock function for the type Executor +func (_mock *Executor) VerifyCommit(state1 state.State, blockID types.BlockID, height int64, commit *types.Commit) (types.VerifiedCommit, error) { + ret := _mock.Called(state1, blockID, height, commit) + + if len(ret) == 0 { + panic("no return value specified for VerifyCommit") + } + + var r0 types.VerifiedCommit + var r1 error + if returnFunc, ok := ret.Get(0).(func(state.State, types.BlockID, int64, *types.Commit) (types.VerifiedCommit, error)); ok { + return returnFunc(state1, blockID, height, commit) + } + if returnFunc, ok := ret.Get(0).(func(state.State, types.BlockID, int64, *types.Commit) types.VerifiedCommit); ok { + r0 = returnFunc(state1, blockID, height, commit) + } else { + r0 = ret.Get(0).(types.VerifiedCommit) + } + if returnFunc, ok := ret.Get(1).(func(state.State, types.BlockID, int64, *types.Commit) error); ok { + r1 = returnFunc(state1, blockID, height, commit) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// Executor_VerifyCommit_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'VerifyCommit' +type Executor_VerifyCommit_Call struct { + *mock.Call +} + +// VerifyCommit is a helper method to define mock.On call +// - state1 state.State +// - blockID types.BlockID +// - height int64 +// - commit *types.Commit +func (_e *Executor_Expecter) VerifyCommit(state1 any, blockID any, height any, commit any) *Executor_VerifyCommit_Call { + return &Executor_VerifyCommit_Call{Call: _e.mock.On("VerifyCommit", state1, blockID, height, commit)} +} + +func (_c *Executor_VerifyCommit_Call) Run(run func(state1 state.State, blockID types.BlockID, height int64, commit *types.Commit)) *Executor_VerifyCommit_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 state.State + if args[0] != nil { + arg0 = args[0].(state.State) + } + var arg1 types.BlockID + if args[1] != nil { + arg1 = args[1].(types.BlockID) + } + var arg2 int64 + if args[2] != nil { + arg2 = args[2].(int64) + } + var arg3 *types.Commit + if args[3] != nil { + arg3 = args[3].(*types.Commit) + } + run( + arg0, + arg1, + arg2, + arg3, + ) + }) + return _c +} + +func (_c *Executor_VerifyCommit_Call) Return(verifiedCommit types.VerifiedCommit, err error) *Executor_VerifyCommit_Call { + _c.Call.Return(verifiedCommit, err) + return _c +} + +func (_c *Executor_VerifyCommit_Call) RunAndReturn(run func(state1 state.State, blockID types.BlockID, height int64, commit *types.Commit) (types.VerifiedCommit, error)) *Executor_VerifyCommit_Call { _c.Call.Return(run) return _c } diff --git a/internal/state/validation.go b/internal/state/validation.go index 89914093c..a9bb3bc70 100644 --- a/internal/state/validation.go +++ b/internal/state/validation.go @@ -14,7 +14,10 @@ import ( //----------------------------------------------------- // Validate block -func validateBlock(state State, block *types.Block) error { +// validateBlock validates block against state. lastCommit carries the caller's +// proof of block.LastCommit's verification, if it holds one; see +// verifyLastCommit. +func validateBlock(state State, block *types.Block, lastCommit types.VerifiedCommit, metrics *Metrics) error { // Validate internal consistency. if err := block.ValidateBasic(); err != nil { return err @@ -79,14 +82,8 @@ func validateBlock(state State, block *types.Block) error { if len(block.LastCommit.ThresholdBlockSignature) != 0 { return errors.New("initial block can't have ThresholdBlockSignature set") } - } else { - // fmt.Printf("validating against state with lastBlockId %s lastStateId %s\n", state.LastBlockID.String(), - // state.LastStateID.String()) - // LastPrecommits.Signatures length is checked in VerifyCommit. - if err := state.LastValidators.VerifyCommit( - state.ChainID, state.LastBlockID, block.Height-1, block.LastCommit); err != nil { - return fmt.Errorf("error validating block: %w", err) - } + } else if err := verifyLastCommit(state, block, lastCommit, metrics); err != nil { + return err } // NOTE: We can't actually verify it's the right proposer because we don't @@ -136,6 +133,25 @@ func validateBlock(state State, block *types.Block) error { return nil } +// verifyLastCommit verifies block.LastCommit against state.LastValidators as +// the commit for state.LastBlockID. lastCommit's proof spares that BLS +// threshold verification only when it covers exactly this verification — same +// chain, height, block ID, quorum, threshold key and commit content — and each +// skip is counted in metrics. Anything else, including a VerifiedCommit without +// proof that callers holding none pass, is verified in full. block.LastCommit +// is what is verified; the commit lastCommit holds is never read. +func verifyLastCommit(state State, block *types.Block, lastCommit types.VerifiedCommit, metrics *Metrics) error { + skipped, err := state.LastValidators.VerifyCommitUnlessVerified( + state.ChainID, state.LastBlockID, block.Height-1, block.LastCommit, lastCommit) + if err != nil { + return fmt.Errorf("error validating block: %w", err) + } + if skipped { + metrics.LastCommitVerificationSkipped.Add(1) + } + return nil +} + // ValidateBlockChainLock validates the given block chain lock against the given state. // If the block is invalid, it returns an error. // Validation does not mutate state, but does require historical information from the stateDB, diff --git a/internal/state/validation_test.go b/internal/state/validation_test.go index b543f4a50..a82864c23 100644 --- a/internal/state/validation_test.go +++ b/internal/state/validation_test.go @@ -146,7 +146,7 @@ func TestValidateBlockHeader(t *testing.T) { tc.malleateBlock(block) - err = blockExec.ValidateBlockWithRoundState(ctx, state, changes, block) + err = blockExec.ValidateBlockWithRoundState(ctx, state, changes, block, types.VerifiedCommit{}) t.Logf("%s: %v", tc.name, err) require.Error(t, err, tc.name) }) @@ -163,7 +163,7 @@ func TestValidateBlockHeader(t *testing.T) { block, err := statefactory.MakeBlock(state, nextHeight, lastCommit, 0) require.NoError(t, err) state.InitialHeight = nextHeight + 1 - err = blockExec.ValidateBlock(ctx, state, block) + err = blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{}) require.Error(t, err, "expected an error when state is ahead of block") assert.Contains(t, err.Error(), "lower than initial height") } @@ -244,7 +244,7 @@ func TestValidateBlockCommit(t *testing.T) { ) block, err := statefactory.MakeBlock(state, height, wrongHeightCommit, 0) require.NoError(t, err) - err = blockExec.ValidateBlock(ctx, state, block) + err = blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{}) var wantErr types.ErrInvalidCommitHeight require.True(t, errors.As(err, &wantErr), "expected ErrInvalidCommitHeight at height %d but got: %v", height, err) @@ -253,7 +253,7 @@ func TestValidateBlockCommit(t *testing.T) { */ block, err = statefactory.MakeBlock(state, height, wrongVoteMessageSignedCommit, 0) require.NoError(t, err) - err = blockExec.ValidateBlock(ctx, state, block) + err = blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{}) require.True( t, strings.HasPrefix( @@ -419,7 +419,7 @@ func TestValidateBlockEvidence(t *testing.T) { 0, ) - err := blockExec.ValidateBlock(ctx, state, block) + err := blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{}) if assert.Error(t, err) { _, ok := err.(*types.ErrEvidenceOverflow) require.True( diff --git a/internal/test/metricspy/counter.go b/internal/test/metricspy/counter.go new file mode 100644 index 000000000..67f00a752 --- /dev/null +++ b/internal/test/metricspy/counter.go @@ -0,0 +1,35 @@ +package metricspy + +import ( + "sync" + + "github.com/go-kit/kit/metrics" +) + +// Counter sums everything added to it, whatever the labels, so a test can read +// how often a code path was taken. Every counter derived through With shares +// the same total. +type Counter struct { + mtx *sync.Mutex + total *float64 +} + +// NewCounter returns a counter at zero. +func NewCounter() *Counter { + return &Counter{mtx: &sync.Mutex{}, total: new(float64)} +} + +func (c *Counter) With(_ ...string) metrics.Counter { return c } + +func (c *Counter) Add(delta float64) { + c.mtx.Lock() + defer c.mtx.Unlock() + *c.total += delta +} + +// Value returns the sum of everything added so far. +func (c *Counter) Value() float64 { + c.mtx.Lock() + defer c.mtx.Unlock() + return *c.total +} diff --git a/node/node_test.go b/node/node_test.go index fc7eba860..15f144cd3 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -616,7 +616,7 @@ func TestCreateProposalBlock(t *testing.T) { } assert.EqualValues(t, partSetFromHeader.ByteSize(), partSet.ByteSize()) - err = blockExec.ValidateBlock(ctx, state, block) + err = blockExec.ValidateBlock(ctx, state, block, types.VerifiedCommit{}) assert.NoError(t, err) assert.EqualValues(t, block.Header.ProposedAppVersion, proposedAppVersion) diff --git a/types/validator_set.go b/types/validator_set.go index 86a4b3ac2..1541ec7fc 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -959,6 +959,32 @@ func (vals *ValidatorSet) VerifyCommitWithBudget( return vals.verifyCommit(chainID, blockID, height, commit, budget) } +// VerifyCommitUnlessVerified verifies commit exactly as VerifyCommit does, +// unless verified carries proof of that very verification: the same chain, +// height, block ID, quorum and threshold key, and a commit whose signed content +// and signatures are unchanged. It reports whether the verification was +// skipped. +// +// Anything verified's proof does not cover — including the zero +// VerifiedCommit, the only proof-less value code outside this package can +// hold — falls through to VerifyCommit's own check and reports its errors +// unchanged, so callers can keep telling a forged commit from an honest +// disagreement by the error's type. +// +// commit is what is verified. The commit verified holds is never consulted. +func (vals *ValidatorSet) VerifyCommitUnlessVerified( + chainID string, + blockID BlockID, + height int64, + commit *Commit, + verified VerifiedCommit, +) (skipped bool, err error) { + if verified.proof.checkMatches(chainID, vals, blockID, height, commit) == nil { + return true, nil + } + return false, vals.verifyCommit(chainID, blockID, height, commit, nil) +} + func (vals *ValidatorSet) verifyCommit( chainID string, blockID BlockID, @@ -966,34 +992,34 @@ func (vals *ValidatorSet) verifyCommit( commit *Commit, budget VerificationBudget, ) error { - // Validate Height and BlockID. - if height != commit.Height { - return NewErrInvalidCommitHeight(height, commit.Height) - } - if !blockID.Equals(commit.BlockID) { - return fmt.Errorf("invalid commit -- wrong block ID: want %v, got %v", - blockID, commit.BlockID) - } + _, _, err := vals.verifyCommitReportingSigns(chainID, blockID, height, commit, budget) + return err +} - canonVote, err := commit.GetCanonicalVote() - if err != nil { - return err - } - quorumSigns, err := makeVerifyQuorumSigns(chainID, vals.QuorumType, vals.QuorumHash, canonVote.ToProto()) +// verifyCommitReportingSigns verifies commit and, on success, reports the +// signing data that was checked and the signatures it was checked against, so +// a caller can later establish that a commit still holds what this +// verification covered without repeating it. +func (vals *ValidatorSet) verifyCommitReportingSigns( + chainID string, + blockID BlockID, + height int64, + commit *Commit, + budget VerificationBudget, +) (QuorumSignData, QuorumSigns, error) { + quorumSigns, err := vals.commitSignData(chainID, blockID, height, commit) if err != nil { - return err - } - if !vals.QuorumHash.Equal(commit.QuorumHash) { - return ErrInvalidCommitQuorumHash{Expected: vals.QuorumHash, Actual: commit.QuorumHash} + return QuorumSignData{}, QuorumSigns{}, err } + signs := NewQuorumSignsFromCommit(commit) if budget != nil { - err = quorumSigns.VerifyWithBudget(vals.ThresholdPublicKey, NewQuorumSignsFromCommit(commit), budget) + err = quorumSigns.VerifyWithBudget(vals.ThresholdPublicKey, signs, budget) } else { - err = quorumSigns.Verify(vals.ThresholdPublicKey, NewQuorumSignsFromCommit(commit)) + err = quorumSigns.Verify(vals.ThresholdPublicKey, signs) } if err != nil { if errors.Is(err, ErrVerificationBudgetExhausted) { - return err + return QuorumSignData{}, QuorumSigns{}, err } // A vote-extension count mismatch is an application/version disagreement an // honest peer can produce (it stores and relays a commit whose extension @@ -1001,19 +1027,52 @@ func (vals *ValidatorSet) verifyCommit( // caller does not evict the sender for it. var countMismatch ErrVoteExtensionCountMismatch if errors.As(err, &countMismatch) { - return err + return QuorumSignData{}, QuorumSigns{}, err } // Otherwise the threshold signature itself is forged: a node stores a commit // only after it verifies, so no honest peer relays one with a bad signature. // Typed so the caller can evict the sender. - return ErrInvalidCommitSignature{ + return QuorumSignData{}, QuorumSigns{}, ErrInvalidCommitSignature{ QuorumType: vals.QuorumType, QuorumHash: vals.QuorumHash, ThresholdPublicKey: vals.ThresholdPublicKey, Err: err, } } - return nil + return quorumSigns, signs, nil +} + +// commitSignData runs every check verifyCommit makes before it touches a +// signature and returns the signing data the signatures are verified against. +// A commitProof re-runs it on the commit it is offered, so the two +// cannot come to disagree about what those checks are. +func (vals *ValidatorSet) commitSignData( + chainID string, + blockID BlockID, + height int64, + commit *Commit, +) (QuorumSignData, error) { + // Validate Height and BlockID. + if height != commit.Height { + return QuorumSignData{}, NewErrInvalidCommitHeight(height, commit.Height) + } + if !blockID.Equals(commit.BlockID) { + return QuorumSignData{}, fmt.Errorf("invalid commit -- wrong block ID: want %v, got %v", + blockID, commit.BlockID) + } + + canonVote, err := commit.GetCanonicalVote() + if err != nil { + return QuorumSignData{}, err + } + quorumSigns, err := makeVerifyQuorumSigns(chainID, vals.QuorumType, vals.QuorumHash, canonVote.ToProto()) + if err != nil { + return QuorumSignData{}, err + } + if !vals.QuorumHash.Equal(commit.QuorumHash) { + return QuorumSignData{}, ErrInvalidCommitQuorumHash{Expected: vals.QuorumHash, Actual: commit.QuorumHash} + } + return quorumSigns, nil } //----------------- diff --git a/types/verified_commit.go b/types/verified_commit.go new file mode 100644 index 000000000..b96aa0446 --- /dev/null +++ b/types/verified_commit.go @@ -0,0 +1,200 @@ +package types + +import ( + "bytes" + "errors" + "fmt" + "reflect" + + "github.com/dashpay/dashd-go/btcjson" + + "github.com/dashpay/tenderdash/crypto" +) + +// errCommitProofMismatch reports a commit proof offered for a commit, a chain or +// a quorum it was not produced for, or for a commit whose signed content has +// changed since — or no proof at all. ValidatorSet.VerifyCommitUnlessVerified +// answers it by verifying the commit in full, so it never reaches a caller. +var errCommitProofMismatch = errors.New("commit proof does not match the commit being verified") + +// VerifiedCommit is a commit together with, when it has one, proof that the +// commit's threshold signatures were verified and against exactly which +// parameters. ValidatorSet.VerifyCommitUnlessVerified accepts the proof in place +// of verifying the same commit again, but only after checking that every +// parameter it records is one it would have verified with itself. +// +// Only VerifyCommitSignatures attaches a proof, and only after the signatures +// verified. Nothing outside this package can build a VerifiedCommit that +// names a commit without a proof to match it: the zero value is the only +// proof-less VerifiedCommit a caller can construct, and it names no commit +// either. Code that has verified nothing passes the zero value freely, since +// it only ever leads to a full verification. +// +// The commit is the caller's pointer: it names what the proof is about and is +// never itself evidence of anything. See Commit. +// +// A VerifiedCommit is meaningful only within the process and flow that built it; +// it is never persisted or sent anywhere. +type VerifiedCommit struct { + commit *Commit + proof *commitProof +} + +// commitProof is evidence that one specific commit's threshold signatures were +// verified, and against exactly which parameters. +// +// Whether a commit's signatures check out is a function of the commit's signed +// content, the chain, the quorum and the threshold key alone. Evidence naming +// the same values therefore establishes exactly what a fresh verification +// would. Recording those values — rather than carrying a bare "already +// verified" flag — is what makes evidence produced for one chain, quorum, +// height or commit worthless anywhere else. +// +// Everything recorded is an immutable value or a copy this package owns, never +// a reference into the validator set, block ID or commit it was minted from. A +// commit verified against one height's validator set is checked one height on +// against another object holding the same quorum, and anything held by +// reference would report whatever its owner last wrote. +type commitProof struct { + chainID string + height int64 + blockID BlockID + quorumType btcjson.LLMQType + quorumHash crypto.QuorumHash + + // thresholdKeyType and thresholdKeyBytes name the key the signatures were + // checked against, as material rather than as the key object itself. A key + // is an interface over bytes its supplier owns, and the implementation + // behind it is whatever the caller passed — one that answers yes to every + // question is as easy to supply as a real one. Recording the type and a copy + // of the bytes lets the key the consumer trusts decide the comparison. + thresholdKeyType reflect.Type + thresholdKeyBytes []byte + + // signHashes and signatures are exactly what was handed to the signature + // check: the digest of the block followed by one digest per + // threshold-recoverable vote extension, and the signature verified against + // each. A real verification always records at least the block digest. + signHashes [][]byte + signatures [][]byte +} + +// Commit returns the commit v was built for, as the pointer v was built with. +// +// It is not evidence. Holding a VerifiedCommit proves nothing about the commit +// it returns, which the caller that built v may have changed since, and a proof +// only ever covers the commit ValidatorSet.VerifyCommitUnlessVerified is handed. +// Never verify or validate this commit in place of the one actually received — +// a block's LastCommit is read from the block — or a commit other than the one +// the chain carries could be accepted. +func (v VerifiedCommit) Commit() *Commit { + return v.commit +} + +// VerifyCommitSignatures verifies commit against vals as the commit for +// blockID at height, charging each stage to budget when one is given. It runs +// the same check, and reports the same errors, as ValidatorSet.VerifyCommit; +// on success it returns commit with the proof +// ValidatorSet.VerifyCommitUnlessVerified accepts in place of repeating that +// check. On failure it returns the zero VerifiedCommit. +func VerifyCommitSignatures( + vals *ValidatorSet, + chainID string, + blockID BlockID, + height int64, + commit *Commit, + budget VerificationBudget, +) (VerifiedCommit, error) { + if vals == nil { + return VerifiedCommit{}, ErrValidatorSetNilOrEmpty + } + if commit == nil { + return VerifiedCommit{}, errors.New("nil commit") + } + signData, signs, err := vals.verifyCommitReportingSigns(chainID, blockID, height, commit, budget) + if err != nil { + return VerifiedCommit{}, err + } + signHashes, signatures := signedContent(signData, signs) + return VerifiedCommit{ + commit: commit, + proof: &commitProof{ + chainID: chainID, + height: height, + blockID: blockID.Copy(), + quorumType: vals.QuorumType, + quorumHash: bytes.Clone(vals.QuorumHash), + thresholdKeyType: reflect.TypeOf(vals.ThresholdPublicKey), + thresholdKeyBytes: bytes.Clone(vals.ThresholdPublicKey.Bytes()), + signHashes: signHashes, + signatures: signatures, + }, + }, nil +} + +// checkMatches reports whether p is evidence about this exact commit under +// these exact parameters. A nil p — a VerifiedCommit without proof — matches +// nothing. It compares what a verification would compare, so a commit it does +// not reject establishes what verifying that commit again would establish — no +// more, and nothing that verification would have caught less. +// +// The commit's content is compared by running the checks a verification runs +// before it touches a signature, rebuilding the digests it would verify, and +// holding them and the commit's signatures against what was verified. That +// costs a marshal and a hash per signature — never a pairing. +// +// commit is compared by content, never by identity with the commit the proof +// was minted with: a pointer says nothing about what it points to now. +func (p *commitProof) checkMatches( + chainID string, + vals *ValidatorSet, + blockID BlockID, + height int64, + commit *Commit, +) error { + switch { + case p == nil || len(p.signHashes) == 0: + return fmt.Errorf("%w: nothing was verified", errCommitProofMismatch) + case vals == nil: + return fmt.Errorf("%w: no validator set to verify against", errCommitProofMismatch) + case commit == nil: + return fmt.Errorf("%w: no commit to verify", errCommitProofMismatch) + case p.chainID != chainID: + return fmt.Errorf("%w: verified for chain %q, offered for %q", + errCommitProofMismatch, p.chainID, chainID) + case p.height != height: + return fmt.Errorf("%w: verified for height %d, offered for %d", + errCommitProofMismatch, p.height, height) + case !p.blockID.Equals(blockID): + return fmt.Errorf("%w: verified for block %v, offered for %v", + errCommitProofMismatch, p.blockID, blockID) + case p.quorumType != vals.QuorumType: + return fmt.Errorf("%w: verified for quorum type %d, offered under %d", + errCommitProofMismatch, p.quorumType, vals.QuorumType) + case !bytes.Equal(p.quorumHash, vals.QuorumHash): + return fmt.Errorf("%w: verified for quorum %X, offered under %X", + errCommitProofMismatch, p.quorumHash, vals.QuorumHash) + case vals.ThresholdPublicKey == nil || + reflect.TypeOf(vals.ThresholdPublicKey) != p.thresholdKeyType || + !bytes.Equal(p.thresholdKeyBytes, vals.ThresholdPublicKey.Bytes()): + return fmt.Errorf("%w: verified against a different threshold public key", + errCommitProofMismatch) + } + + signData, err := vals.commitSignData(chainID, blockID, height, commit) + if err != nil { + return fmt.Errorf("%w: the commit no longer passes verification's preliminary checks: %s", + errCommitProofMismatch, err) + } + signHashes, signatures := signedContent(signData, NewQuorumSignsFromCommit(commit)) + if !equalByteSlices(p.signHashes, signHashes) { + return fmt.Errorf("%w: the commit's signed content is not what was verified", + errCommitProofMismatch) + } + if !equalByteSlices(p.signatures, signatures) { + return fmt.Errorf("%w: the commit no longer carries the signatures that were verified", + errCommitProofMismatch) + } + + return nil +} diff --git a/types/verified_commit_test.go b/types/verified_commit_test.go new file mode 100644 index 000000000..8cdd02188 --- /dev/null +++ b/types/verified_commit_test.go @@ -0,0 +1,396 @@ +package types + +import ( + "bytes" + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/dashpay/tenderdash/crypto" + "github.com/dashpay/tenderdash/crypto/bls12381" + tmproto "github.com/dashpay/tenderdash/proto/tendermint/types" +) + +// A verified commit's proof is accepted in place of verifying a commit again, +// so it is only as safe as what it names. These pin that it names exactly the +// inputs a verification is handed, that it can only be produced by verifying, +// and that anything it does not cover is verified in full — with the errors a +// full verification reports, since callers decide whether to evict a peer on +// their type. + +// commitInputs is everything ValidatorSet.VerifyCommit is handed. +type commitInputs struct { + chainID string + vals *ValidatorSet + blockID BlockID + height int64 + commit *Commit +} + +// newCommitInputs returns a genuine threshold-signed commit, carrying +// threshold-recoverable vote extensions, and the inputs it verifies against. +func newCommitInputs(t *testing.T) commitInputs { + t.Helper() + ctx := context.Background() + const height = int64(3) + blockID := makeBlockIDRandom() + voteSet, valSet, privVals := randVoteSet(ctx, t, height, 0, tmproto.PrecommitType, 4) + commit, err := makeCommit(ctx, blockID, height, 0, voteSet, privVals) + require.NoError(t, err) + require.NotEmpty(t, commit.ThresholdVoteExtensions, + "the commit must carry vote extensions for their bytes to be part of what is verified") + in := commitInputs{chainID: voteSet.ChainID(), vals: valSet, blockID: blockID, height: height, commit: commit} + require.NoError(t, in.verify(), "the genuine commit must verify, otherwise the negative cases prove nothing") + return in +} + +// clone returns a copy sharing no memory with in, so a case can rewrite any +// input without disturbing another. +func (in commitInputs) clone(t *testing.T) commitInputs { + t.Helper() + vals := in.vals.Copy() + vals.QuorumHash = bytes.Clone(in.vals.QuorumHash) + vals.ThresholdPublicKey = bls12381.PubKey(bytes.Clone(in.vals.ThresholdPublicKey.Bytes())) + + encoded, err := in.commit.ToProto().Marshal() + require.NoError(t, err) + var decoded tmproto.Commit + require.NoError(t, decoded.Unmarshal(encoded)) + commit, err := CommitFromProto(&decoded) + require.NoError(t, err) + + return commitInputs{chainID: in.chainID, vals: vals, blockID: in.blockID.Copy(), height: in.height, commit: commit} +} + +func (in commitInputs) verify() error { + return in.vals.VerifyCommit(in.chainID, in.blockID, in.height, in.commit) +} + +func (in commitInputs) mint(budget VerificationBudget) (VerifiedCommit, error) { + return VerifyCommitSignatures(in.vals, in.chainID, in.blockID, in.height, in.commit, budget) +} + +func (in commitInputs) matches(v VerifiedCommit) error { + return v.proof.checkMatches(in.chainID, in.vals, in.blockID, in.height, in.commit) +} + +func (in commitInputs) verifyUnlessVerified(v VerifiedCommit) (bool, error) { + return in.vals.VerifyCommitUnlessVerified(in.chainID, in.blockID, in.height, in.commit, v) +} + +// A verified commit covers the commit it was minted for, and a caller holding +// it skips the threshold verification for that commit. +func TestVerifyCommitSignaturesCoversTheCommitItVerified(t *testing.T) { + in := newCommitInputs(t) + + verified, err := in.mint(nil) + require.NoError(t, err) + require.NotNil(t, verified.proof) + require.Same(t, in.commit, verified.Commit(), "the verified commit holds the caller's commit") + require.NoError(t, in.matches(verified)) + + // Equal inputs held in different memory: the proof is about values, and block + // sync compares it against a validator set one height on that is a different + // object holding the same quorum. + require.NoError(t, in.clone(t).matches(verified)) + + skipped, err := in.verifyUnlessVerified(verified) + require.NoError(t, err) + require.True(t, skipped, "a commit the proof covers must not be verified again") +} + +// Nothing is handed out for a commit that failed verification, so there is +// nothing to skip on: the commit is verified again and rejected again. +func TestVerifyCommitSignaturesYieldsNothingForABadCommit(t *testing.T) { + in := newCommitInputs(t).clone(t) + in.commit.ThresholdBlockSignature[0] ^= 0xff + + verified, err := in.mint(nil) + require.ErrorAs(t, err, &ErrInvalidCommitSignature{}) + require.Equal(t, VerifiedCommit{}, verified) + + skipped, err := in.verifyUnlessVerified(verified) + require.False(t, skipped) + require.ErrorAs(t, err, &ErrInvalidCommitSignature{}) +} + +// Missing inputs are reported rather than dereferenced: block sync verifies +// peer-supplied commits through here. +func TestVerifyCommitSignaturesRejectsMissingInputs(t *testing.T) { + in := newCommitInputs(t) + + verified, err := VerifyCommitSignatures(nil, in.chainID, in.blockID, in.height, in.commit, nil) + require.ErrorIs(t, err, ErrValidatorSetNilOrEmpty) + require.Equal(t, VerifiedCommit{}, verified) + + verified, err = VerifyCommitSignatures(in.vals, in.chainID, in.blockID, in.height, nil, nil) + require.Error(t, err) + require.Equal(t, VerifiedCommit{}, verified) +} + +// A VerifiedCommit without proof is what code that verified nothing holds — +// consensus, the replayer and block sync all pass the zero value. Nothing +// outside this package can build a proof-less VerifiedCommit that names a +// real commit; this in-package literal exists only to pin that even that +// stronger case — a bare commit with no proof, and no exported way to +// construct it — may never let a verification be skipped, not even when it +// holds the very commit being verified, and may never cause a commit to be +// rejected: it only ever falls through to a real verification. +func TestUnverifiedCommitMatchesNothing(t *testing.T) { + in := newCommitInputs(t) + + holding := VerifiedCommit{commit: in.commit} + require.Same(t, in.commit, holding.Commit()) + require.Nil(t, holding.proof) + + for name, unverified := range map[string]VerifiedCommit{ + "zero value": {}, + "holding the commit it verifies": holding, + } { + t.Run(name, func(t *testing.T) { + require.ErrorIs(t, in.matches(unverified), errCommitProofMismatch) + + skipped, err := in.verifyUnlessVerified(unverified) + require.NoError(t, err, "a genuine commit offered without proof must still pass") + require.False(t, skipped) + + forged := in.clone(t) + forged.commit.ThresholdBlockSignature[0] ^= 0xff + skipped, err = forged.verifyUnlessVerified(unverified) + require.False(t, skipped) + require.ErrorAs(t, err, &ErrInvalidCommitSignature{}) + }) + } +} + +// A proof is about one commit under one set of inputs. Changing any one of +// them — on the verifier's side or in the commit — must not be covered, and +// must fall through to a verification that rejects it exactly as VerifyCommit +// does. +func TestVerifiedCommitDoesNotCoverADifferentVerification(t *testing.T) { + genuine := newCommitInputs(t) + verified, err := genuine.mint(nil) + require.NoError(t, err) + + testCases := []struct { + name string + mutate func(in *commitInputs) + // panics is set where VerifyCommit itself dereferences the missing input; + // those cases are only asked whether the proof matches + panics bool + }{ + {name: "chain ID", mutate: func(in *commitInputs) { in.chainID += "-fork" }}, + {name: "height", mutate: func(in *commitInputs) { in.height++ }}, + {name: "block ID", mutate: func(in *commitInputs) { in.blockID.Hash[0] ^= 0x01 }}, + {name: "quorum type", mutate: func(in *commitInputs) { in.vals.QuorumType++ }}, + {name: "quorum hash", mutate: func(in *commitInputs) { in.vals.QuorumHash = crypto.RandQuorumHash() }}, + {name: "threshold key", mutate: func(in *commitInputs) { + in.vals.ThresholdPublicKey = bls12381.GenPrivKey().PubKey() + }}, + {name: "threshold key bytes", mutate: func(in *commitInputs) { + in.vals.ThresholdPublicKey.Bytes()[0] ^= 0x01 + }}, + {name: "nil threshold key", mutate: func(in *commitInputs) { in.vals.ThresholdPublicKey = nil }, panics: true}, + {name: "nil validator set", mutate: func(in *commitInputs) { in.vals = nil }, panics: true}, + {name: "nil commit", mutate: func(in *commitInputs) { in.commit = nil }, panics: true}, + {name: "commit height", mutate: func(in *commitInputs) { in.commit.Height++ }}, + {name: "commit round", mutate: func(in *commitInputs) { in.commit.Round++ }}, + {name: "commit block hash", mutate: func(in *commitInputs) { in.commit.BlockID.Hash[0] ^= 0x01 }}, + {name: "commit state ID", mutate: func(in *commitInputs) { in.commit.BlockID.StateID[0] ^= 0x01 }}, + {name: "commit quorum hash", mutate: func(in *commitInputs) { in.commit.QuorumHash = crypto.RandQuorumHash() }}, + {name: "commit block signature", mutate: func(in *commitInputs) { + in.commit.ThresholdBlockSignature[0] ^= 0x01 + }}, + {name: "commit vote extension", mutate: func(in *commitInputs) { + ext := in.commit.ThresholdVoteExtensions[len(in.commit.ThresholdVoteExtensions)-1] + ext.Extension[0] ^= 0x01 + }}, + {name: "commit vote-extension signature", mutate: func(in *commitInputs) { + in.commit.ThresholdVoteExtensions[0].Signature[0] ^= 0x01 + }}, + {name: "commit gains a vote extension", mutate: func(in *commitInputs) { + in.commit.ThresholdVoteExtensions = append(in.commit.ThresholdVoteExtensions, + &tmproto.VoteExtension{ + Type: tmproto.VoteExtensionType_THRESHOLD_RECOVER, + Extension: []byte("extra"), + Signature: bytes.Clone(in.commit.ThresholdVoteExtensions[0].Signature), + }) + }}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + in := genuine.clone(t) + tc.mutate(&in) + + require.ErrorIs(t, in.matches(verified), errCommitProofMismatch) + if tc.panics { + return + } + + want := in.verify() + require.Error(t, want, "the mutation left the commit verifiable, so not covering it would prove nothing") + skipped, err := in.verifyUnlessVerified(verified) + require.False(t, skipped) + require.Equal(t, want, err, "an uncovered commit must be verified exactly as VerifyCommit verifies it") + }) + } +} + +// A proof owns what it records. Rewriting the inputs it was minted from +// afterwards — through the commit, block ID or validator set the caller still +// holds — must not change what it covers. The commit itself is held by the +// caller's pointer on purpose: it is what the proof is about, not part of it, +// so rewriting it must make the proof refuse even that very commit. +func TestVerifiedCommitProofSurvivesMutationOfItsInputs(t *testing.T) { + genuine := newCommitInputs(t) + minted := genuine.clone(t) + pristine := genuine.clone(t) + + verified, err := minted.mint(nil) + require.NoError(t, err) + require.Same(t, minted.commit, verified.Commit(), + "the verified commit embeds the caller's commit pointer by design") + + minted.blockID.Hash[0] ^= 0xff + minted.commit.ThresholdBlockSignature[0] ^= 0xff + minted.commit.ThresholdVoteExtensions[0].Signature[0] ^= 0xff + minted.vals.QuorumHash[0] ^= 0xff + minted.vals.ThresholdPublicKey.Bytes()[0] ^= 0xff + + require.NoError(t, pristine.matches(verified), + "the proof must not alias the block ID, commit or validator set it was minted from") + require.ErrorIs(t, minted.matches(verified), errCommitProofMismatch, + "the rewritten inputs, including the embedded commit itself, are not what was verified") +} + +// The threshold key is an interface, so a caller can mint with an +// implementation that verifies anything while reporting the real key's bytes. +// Whether a proof covers a verification has to be decided by the key the +// consumer trusts, not by the key the proof was minted with. +func TestVerifiedCommitFromAKeyThatAnswersYesMatchesNothing(t *testing.T) { + in := newCommitInputs(t).clone(t) + in.commit.ThresholdBlockSignature = make([]byte, SignatureSize) + + hostile := in.vals.Copy() + hostile.ThresholdPublicKey = yesPubKey{bytes: bytes.Clone(in.vals.ThresholdPublicKey.Bytes())} + + // whether such a key can mint at all is not the point — the zero value it + // leaves behind on failure is refused for the same reason + verified, _ := VerifyCommitSignatures(hostile, in.chainID, in.blockID, in.height, in.commit, nil) + + require.ErrorIs(t, in.matches(verified), errCommitProofMismatch, + "a proof minted by a key of the caller's choosing was accepted") + skipped, err := in.verifyUnlessVerified(verified) + require.False(t, skipped) + require.ErrorAs(t, err, &ErrInvalidCommitSignature{}) +} + +// Callers evict a peer on ErrInvalidCommitSignature alone, and tolerate every +// other commit failure. Minting a verified commit, and consuming one whose +// proof does not cover the commit, must report exactly the error VerifyCommit +// reports — same type, same content — for every failure VerifyCommit +// distinguishes. +func TestVerifiedCommitKeepsTheVerifyCommitErrors(t *testing.T) { + genuine := newCommitInputs(t) + verified, err := genuine.mint(nil) + require.NoError(t, err) + + testCases := []struct { + name string + mutate func(in *commitInputs) + typed bool + check func(t *testing.T, err error) + }{ + { + name: "forged threshold signature", + mutate: func(in *commitInputs) { in.commit.ThresholdBlockSignature[0] ^= 0xff }, + typed: true, + }, + { + name: "wrong block ID", + mutate: func(in *commitInputs) { in.blockID = makeBlockIDRandom() }, + }, + { + name: "wrong quorum hash", + mutate: func(in *commitInputs) { in.commit.QuorumHash = crypto.RandQuorumHash() }, + check: func(t *testing.T, err error) { + require.ErrorAs(t, err, &ErrInvalidCommitQuorumHash{}) + }, + }, + { + name: "wrong height", + mutate: func(in *commitInputs) { in.height++ }, + check: func(t *testing.T, err error) { + require.ErrorAs(t, err, &ErrInvalidCommitHeight{}) + }, + }, + { + // Extensions are outside the block signature's digest and a DEFAULT one + // yields no sign item, so the block signature verifies and the counts + // then disagree: what an honest peer with another extension + // configuration produces. + name: "vote-extension count mismatch", + mutate: func(in *commitInputs) { + in.commit.ThresholdVoteExtensions = append(in.commit.ThresholdVoteExtensions, + &tmproto.VoteExtension{ + Type: tmproto.VoteExtensionType_DEFAULT, + Extension: []byte("not threshold-recoverable"), + Signature: make([]byte, SignatureSize), + }) + }, + check: func(t *testing.T, err error) { + require.ErrorAs(t, err, &ErrVoteExtensionCountMismatch{}) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + in := genuine.clone(t) + tc.mutate(&in) + + want := in.verify() + require.Error(t, want) + if tc.typed { + require.ErrorAs(t, want, &ErrInvalidCommitSignature{}) + } else { + require.NotErrorAs(t, want, &ErrInvalidCommitSignature{}) + } + if tc.check != nil { + tc.check(t, want) + } + + minted, err := in.mint(nil) + require.Equal(t, want, err, "minting must fail exactly as VerifyCommit fails") + require.Equal(t, VerifiedCommit{}, minted) + + skipped, err := in.verifyUnlessVerified(verified) + require.False(t, skipped) + require.Equal(t, want, err, "an uncovered commit must fail exactly as VerifyCommit fails") + }) + } +} + +// Minting charges the verification budget exactly as VerifyCommitWithBudget +// does, and an exhausted budget is reported as such rather than as forgery. +func TestVerifyCommitSignaturesChargesTheBudget(t *testing.T) { + in := newCommitInputs(t) + + exhausted := &recordingVerificationBudget{decisions: []bool{false}} + verified, err := in.mint(exhausted) + require.ErrorIs(t, err, ErrVerificationBudgetExhausted) + require.NotErrorAs(t, err, &ErrInvalidCommitSignature{}) + require.Equal(t, VerifiedCommit{}, verified) + require.Equal(t, []int{1}, exhausted.costs) + + charged := &recordingVerificationBudget{} + verified, err = in.mint(charged) + require.NoError(t, err) + require.NoError(t, in.matches(verified)) + + reference := &recordingVerificationBudget{} + require.NoError(t, in.vals.VerifyCommitWithBudget(in.chainID, in.blockID, in.height, in.commit, reference)) + require.Equal(t, reference.costs, charged.costs) +}