diff --git a/internal/state/execution_test.go b/internal/state/execution_test.go index 41d7ae27c..2a24e9cca 100644 --- a/internal/state/execution_test.go +++ b/internal/state/execution_test.go @@ -690,8 +690,7 @@ func TestEmptyPrepareProposal(t *testing.T) { // TestPrepareProposalErrorOnNonExistingRemoved tests that the block creation logic returns // an error if the ResponsePrepareProposal returned from the application marks -// -// a transaction as REMOVED that was not present in the original proposal. +// a transaction that would cause the block to exceed size limits func TestPrepareProposalErrorOnNonExistingRemoved(t *testing.T) { const height = 2 ctx, cancel := context.WithCancel(context.Background()) @@ -702,22 +701,26 @@ func TestPrepareProposalErrorOnNonExistingRemoved(t *testing.T) { require.NoError(t, eventBus.Start(ctx)) state, stateDB, privVals := makeState(t, 1, height) + // Set a reasonable max block size that can accommodate header/commit/evidence + // We don't have to set this in the test, but it's good to have it here for reference + state.ConsensusParams.Block.MaxBytes = 1000 stateStore := sm.NewStore(stateDB) evpool := &mocks.EvidencePool{} evpool.On("PendingEvidence", mock.Anything).Return([]types.Evidence{}, int64(0)) + // Create a transaction that's too large + maxDataBytes := types.MaxDataBytes(state.ConsensusParams.Block.MaxBytes, 0, 1) + bigTx := make([]byte, maxDataBytes+1) mp := &mpmocks.Mempool{} mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(types.Txs{}) app := abcimocks.NewApplication(t) - - // create an invalid ResponsePrepareProposal rpp := &abci.ResponsePrepareProposal{ TxRecords: []*abci.TxRecord{ { Action: abci.TxRecord_UNMODIFIED, - Tx: []byte("new tx"), + Tx: bigTx, }, }, } @@ -741,7 +744,7 @@ func TestPrepareProposalErrorOnNonExistingRemoved(t *testing.T) { pa, _ := state.Validators.GetByIndex(0) commit, _ := makeValidCommit(ctx, t, height, types.BlockID{}, state.Validators, privVals) block, err := blockExec.CreateProposalBlock(ctx, height, state, commit, pa) - require.ErrorContains(t, err, "new transaction incorrectly marked as removed") + require.ErrorContains(t, err, "transaction data size exceeds maximum") require.Nil(t, block) mp.AssertExpectations(t) diff --git a/types/tx.go b/types/tx.go index 5b7c2377e..b3fe14516 100644 --- a/types/tx.go +++ b/types/tx.go @@ -159,23 +159,11 @@ func (t TxRecordSet) RemovedTxs() []Tx { // Validate checks that the record set was correctly constructed from the original // list of transactions. -func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error { +func (t TxRecordSet) Validate(maxSizeBytes int64, _ Txs) error { if len(t.unknown) > 0 { return fmt.Errorf("%d transactions marked unknown (first unknown hash: %x)", len(t.unknown), t.unknown[0].Hash()) } - // The following validation logic performs a set of sorts on the data in the TxRecordSet indexes. - // It sorts the original transaction list, otxs, once. - // It sorts the new transaction list twice: once when sorting 'all', the total list, - // and once by sorting the set of the added, removed, and unmodified transactions indexes, - // which, when combined, comprise the complete list of modified transactions. - // - // Each of the added, removed, and unmodified indices is then iterated and once - // and each value index is checked against the sorted original list for containment. - // Asymptotically, this yields a total runtime of O(N*log(N) + 2*M*log(M) + M*log(N)). - // in the input size of the original list, N, and the input size of the new list, M, respectively. - // Performance gains are likely possible, but this was preferred for readability and maintainability. - // Sort a copy of the complete transaction slice so we can check for // duplication. The copy is so we do not change the original ordering. // Only the slices are copied, the transaction contents are shared. @@ -188,34 +176,15 @@ func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error { } } - // create copies of each of the action-specific indexes so that order of the original - // indexes can be preserved. - addedCopy := sortedCopy(t.added) - removedCopy := sortedCopy(t.removed) - unmodifiedCopy := sortedCopy(t.unmodified) - + // Check total size of included transactions var size int64 - for _, cur := range append(unmodifiedCopy, addedCopy...) { + for _, cur := range append(t.unmodified, t.added...) { size += int64(len(cur)) if size > maxSizeBytes { return fmt.Errorf("transaction data size exceeds maximum %d", maxSizeBytes) } } - // make a defensive copy of otxs so that the order of - // the caller's data is not altered. - otxsCopy := sortedCopy(otxs) - - if ix, ok := containsAll(otxsCopy, unmodifiedCopy); !ok { - return fmt.Errorf("new transaction incorrectly marked as removed, transaction hash: %x", unmodifiedCopy[ix].Hash()) - } - - if ix, ok := containsAll(otxsCopy, removedCopy); !ok { - return fmt.Errorf("new transaction incorrectly marked as removed, transaction hash: %x", removedCopy[ix].Hash()) - } - if ix, ok := containsAny(otxsCopy, addedCopy); ok { - return fmt.Errorf("existing transaction incorrectly marked as added, transaction hash: %x", addedCopy[ix].Hash()) - } return nil } diff --git a/types/tx_test.go b/types/tx_test.go index bdbd003b9..ae7207fe9 100644 --- a/types/tx_test.go +++ b/types/tx_test.go @@ -49,16 +49,37 @@ func TestTxIndexByHash(t *testing.T) { } func TestValidateTxRecordSet(t *testing.T) { - t.Run("should error on new transactions marked UNMODIFIED", func(t *testing.T) { + t.Run("should error when total size exceeds max bytes", func(t *testing.T) { + // Create a transaction that exceeds the max size + bigTx := make([]byte, 101) // 101 bytes trs := []*abci.TxRecord{ { Action: abci.TxRecord_UNMODIFIED, - Tx: Tx([]byte{1, 2, 3, 4, 5}), + Tx: Tx(bigTx), + }, + } + txrSet := NewTxRecordSet(trs) + err := txrSet.Validate(100, []Tx{}) // Max 100 bytes + require.Error(t, err) + require.Contains(t, err.Error(), "transaction data size exceeds maximum") + }) + + t.Run("should error on duplicate transactions", func(t *testing.T) { + tx := Tx([]byte{1, 2, 3, 4, 5}) + trs := []*abci.TxRecord{ + { + Action: abci.TxRecord_UNMODIFIED, + Tx: tx, + }, + { + Action: abci.TxRecord_UNMODIFIED, + Tx: tx, // Same transaction }, } txrSet := NewTxRecordSet(trs) err := txrSet.Validate(100, []Tx{}) require.Error(t, err) + require.Contains(t, err.Error(), "found duplicate transaction") }) }