implement ResponsePrepareProposal validation rules

abci/types/types.go

@@ -3,6 +3,8 @@ package types
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
+	"fmt"
 
 	"github.com/gogo/protobuf/jsonpb"
 
@@ -203,14 +205,57 @@ func mustResultsToByteSlices(r []*ExecTxResult) [][]byte {
 	return s
 }
 
+func (tr *TxRecord) IsIncluded() bool {
+	return tr.Action == TxRecord_ADDED || tr.Action == TxRecord_UNMODIFIED
+}
+
 // IncludedTxs returns all of the TxRecords that are marked for inclusion in the
 // proposed block.
 func (rpp *ResponsePrepareProposal) IncludedTxs() []*TxRecord {
 	trs := []*TxRecord{}
 	for _, tr := range rpp.TxRecords {
-		if tr.Action == TxRecord_ADDED || tr.Action == TxRecord_UNMODIFIED {
+		if tr.IsIncluded() {
 			trs = append(trs, tr)
 		}
 	}
 	return trs
 }
+
+// Validate checks that the fields of the ResponsePrepareProposal are properly
+// constructed. Validate returns an error if any of the validation checks fail.
+func (rpp *ResponsePrepareProposal) Validate(maxSizeBytes int64, otxs [][]byte) error {
+	// TODO: this feels like a large amount allocated data. We move all the Txs into strings
+	// in the map. The map will be as large as the original byte slice.
+	// Is there a key we can use?
+	otxsSet := make(map[string]struct{}, len(otxs))
+	for _, tx := range otxs {
+		otxsSet[string(tx)] = struct{}{}
+	}
+	ntx := map[string]struct{}{}
+	var size int64
+	for _, tr := range rpp.TxRecords {
+		if tr.IsIncluded() {
+			size += int64(len(tr.Tx))
+			if _, ok := ntx[string(tr.Tx)]; ok {
+				return errors.New("duplicate included transaction")
+			}
+			ntx[string(tr.Tx)] = struct{}{}
+		}
+		if _, ok := otxsSet[string(tr.Tx)]; ok {
+			if tr.Action == TxRecord_ADDED {
+				return fmt.Errorf("unmodified transaction incorrectly marked as %s", tr.Action.String())
+			}
+		} else {
+			if tr.Action == TxRecord_REMOVED || tr.Action == TxRecord_UNMODIFIED {
+				return fmt.Errorf("new transaction incorrectly marked as %s", tr.Action.String())
+			}
+		}
+		if tr.Action == TxRecord_UNKNOWN {
+			return fmt.Errorf("transaction incorrectly marked as %s", tr.Action.String())
+		}
+	}
+	if size > maxSizeBytes {
+		return fmt.Errorf("transaction data size %d exceeds maximum %d", size, maxSizeBytes)
+	}
+	return nil
+}

abci/types/types_test.go

@@ -1,6 +1,7 @@
 package types_test
 
 import (
+	fmt "fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -39,3 +40,96 @@ func TestHashAndProveResults(t *testing.T) {
 		assert.NoError(t, valid, "%d", i)
 	}
 }
+
+func TestValidateResponsePrepareProposal(t *testing.T) {
+	t.Run("should error on total transaction size exceeding max data size", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{6, 7, 8, 9, 10},
+				},
+			},
+		}
+		err := rpp.Validate(9, [][]byte{})
+		require.Error(t, err)
+	})
+	t.Run("should error on duplicate transactions", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{100},
+				},
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{200},
+				},
+			},
+		}
+		err := rpp.Validate(100, [][]byte{})
+		require.Error(t, err)
+	})
+	t.Run("should error on new transactions marked UNMODIFIED", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_UNMODIFIED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+			},
+		}
+		err := rpp.Validate(100, [][]byte{})
+		fmt.Println(err)
+		require.Error(t, err)
+	})
+	t.Run("should error on new transactions marked REMOVED", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_REMOVED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+			},
+		}
+		err := rpp.Validate(100, [][]byte{})
+		fmt.Println(err)
+		require.Error(t, err)
+	})
+	t.Run("should error on unmodified transaction marked as ADDED", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_ADDED,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+			},
+		}
+		err := rpp.Validate(100, [][]byte{{1, 2, 3, 4, 5}})
+		require.Error(t, err)
+	})
+	t.Run("should error if any transaction marked as UNKNOWN", func(t *testing.T) {
+		rpp := &abci.ResponsePrepareProposal{
+			TxRecords: []*abci.TxRecord{
+				{
+					Action: abci.TxRecord_UNKNOWN,
+					Tx:     []byte{1, 2, 3, 4, 5},
+				},
+			},
+		}
+		err := rpp.Validate(100, [][]byte{})
+		require.Error(t, err)
+	})
+}

internal/state/execution.go

@@ -117,7 +117,7 @@ func (blockExec *BlockExecutor) CreateProposalBlock(
 	block, err := state.MakeBlock(height, txs, commit, evidence, proposerAddr)
 
 	localLastCommit := buildLastCommitInfo(block, blockExec.store, state.InitialHeight)
-	preparedProposal, err := blockExec.appClient.PrepareProposal(
+	rpp, err := blockExec.appClient.PrepareProposal(
 		ctx,
 		abci.RequestPrepareProposal{
 			Hash:                block.Hash(),
@@ -141,11 +141,11 @@ func (blockExec *BlockExecutor) CreateProposalBlock(
 		// purpose for now.
 		panic(err)
 	}
-	if err := state.ValidateResponsePrepareProposal(preparedProposal); err != nil {
-		panic(fmt.Sprintf("application returned invalid ResponsePrepareProposal: %s", err))
+	if err := rpp.Validate(maxDataBytes, txs.ToSliceOfBytes()); err != nil {
+		return nil, err
 	}
 
-	return state.MakeBlock(height, types.TxRecordsToTxs(preparedProposal.IncludedTxs()), commit, evidence, proposerAddr)
+	return state.MakeBlock(height, types.TxRecordsToTxs(rpp.IncludedTxs()), commit, evidence, proposerAddr)
 }
 
 func (blockExec *BlockExecutor) ProcessProposal(

internal/state/state.go

@@ -283,31 +283,6 @@ func (state State) BlockFromResponsePrepareProposal(height int64, rpp *abci.Resp
 	return &types.Block{}, nil
 }
 
-func (state State) ValidateResponsePrepareProposal(rpp *abci.ResponsePrepareProposal) error {
-	rpp := abci.ResponsePrepareProposal{
-		ModifiedTx: false,
-		TxRecords:  []*abci.TxRecord{},
-
-		// TODO: What is this field for?
-		// Need to check that the size of appsigned does not grow I guess, says spec
-		AppSignedUpdates: [][]byte{},
-	}
-	// TODO: Implement logic to validate block.
-
-	// Checks:
-	// * Size does not exceed max size bytes
-	// * Check that there are no duplicate Tx in the list
-	// * Check that there are no new or modified transactions marked as 'removed' or 'unmodified'
-	// * Check that there are no unmodified transactions marked as txadded
-	// * no tx marked as Tx unknown
-	//
-	// Spec says to just forgo this validators turn to propose if failure
-	// Another option is to panic. I think fail to propose is fine, but we should
-	// make it clear that this occurs to operators.
-
-	return nil
-}
-
 //------------------------------------------------------------------------
 // Genesis