Drop validating if record set was constructed from original transactions

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)

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
 }
 

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")
 	})
 }