From 9fbbdecb65a14b3ad082223d4fd914eab9229139 Mon Sep 17 00:00:00 2001 From: William Banfield Date: Tue, 8 Mar 2022 11:59:23 -0500 Subject: [PATCH] implement ResponsePrepareProposal validation rules --- abci/types/types.go | 47 ++++++++++++++++++- abci/types/types_test.go | 94 +++++++++++++++++++++++++++++++++++++ internal/state/execution.go | 8 ++-- internal/state/state.go | 25 ---------- 4 files changed, 144 insertions(+), 30 deletions(-) diff --git a/abci/types/types.go b/abci/types/types.go index c3bb7109f..dd266e1ce 100644 --- a/abci/types/types.go +++ b/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 +} diff --git a/abci/types/types_test.go b/abci/types/types_test.go index a35c56b5a..2f0377673 100644 --- a/abci/types/types_test.go +++ b/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) + }) +} diff --git a/internal/state/execution.go b/internal/state/execution.go index f38979c4e..9623e18a1 100644 --- a/internal/state/execution.go +++ b/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( diff --git a/internal/state/state.go b/internal/state/state.go index 111042326..c002bf065 100644 --- a/internal/state/state.go +++ b/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