fix(consensus): verify commit vote extensions before a commit is taken

.github/workflows/proto-lint.yml

@@ -17,6 +17,7 @@ jobs:
       - uses: bufbuild/buf-setup-action@v1.50.0
         with:
           version: "1.59.0"
+          github_token: ${{ github.token }}
       - uses: bufbuild/buf-lint-action@v1
         with:
           input: "proto"

UPGRADING.md

@@ -2,6 +2,111 @@
 
 This guide provides instructions for upgrading to specific versions of Tenderdash.
 
+## Unreleased
+
+### ABCI: commit vote-extension verification
+
+Tenderdash now calls `VerifyVoteExtension` after `ProcessProposal` and before it
+saves or finalizes a block. An empty `validator_pro_tx_hash` identifies this
+commit-level call. Applications must compare the supplied extensions, in order,
+with the threshold-recoverable extensions expected for the processed block and
+return `REJECT` on any difference.
+
+Check for an empty `validator_pro_tx_hash` before applying individual-validator
+validation. For this request, return `ACCEPT` or `REJECT`: any status other than
+`ACCEPT`, including `UNKNOWN`, is treated as `REJECT`, and only an ABCI-level
+error (including a transport failure) is fatal and causes Tenderdash to panic.
+Applications that keep the default unconditional `ACCEPT` remain compatible but
+do not gain protection against altered commit extension vectors.
+
+**Before you upgrade:** an application that returns anything but `ACCEPT` for an
+empty or unknown `validator_pro_tx_hash`, or validates extensions against a
+per-validator key, will reject every commit once Tenderdash is upgraded, and the
+network stops producing blocks. Ship the application's commit-level handling
+first: older Tenderdash never sends this call, so that order is safe.
+
+Consensus discards a rejected commit before saving anything, like a commit with
+an invalid signature but without blaming its sender. A commit whose block the
+node already holds is checked before anything else and never changes the round.
+A commit for a later round whose block has not arrived moves the node to that
+round on its authentic threshold signature alone, as before; the application's
+check runs when the block completes. A commit that arrived before its block is
+checked when the block arrives, followed by the commits other peers sent
+meanwhile, one queue slot per connected peer, so an attacker cannot push an
+honest peer's commit out; a copy of the parked commit, or of the commit its
+sender already queued, is dropped, while a copy of another peer's queued commit
+takes the sender's own slot. If the commit assembled from the node's own +2/3
+precommits is rejected, the node logs the error `application rejected the commit
+of this node's own precommits; waiting for a peer's commit`, persists nothing and
+waits at the height, with no timeout, for a peer's commit instead of panicking
+into a restart loop. Ordinary consensus WAL replay behaves like consensus. Block
+sync removes the peer that served the rejected commit and retries the height
+from another peer. During handshake catch-up of application state to blocks
+already in the block store, rejection stops startup because the commit is
+already persisted. The error identifies the failing height, round and hash.
+
+Every rejection is counted by the `consensus_commit_verify_failures` counter
+with `reason="extensions_rejected"` (see [metrics](docs/nodes/metrics.md)); a
+sustained rate means the application and the network disagree on extension
+vectors.
+
+**`ExtendVote` is now also a commit oracle.** After the application rejects a
+commit's vote extensions in a commit-level `VerifyVoteExtension`, which follows
+`ProcessProposal` of that block and round, Tenderdash calls `ExtendVote` for that
+block, height and round, on any node including non-validators. A commit's extension list is not covered by any signature beyond
+each entry's own, so one genuine commit yields any number of signature-valid
+vectors; Tenderdash refuses, without processing the block again or calling the
+application, a later commit of that block and round that would need the block
+processed again, when its threshold-recoverable vector (type, extension and sign
+request ID, in order; an empty sign request ID equals an unset one) differs from
+what `ExtendVote` returned, or equals it and was rejected. A commit of the round
+processed last still goes to the application. This bounds the `ProcessProposal` executions commits can force
+per height to 2·G, plus the one that finalizes, where G is the number of rounds
+other than the node's own holding a commit with a valid threshold block
+signature for the held block, independent of peers and vectors. Applications
+must therefore keep `ExtendVote` free of side effects and return exactly the
+vector their commit-level `VerifyVoteExtension` accepts; an ABCI error or an
+invalid response from this call is fatal. Extensions that are not
+threshold-recoverable can still be non-deterministic. Drive and the e2e application do; kvstore extends with no
+vector and accepts any. With an application whose `ExtendVote` returns another
+vector than it accepts, a node may be unable to finalize from a peer's commit
+of a round at which it already rejected one.
+
+Before verifying a peer's precommit, and before extending its own precommit
+when relocking, Tenderdash has the round's proposal block processed for the
+current round again if a rejected commit of another round left the application
+on that round.
+
+Tenderdash processes a block again with `ProcessProposal` when it returns to a
+round after processing another round at the same height, and may repeat
+`ProcessProposal` for the same height, round and block, for example after the
+application rejected a commit's vote extensions or when block sync hands over
+to consensus. It finalizes only the round processed last. An application may
+keep a single execution context per height, but must treat either call as
+re-execution that replaces that context, not as an error. Drive does; the
+reference kvstore application errors only on a repeat of the round processed
+last with a different block hash.
+
+For a catch-up failure, first ensure the application implements verification for
+the failing historical height; re-sync cannot fix incompatible application logic.
+Restore a consistent application and Tenderdash backup, or use the application's
+supported state-sync or genesis-sync procedure with fresh stores. Preserve the
+validator keys and signing state to avoid double-signing.
+
+If the block store is exactly one height ahead of consensus state and the
+application has not applied the rejected block, `tenderdash rollback --store`
+can remove that last block and its stored commit without rolling consensus state
+back. Restarting then allows the node to fetch and verify a replacement.
+Without `--store`, rollback does not remove the stored block or commit. Rollback
+never rolls back application state; verify the application height before using it.
+For multi-block application catch-up, rollback is not a general repair: it
+requires consensus state to be at the block-store height or one below, and deleting
+the last block cannot remove an offending commit earlier in the store.
+
+This check protects the commit supplied for the block being finalized. It does
+not authenticate the extension vector embedded in a proposal's `LastCommit`;
+applications must not trust that vector solely on its block threshold signature.
+
 ## v1.7.0
 
 ### Consensus DoS hardening (peer verification limits)

abci/example/kvstore/kvstore.go

@@ -54,10 +54,11 @@ type Application struct {
 	// If `nil`, duplicate call detection is disabled.
 	preparedProposals map[int32]bool
 
-	// processedProposals stores info about all rounds that got ProcessProposal executed, used to detect
-	// duplicate ProcessProposal calls.
+	// processedProposals holds the round and block hash of the latest ProcessProposal at this height.
+	// Like Drive, a repeat of that round with the same block hash re-executes it, and any other round
+	// may be processed again; that round with another or no block hash is an error.
 	// If `nil`, duplicate call detection is disabled.
-	processedProposals map[int32]bool
+	processedProposals map[int32][]byte
 
 	logger log.Logger
 
@@ -217,7 +218,7 @@ func newApplication(stateStore StoreFactory, opts ...OptFunc) (*Application, err
 		LastCommittedState:     NewKvState(dbm.NewMemDB(), initialHeight), // initial state to avoid InitChain() in unit tests
 		roundStates:            map[string]State{},
 		preparedProposals:      map[int32]bool{},
-		processedProposals:     map[int32]bool{},
+		processedProposals:     map[int32][]byte{},
 		validatorSetUpdates:    map[int64]abci.ValidatorSetUpdate{},
 		consensusParamsUpdates: map[int64]types1.ConsensusParams{},
 		initialHeight:          initialHeight,
@@ -377,10 +378,11 @@ func (app *Application) ProcessProposal(_ context.Context, req *abci.RequestProc
 	defer app.mu.Unlock()
 
 	if app.processedProposals != nil {
-		if app.processedProposals[req.Round] {
+		if last, ok := app.processedProposals[req.Round]; ok && (len(req.Hash) == 0 || !bytes.Equal(last, req.Hash)) {
 			return &abci.ResponseProcessProposal{}, fmt.Errorf("duplicate ProcessProposal call at height %d, round %d", req.Height, req.Round)
 		}
-		app.processedProposals[req.Round] = true
+		clear(app.processedProposals)
+		app.processedProposals[req.Round] = bytes.Clone(req.Hash)
 	}
 
 	roundState, txResults, err := app.executeProposal(req.Height, req.Round, types.NewTxs(req.Txs))
@@ -776,7 +778,7 @@ func (app *Application) newHeight(committedAppHash tmbytes.HexBytes, height int6
 func (app *Application) resetDuplicateDetection(enabled bool) {
 	if enabled {
 		app.preparedProposals = map[int32]bool{}
-		app.processedProposals = map[int32]bool{}
+		app.processedProposals = map[int32][]byte{}
 	} else {
 		app.preparedProposals = nil
 		app.processedProposals = nil
@@ -822,7 +824,13 @@ func (app *Application) executeProposal(height int64, round int32, txs types.Txs
 			return nil, nil, fmt.Errorf("update apphash: %w", err)
 		}
 	}
-	app.roundStates[roundKey(roundState.GetAppHash(), roundState.GetHeight(), roundState.GetRound())] = roundState
+	key := roundKey(roundState.GetAppHash(), roundState.GetHeight(), roundState.GetRound())
+	if replaced, ok := app.roundStates[key]; ok {
+		if err := replaced.Close(); err != nil {
+			app.logger.Error("cannot close replaced round state", "key", key, "err", err)
+		}
+	}
+	app.roundStates[key] = roundState
 
 	return roundState, txResults, nil
 }

abci/example/kvstore/kvstore_test.go

@@ -597,3 +597,50 @@ func bytes2Txs(items [][]byte) []tmtypes.Tx {
 	}
 	return txs
 }
+
+// TestProcessProposalRoundReprocessing checks that duplicate detection matches
+// Drive: a round may be processed again once another round was processed after
+// it, and a repeat of the round processed last re-executes the same block; only
+// that round with another block, or with no block hash, is an error. The round
+// processed again can still be finalized.
+func TestProcessProposalRoundReprocessing(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	app := newKvApp(ctx, t, 1)
+	const height = 1
+	tx := []byte(testKey)
+	blockHash := []byte("block-hash")
+	process := func(round int32, hash []byte) (*types.ResponseProcessProposal, error) {
+		return app.ProcessProposal(ctx, &types.RequestProcessProposal{
+			Txs:     [][]byte{tx},
+			Hash:    hash,
+			Height:  height,
+			Round:   round,
+			Version: &pbversion.Consensus{App: uint64(height)},
+		})
+	}
+
+	first, err := process(0, blockHash)
+	require.NoError(t, err)
+	repeat, err := process(0, blockHash)
+	require.NoError(t, err, "same round and block again")
+	require.Equal(t, first.AppHash, repeat.AppHash)
+	_, err = process(0, []byte("other-block"))
+	require.ErrorContains(t, err, "duplicate ProcessProposal call", "same round with another block")
+	_, err = process(0, nil)
+	require.ErrorContains(t, err, "duplicate ProcessProposal call", "same round without a block hash")
+
+	_, err = process(1, blockHash)
+	require.NoError(t, err)
+	again, err := process(0, blockHash)
+	require.NoError(t, err, "round 0 again after round 1 was processed")
+	require.Equal(t, first.AppHash, again.AppHash)
+	_, err = process(0, blockHash)
+	require.NoError(t, err, "same round and block again after reprocessing")
+
+	reqFin := &types.RequestFinalizeBlock{Height: height, Round: 0}
+	reqFin.Block, reqFin.BlockID = makeBlock(t, height, [][]byte{tx}, again.AppHash)
+	_, err = app.FinalizeBlock(ctx, reqFin)
+	require.NoError(t, err)
+}