Sync PrepareProposal with Spec. Main part (#9158)

* ----start----

* [PARTIAL cherry-pick] ABCI Vote Extension 2 (#6885)

* Cherry-picked #6567: state/types: refactor makeBlock, makeBlocks and makeTxs (#6567)

* [Cherrypicked] types: remove panic from block methods (#7501)

* [cherrypicked] abci++: synchronize PrepareProposal with the newest version of the spec (#8094)

This change implements the logic for the PrepareProposal ABCI++ method call. The main logic for creating and issuing the PrepareProposal request lives in execution.go and is tested in a set of new tests in execution_test.go. This change also updates the mempool mock to use a mockery generated version and removes much of the plumbing for the no longer used ABCIResponses.

* make proto-gen

* Backported EvidenceList's method ToABCI from #7961

* make build

* Fix mockery for Mempool

* mockery

* Backported abci Application mocks from #7961

* mockery2

* Fixed new PrepareProposal test cases in state/execution_test.go

* Fixed returned errors in consensus/state.go

* lint

* Addressed @cmwaters' comment

Co-authored-by: mconcat <monoidconcat@gmail.com>
Co-authored-by: JayT106 <JayT106@users.noreply.github.com>
Co-authored-by: Sam Kleinman <garen@tychoish.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

types/block.go

@@ -137,22 +137,22 @@ func (b *Block) Hash() tmbytes.HexBytes {
 // MakePartSet returns a PartSet containing parts of a serialized block.
 // This is the form in which the block is gossipped to peers.
 // CONTRACT: partSize is greater than zero.
-func (b *Block) MakePartSet(partSize uint32) *PartSet {
+func (b *Block) MakePartSet(partSize uint32) (*PartSet, error) {
 	if b == nil {
-		return nil
+		return nil, errors.New("nil block")
 	}
 	b.mtx.Lock()
 	defer b.mtx.Unlock()
 
 	pbb, err := b.ToProto()
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
 	bz, err := proto.Marshal(pbb)
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
-	return NewPartSetFromData(bz, partSize)
+	return NewPartSetFromData(bz, partSize), nil
 }
 
 // HashesTo is a convenience function that checks if a block hashes to the given argument.

types/block_test.go

@@ -110,15 +110,20 @@ func TestBlockHash(t *testing.T) {
 }
 
 func TestBlockMakePartSet(t *testing.T) {
-	assert.Nil(t, (*Block)(nil).MakePartSet(2))
+	bps, err := (*Block)(nil).MakePartSet(2)
+	assert.Error(t, err)
+	assert.Nil(t, bps)
 
-	partSet := MakeBlock(int64(3), []Tx{Tx("Hello World")}, nil, nil).MakePartSet(1024)
+	partSet, err := MakeBlock(int64(3), []Tx{Tx("Hello World")}, nil, nil).MakePartSet(1024)
+	require.NoError(t, err)
 	assert.NotNil(t, partSet)
 	assert.EqualValues(t, 1, partSet.Total())
 }
 
 func TestBlockMakePartSetWithEvidence(t *testing.T) {
-	assert.Nil(t, (*Block)(nil).MakePartSet(2))
+	bps, err := (*Block)(nil).MakePartSet(2)
+	assert.Error(t, err)
+	assert.Nil(t, bps)
 
 	lastID := makeBlockIDRandom()
 	h := int64(3)
@@ -130,7 +135,9 @@ func TestBlockMakePartSetWithEvidence(t *testing.T) {
 	ev := NewMockDuplicateVoteEvidenceWithValidator(h, time.Now(), vals[0], "block-test-chain")
 	evList := []Evidence{ev}
 
-	partSet := MakeBlock(h, []Tx{Tx("Hello World")}, commit, evList).MakePartSet(512)
+	partSet, err := MakeBlock(h, []Tx{Tx("Hello World")}, commit, evList).MakePartSet(512)
+	require.NoError(t, err)
+
 	assert.NotNil(t, partSet)
 	assert.EqualValues(t, 4, partSet.Total())
 }

types/evidence.go

@@ -459,6 +459,16 @@ func (evl EvidenceList) Has(evidence Evidence) bool {
 	return false
 }
 
+// ToABCI converts the evidence list to a slice of the ABCI protobuf messages
+// for use when communicating the evidence to an application.
+func (evl EvidenceList) ToABCI() []abci.Evidence {
+	var el []abci.Evidence
+	for _, e := range evl {
+		el = append(el, e.ABCI()...)
+	}
+	return el
+}
+
 //------------------------------------------ PROTO --------------------------------------
 
 // EvidenceToProto is a generalized function for encoding evidence that conforms to the

types/evidence_test.go

@@ -245,9 +245,8 @@ func makeVote(
 
 	vpb := v.ToProto()
 	err = val.SignVote(chainID, vpb)
-	if err != nil {
-		panic(err)
-	}
+	require.NoError(t, err)
+
 	v.Signature = vpb.Signature
 	return v
 }

types/tx.go

@@ -5,7 +5,9 @@ import (
 	"crypto/sha256"
 	"errors"
 	"fmt"
+	"sort"
 
+	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto/merkle"
 	"github.com/tendermint/tendermint/crypto/tmhash"
 	tmbytes "github.com/tendermint/tendermint/libs/bytes"
@@ -45,13 +47,8 @@ type Txs []Tx
 // Hash returns the Merkle root hash of the transaction hashes.
 // i.e. the leaves of the tree are the hashes of the txs.
 func (txs Txs) Hash() []byte {
-	// These allocations will be removed once Txs is switched to [][]byte,
-	// ref #2603. This is because golang does not allow type casting slices without unsafe
-	txBzs := make([][]byte, len(txs))
-	for i := 0; i < len(txs); i++ {
-		txBzs[i] = txs[i].Hash()
-	}
-	return merkle.HashFromByteSlices(txBzs)
+	hl := txs.hashList()
+	return merkle.HashFromByteSlices(hl)
 }
 
 // Index returns the index of this transaction in the list, or -1 if not found
@@ -74,16 +71,9 @@ func (txs Txs) IndexByHash(hash []byte) int {
 	return -1
 }
 
-// Proof returns a simple merkle proof for this node.
-// Panics if i < 0 or i >= len(txs)
-// TODO: optimize this!
 func (txs Txs) Proof(i int) TxProof {
-	l := len(txs)
-	bzs := make([][]byte, l)
-	for i := 0; i < l; i++ {
-		bzs[i] = txs[i].Hash()
-	}
-	root, proofs := merkle.ProofsFromByteSlices(bzs)
+	hl := txs.hashList()
+	root, proofs := merkle.ProofsFromByteSlices(hl)
 
 	return TxProof{
 		RootHash: root,
@@ -92,11 +82,23 @@ func (txs Txs) Proof(i int) TxProof {
 	}
 }
 
+func (txs Txs) hashList() [][]byte {
+	hl := make([][]byte, len(txs))
+	for i := 0; i < len(txs); i++ {
+		hl[i] = txs[i].Hash()
+	}
+	return hl
+}
+
+// Txs is a slice of transactions. Sorting a Txs value orders the transactions
+// lexicographically.
+func (txs Txs) Len() int      { return len(txs) }
+func (txs Txs) Swap(i, j int) { txs[i], txs[j] = txs[j], txs[i] }
+func (txs Txs) Less(i, j int) bool {
+	return bytes.Compare(txs[i], txs[j]) == -1
+}
+
 // ToSliceOfBytes converts a Txs to slice of byte slices.
-//
-// NOTE: This method should become obsolete once Txs is switched to [][]byte.
-// ref: #2603
-// TODO This function is to disappear when TxRecord is introduced
 func (txs Txs) ToSliceOfBytes() [][]byte {
 	txBzs := make([][]byte, len(txs))
 	for i := 0; i < len(txs); i++ {
@@ -105,13 +107,182 @@ func (txs Txs) ToSliceOfBytes() [][]byte {
 	return txBzs
 }
 
-// ToTxs converts a raw slice of byte slices into a Txs type.
-func ToTxs(txs [][]byte) Txs {
-	txBzs := make(Txs, len(txs))
-	for i := 0; i < len(txs); i++ {
-		txBzs[i] = txs[i]
+// TxRecordSet contains indexes into an underlying set of transactions.
+// These indexes are useful for validating and working with a list of TxRecords
+// from the PrepareProposal response.
+//
+// Only one copy of the original data is referenced by all of the indexes but a
+// transaction may appear in multiple indexes.
+type TxRecordSet struct {
+	// all holds the complete list of all transactions from the original list of
+	// TxRecords.
+	all Txs
+
+	// included is an index of the transactions that will be included in the block
+	// and is constructed from the list of both added and unmodified transactions.
+	// included maintains the original order that the transactions were present
+	// in the list of TxRecords.
+	included Txs
+
+	// added, unmodified, removed, and unknown are indexes for each of the actions
+	// that may be supplied with a transaction.
+	//
+	// Because each transaction only has one action, it can be referenced by
+	// at most 3 indexes in this data structure: the action-specific index, the
+	// included index, and the all index.
+	added      Txs
+	unmodified Txs
+	removed    Txs
+	unknown    Txs
+}
+
+// NewTxRecordSet constructs a new set from the given transaction records.
+// The contents of the input transactions are shared by the set, and must not
+// be modified during the lifetime of the set.
+func NewTxRecordSet(trs []*abci.TxRecord) TxRecordSet {
+	txrSet := TxRecordSet{
+		all: make([]Tx, len(trs)),
 	}
-	return txBzs
+	for i, tr := range trs {
+
+		txrSet.all[i] = Tx(tr.Tx)
+
+		// The following set of assignments do not allocate new []byte, they create
+		// pointers to the already allocated slice.
+		switch tr.GetAction() {
+		case abci.TxRecord_UNKNOWN:
+			txrSet.unknown = append(txrSet.unknown, txrSet.all[i])
+		case abci.TxRecord_UNMODIFIED:
+			txrSet.unmodified = append(txrSet.unmodified, txrSet.all[i])
+			txrSet.included = append(txrSet.included, txrSet.all[i])
+		case abci.TxRecord_ADDED:
+			txrSet.added = append(txrSet.added, txrSet.all[i])
+			txrSet.included = append(txrSet.included, txrSet.all[i])
+		case abci.TxRecord_REMOVED:
+			txrSet.removed = append(txrSet.removed, txrSet.all[i])
+		}
+	}
+	return txrSet
+}
+
+// IncludedTxs returns the transactions marked for inclusion in a block. This
+// list maintains the order that the transactions were included in the list of
+// TxRecords that were used to construct the TxRecordSet.
+func (t TxRecordSet) IncludedTxs() []Tx {
+	return t.included
+}
+
+// AddedTxs returns the transactions added by the application.
+func (t TxRecordSet) AddedTxs() []Tx {
+	return t.added
+}
+
+// RemovedTxs returns the transactions marked for removal by the application.
+func (t TxRecordSet) RemovedTxs() []Tx {
+	return t.removed
+}
+
+// Validate checks that the record set was correctly constructed from the original
+// list of transactions.
+func (t TxRecordSet) Validate(maxSizeBytes int64, otxs 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.
+	allCopy := sortedCopy(t.all)
+
+	var size int64
+	for i, cur := range allCopy {
+		size += int64(len(cur))
+		if size > maxSizeBytes {
+			return fmt.Errorf("transaction data size %d exceeds maximum %d", size, maxSizeBytes)
+		}
+
+		// allCopy is sorted, so any duplicated data will be adjacent.
+		if i+1 < len(allCopy) && bytes.Equal(cur, allCopy[i+1]) {
+			return fmt.Errorf("found duplicate transaction with hash: %x", cur.Hash())
+		}
+	}
+
+	// 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)
+
+	// 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
+}
+
+func sortedCopy(txs Txs) Txs {
+	cp := make(Txs, len(txs))
+	copy(cp, txs)
+	sort.Sort(cp)
+	return cp
+}
+
+// containsAny checks that list a contains one of the transactions in list
+// b. If a match is found, the index in b of the matching transaction is returned.
+// Both lists must be sorted.
+func containsAny(a, b []Tx) (int, bool) {
+	for i, cur := range b {
+		if _, ok := contains(a, cur); ok {
+			return i, true
+		}
+	}
+	return -1, false
+}
+
+// containsAll checks that super contains all of the transactions in the sub
+// list. If not all values in sub are present in super, the index in sub of the
+// first Tx absent from super is returned.
+func containsAll(super, sub Txs) (int, bool) {
+	for i, cur := range sub {
+		if _, ok := contains(super, cur); !ok {
+			return i, false
+		}
+	}
+	return -1, true
+}
+
+// contains checks that the sorted list, set contains elem. If set does contain elem, then the
+// index in set of elem is returned.
+func contains(set []Tx, elem Tx) (int, bool) {
+	n := sort.Search(len(set), func(i int) bool {
+		return bytes.Compare(elem, set[i]) <= 0
+	})
+	if n == len(set) || !bytes.Equal(elem, set[n]) {
+		return -1, false
+	}
+	return n, true
 }
 
 // TxProof represents a Merkle proof of the presence of a transaction in the Merkle tree.

types/tx_test.go

@@ -2,11 +2,13 @@ package types
 
 import (
 	"bytes"
+	"math/rand"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	abci "github.com/tendermint/tendermint/abci/types"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
 	ctest "github.com/tendermint/tendermint/libs/test"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -20,11 +22,6 @@ func makeTxs(cnt, size int) Txs {
 	return txs
 }
 
-func randInt(low, high int) int {
-	off := tmrand.Int() % (high - low)
-	return low + off
-}
-
 func TestTxIndex(t *testing.T) {
 	for i := 0; i < 20; i++ {
 		txs := makeTxs(15, 60)
@@ -51,6 +48,160 @@ func TestTxIndexByHash(t *testing.T) {
 	}
 }
 
+func TestValidateTxRecordSet(t *testing.T) {
+	t.Run("should error on total transaction size exceeding max data size", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{6, 7, 8, 9, 10}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(9, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("should error on duplicate transactions with the same action", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{100}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{200}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("should error on duplicate transactions with mixed actions", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{100}),
+			},
+			{
+				Action: abci.TxRecord_REMOVED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{200}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("should error on new transactions marked UNMODIFIED", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_UNMODIFIED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("should error on new transactions marked REMOVED", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_REMOVED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("should error on existing transaction marked as ADDED", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{5, 4, 3, 2, 1}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{6}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{{0}, {1, 2, 3, 4, 5}})
+		require.Error(t, err)
+	})
+	t.Run("should error if any transaction marked as UNKNOWN", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_UNKNOWN,
+				Tx:     Tx([]byte{1, 2, 3, 4, 5}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.Error(t, err)
+	})
+	t.Run("TxRecordSet preserves order", func(t *testing.T) {
+		trs := []*abci.TxRecord{
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{100}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{99}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{55}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{12}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{66}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{9}),
+			},
+			{
+				Action: abci.TxRecord_ADDED,
+				Tx:     Tx([]byte{17}),
+			},
+		}
+		txrSet := NewTxRecordSet(trs)
+		err := txrSet.Validate(100, []Tx{})
+		require.NoError(t, err)
+		for i, tx := range txrSet.IncludedTxs() {
+			require.Equal(t, Tx(trs[i].Tx), tx)
+		}
+	})
+}
+
 func TestValidTxProof(t *testing.T) {
 	cases := []struct {
 		txs Txs
@@ -149,3 +300,7 @@ func assertBadProof(t *testing.T, root []byte, bad []byte, good TxProof) {
 		}
 	}
 }
+
+func randInt(low, high int) int {
+	return rand.Intn(high-low) + low
+}

types/vote_set.go

@@ -227,6 +227,9 @@ func (voteSet *VoteSet) getVote(valIndex int32, blockKey string) (vote *Vote, ok
 }
 
 func (voteSet *VoteSet) GetVotes() []*Vote {
+	if voteSet == nil {
+		return nil
+	}
 	return voteSet.votes
 }
 
@@ -631,6 +634,7 @@ func (voteSet *VoteSet) MakeCommit() *Commit {
 		if commitSig.ForBlock() && !v.BlockID.Equals(*voteSet.maj23) {
 			commitSig = NewCommitSigAbsent()
 		}
+
 		commitSigs[i] = commitSig
 	}