protobuf decode tx patch (#583)

* fix protobuf bug

* clean up

* mispelling

* add more comments to a weird test

* Update types/tx.go

Co-authored-by: John Adler <[email protected]>

* better comment w/ todo and use prexisting hash constant

Co-authored-by: John Adler <[email protected]>

types/tx.go

@@ -165,14 +165,24 @@ func ComputeProtoSizeForTxs(txs []Tx) int64 {
 // transaction wrapper, if this an be done, then it returns true. A child
 // transaction is a normal transaction that has been derived from a different
 // parent transaction. The returned hash is that of the parent transaction,
-// which allows us to remove the parent transaction from the mempool
+// which allows us to remove the parent transaction from the mempool. NOTE:
+// protobuf sometimes does not throw an error if the transaction passed is not
+// a tmproto.ChildTx, since the schema for PayForMessage is kept in the app, we
+// cannot perform further checks without creating an import cycle.
 func DecodeChildTx(tx Tx) (hash []byte, unwrapped Tx, has bool) {
 	// attempt to unmarshal into a a child transaction
 	var childTx tmproto.ChildTx
 	err := proto.Unmarshal(tx, &childTx)
 	if err != nil {
 		return nil, nil, false
 	}
+	// this check will fail to catch unwanted types should those unmarshalled
+	// types happen to have a hash sized slice of bytes in the same field number
+	// as ParentTxHash. TODO(evan): either fix this, or better yet use a different
+	// mechanism
+	if len(childTx.ParentTxHash) != tmhash.Size {
+		return nil, nil, false
+	}
 	return childTx.ParentTxHash, childTx.Tx, true
 }
 

types/tx_test.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"bytes"
+	"crypto/sha256"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -149,3 +150,47 @@ func assertBadProof(t *testing.T, root []byte, bad []byte, good TxProof) {
 		}
 	}
 }
+
+func TestDecodeChildTx(t *testing.T) {
+	// perform a simple test for being unable to decode a non
+	// child transaction
+	tx := Tx{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0}
+	_, _, ok := DecodeChildTx(tx)
+	require.False(t, ok)
+
+	// create a proto message that used to be decoded when it shouldn't have
+	randomBlock := MakeBlock(
+		1,
+		[]Tx{tx},
+		nil,
+		nil,
+		[]Message{
+			{
+				NamespaceID: []byte{1, 2, 3, 4, 5, 6, 7, 8},
+				Data:        []byte{1, 2, 3, 4, 5, 6, 7, 8, 9},
+			},
+		},
+		&Commit{},
+	)
+
+	protoB, err := randomBlock.ToProto()
+	require.NoError(t, err)
+
+	rawBlock, err := protoB.Marshal()
+	require.NoError(t, err)
+
+	// due to protobuf not actually requiring type compatibility
+	// we need to make sure that there is some check
+	_, _, ok = DecodeChildTx(rawBlock)
+	require.False(t, ok)
+
+	pHash := sha256.Sum256(rawBlock)
+	childTx, err := WrapChildTx(pHash[:], rawBlock)
+	require.NoError(t, err)
+
+	// finally, ensure that the unwrapped bytes are identical to the input
+	unwrappedHash, unwrapped, ok := DecodeChildTx(childTx)
+	require.True(t, ok)
+	assert.Equal(t, 32, len(unwrappedHash))
+	require.Equal(t, rawBlock, []byte(unwrapped))
+}