[Bug]: Consensus Failure in Ignoring Malformed Transactions

[davidterpay]

Summary of Bug

Currently FinalizeBlock will ignore any bytes (i.e. txs) included in a proposal that cannot be decoded by base app's TxDecoder. Although this makes sense since Vote Extensions will be included in proposals with ABCI 2.0, this will trigger a consensus failure as the number of deliverTx responses that Comet expects will never match the number of transactions in the proposal.

Lines of code that are of interest are below:

cosmos-sdk/baseapp/abci.go

Lines 674 to 686 in 2100a73

	// Iterate over all raw transactions in the proposal and attempt to execute
	// them, gathering the execution results.
	//
	// NOTE: Not all raw transactions may adhere to the sdk.Tx interface, e.g.
	// vote extensions, so skip those.
	txResults := make([]*abci.ExecTxResult, 0, len(req.Txs))
	for _, rawTx := range req.Txs {
	if _, err := app.txDecoder(rawTx); err == nil {
	txResults = append(txResults, app.deliverTx(rawTx))
	}
	}

Version

The version we have been testing on is Cosmos SDK version v0.50.0-alpha.0.

Steps to Reproduce

We have a private repository we can add individuals to where we can demonstrate the issue. Although any other developer team that is using this version and are injecting any txs that are not sdk.Txs will face the same issue.

Image below shows the consensus failure when we attempt to include a single vote extension in a proposal.

Proposal

In the case where a transaction cannot be decoded, a default deliverTx response can be returned. This can look like the following:

// MalformedTxResponse is the default DeliverTx response that is returned
// when a transaction included in a block proposal is malformed and
// should be ignored in FinalizeBlock.
var MalformedTxResponse = &abci.ExecTxResult{
	Log:       "malformed transaction cannot be processed in DeliverTx",
	GasWanted: 0,
	GasUsed:   0,
	Events:    nil,
	Data:      nil,
	Info:      "malformed transaction in deliverTx",
}
...
func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {
       ...

	// Iterate over all raw transactions in the proposal and attempt to execute
	// them, gathering the execution results.
	//
	// NOTE: Not all raw transactions may adhere to the sdk.Tx interface, e.g.
	// vote extensions, so skip those.
	txResults := make([]*abci.ExecTxResult, 0, len(req.Txs))
	for _, rawTx := range req.Txs {
		if _, err := app.txDecoder(rawTx); err == nil {
			txResults = append(txResults, app.deliverTx(rawTx))
		} else {
			txResults = append(txResults, MalformedTxResponse)
		}
	}


      ...
}

[alexanderbez]

Thanks for the amazing writeup @davidterpay! Given the context of the (limited) method of which we can gossip VE to all validators and that CometBFT ensures len(responses) == len(req.Txs), we have no choice but to include a "dummy" response for non-txs like you proposed.

I would however reconsider the nomenclature of MalformedTxResponse...I can't think of a better name so maybe we go with that.

This is an 0.50.x blocker. Wanna submit a PR @davidterpay?

[davidterpay]

Yup! Will respond with PR in this issue.

[davidterpay]

PR: #16700