From dd0a39c05a5d7c6209f8673a456415dc6cb42ac1 Mon Sep 17 00:00:00 2001 From: Rootul P Date: Tue, 16 Jan 2024 13:49:26 -0500 Subject: [PATCH] feat!: reject proposals with un-decodable txs for appVersion two and above (#3006) Closes https://github.com/celestiaorg/celestia-app/issues/2663 Note: specs don't need to be updated because they already state https://github.com/celestiaorg/celestia-app/blob/dc8b7f5106cd1498d517f0d39a707fb40c0799cc/specs/src/specs/block_validity_rules.md?plain=1#L44 --------- Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com> --- app/process_proposal.go | 13 +++++++--- app/test/process_proposal_test.go | 41 +++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/app/process_proposal.go b/app/process_proposal.go index 8a2fe77a..c14cbca2 100644 --- a/app/process_proposal.go +++ b/app/process_proposal.go @@ -6,6 +6,7 @@ import ( "time" "github.com/celestiaorg/celestia-app/app/ante" + v1 "github.com/celestiaorg/celestia-app/pkg/appconsts/v1" "github.com/celestiaorg/celestia-app/pkg/blob" "github.com/celestiaorg/celestia-app/pkg/da" "github.com/celestiaorg/celestia-app/pkg/shares" @@ -58,10 +59,14 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp sdkTx, err := app.txConfig.TxDecoder()(tx) if err != nil { - // we don't reject the block here because it is not a block validity - // rule that all transactions included in the block data are - // decodable - continue + if req.Header.Version.App == v1.Version { + // For appVersion 1, there was no block validity rule that all + // transactions must be decodable. + continue + } + // An error here means that a tx was included in the block that is not decodable. + logInvalidPropBlock(app.Logger(), req.Header, fmt.Sprintf("tx %d is not decodable", idx)) + return reject() } // handle non-blob transactions first diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go index 26ebeacc..9e196b89 100644 --- a/app/test/process_proposal_test.go +++ b/app/test/process_proposal_test.go @@ -11,11 +11,14 @@ import ( abci "github.com/tendermint/tendermint/abci/types" tmrand "github.com/tendermint/tendermint/libs/rand" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + "github.com/tendermint/tendermint/proto/tendermint/version" coretypes "github.com/tendermint/tendermint/types" "github.com/celestiaorg/celestia-app/app" "github.com/celestiaorg/celestia-app/app/encoding" "github.com/celestiaorg/celestia-app/pkg/appconsts" + v1 "github.com/celestiaorg/celestia-app/pkg/appconsts/v1" + v2 "github.com/celestiaorg/celestia-app/pkg/appconsts/v2" "github.com/celestiaorg/celestia-app/pkg/blob" "github.com/celestiaorg/celestia-app/pkg/da" appns "github.com/celestiaorg/celestia-app/pkg/namespace" @@ -83,6 +86,7 @@ func TestProcessProposal(t *testing.T) { name string input *tmproto.Data mutator func(*tmproto.Data) + appVersion uint64 expectedResult abci.ResponseProcessProposal_Result } @@ -91,6 +95,7 @@ func TestProcessProposal(t *testing.T) { name: "valid untouched data", input: validData(), mutator: func(d *tmproto.Data) {}, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_ACCEPT, }, { @@ -99,6 +104,7 @@ func TestProcessProposal(t *testing.T) { mutator: func(d *tmproto.Data) { d.Txs = d.Txs[1:] }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -107,6 +113,7 @@ func TestProcessProposal(t *testing.T) { mutator: func(d *tmproto.Data) { d.Txs = append(d.Txs, blobTxs[3]) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -123,6 +130,7 @@ func TestProcessProposal(t *testing.T) { blobTxBytes, _ := blobTx.Marshal() d.Txs[0] = blobTxBytes }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -139,6 +147,7 @@ func TestProcessProposal(t *testing.T) { blobTxBytes, _ := blobTx.Marshal() d.Txs[0] = blobTxBytes }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -155,6 +164,7 @@ func TestProcessProposal(t *testing.T) { blobTxBytes, _ := blobTx.Marshal() d.Txs[0] = blobTxBytes }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -171,6 +181,7 @@ func TestProcessProposal(t *testing.T) { blobTxBytes, _ := blobTx.Marshal() d.Txs[0] = blobTxBytes }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -187,6 +198,7 @@ func TestProcessProposal(t *testing.T) { blobTxBytes, _ := blobTx.Marshal() d.Txs[0] = blobTxBytes }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -199,6 +211,7 @@ func TestProcessProposal(t *testing.T) { d.Txs[0] = blobTxBytes d.Hash = calculateNewDataHash(t, d.Txs) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -216,6 +229,7 @@ func TestProcessProposal(t *testing.T) { // Erasure code the data to update the data root so this doesn't doesn't fail on an incorrect data root. d.Hash = calculateNewDataHash(t, d.Txs) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -225,6 +239,7 @@ func TestProcessProposal(t *testing.T) { // swapping the order will cause the data root to be different d.Txs[0], d.Txs[1], d.Txs[2] = d.Txs[1], d.Txs[2], d.Txs[0] }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -234,14 +249,29 @@ func TestProcessProposal(t *testing.T) { btx, _ := coretypes.UnmarshalBlobTx(blobTxs[3]) d.Txs = append(d.Txs, btx.Tx) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { - name: "undecodable tx", + name: "undecodable tx with app version 1", input: validData(), mutator: func(d *tmproto.Data) { - d.Txs = append(d.Txs, tmrand.Bytes(300)) + d.Txs = append([][]byte{tmrand.Bytes(300)}, d.Txs...) + // Update the data hash so that the test doesn't fail due to an incorrect data root. + d.Hash = calculateNewDataHash(t, d.Txs) + }, + appVersion: v1.Version, + expectedResult: abci.ResponseProcessProposal_ACCEPT, + }, + { + name: "undecodable tx with app version 2", + input: validData(), + mutator: func(d *tmproto.Data) { + d.Txs = append([][]byte{tmrand.Bytes(300)}, d.Txs...) + // Update the data hash so that the test doesn't fail due to an incorrect data root. + d.Hash = calculateNewDataHash(t, d.Txs) }, + appVersion: v2.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -251,6 +281,7 @@ func TestProcessProposal(t *testing.T) { // swap txs at index 2 and 3 (essentially swapping a PFB with a normal tx) d.Txs[3], d.Txs[2] = d.Txs[2], d.Txs[3] }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -260,6 +291,7 @@ func TestProcessProposal(t *testing.T) { d.Txs = append(d.Txs, badSigBlobTx) d.Hash = calculateNewDataHash(t, d.Txs) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -269,6 +301,7 @@ func TestProcessProposal(t *testing.T) { d.Txs = append(d.Txs, blobTxWithInvalidNonce) d.Hash = calculateNewDataHash(t, d.Txs) }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, { @@ -295,6 +328,7 @@ func TestProcessProposal(t *testing.T) { // square with a tampered sequence start indicator d.Hash = dah.Hash() }, + appVersion: v1.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, } @@ -318,6 +352,9 @@ func TestProcessProposal(t *testing.T) { Height: 1, DataHash: resp.BlockData.Hash, ChainID: testutil.ChainID, + Version: version.Consensus{ + App: tt.appVersion, + }, }, }) assert.Equal(t, tt.expectedResult, res.Result, fmt.Sprintf("expected %v, got %v", tt.expectedResult, res.Result))