Add signing fix, expands nonce tests

modules/nonce/tx.go

@@ -46,14 +46,22 @@ func (n Tx) Wrap() basecoin.Tx {
 	return basecoin.Tx{n}
 }
 func (n Tx) ValidateBasic() error {
+	// rigel: check if Sequence ==  0, len(Signers) == 0, or Tx.Empty()
+	// these are all invalid, regardless of the state
+	// (also add max sequence number to prevent overflow?)
 	return n.Tx.ValidateBasic()
 }
 
 // CheckIncrementSeq - XXX fill in
 func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error {
 
+	// rigel: nice with the sort, problem is this modifies the TX in place...
+	// if we reserialize the tx after this function, it will be a different
+	// representations... copy n.Signers before sorting them please
+
 	//Generate the sequence key as the hash of the list of signers, sorted by address
 	sort.Sort(basecoin.ByAddress(n.Signers))
+	// rigel: nice sort, no need for a merkle hash... something simpler also works
 	seqKey := merkle.SimpleHashFromBinary(n.Signers)
 
 	// check the current state
@@ -72,6 +80,10 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error {
 		}
 	}
 
+	// rigel: this should be separate.  we check the sequence on CheckTx and DeliverTx
+	// BEFORE we execute the wrapped tx.
+	// we increment the sequence in DeliverTx AFTER it returns success (not on error)
+
 	//finally increment the sequence by 1
 	err = setSeq(store, seqKey, cur+1)
 	if err != nil {

modules/nonce/tx_test.go

@@ -9,8 +9,6 @@ import (
 	"github.com/tendermint/basecoin"
 	"github.com/tendermint/basecoin/stack"
 	"github.com/tendermint/basecoin/state"
-
-	"github.com/tendermint/tmlibs/log"
 )
 
 func TestNonce(t *testing.T) {
@@ -20,41 +18,75 @@ func TestNonce(t *testing.T) {
 	// generic args here...
 	chainID := "my-chain"
 	height := uint64(100)
-	ctx := stack.NewContext(chainID, height, log.NewNopLogger())
+	// rigel: use MockContext, so we can add permissions
+	ctx := stack.MockContext(chainID, height)
 	store := state.NewMemKVStore()
 
+	// rigel: you can leave chainID blank for the actors, note the comment on Actor struct
 	act1 := basecoin.Actor{ChainID: chainID, App: "fooz", Address: []byte{1, 2, 3, 4}}
 	act2 := basecoin.Actor{ChainID: chainID, App: "fooz", Address: []byte{1, 1, 1, 1}}
 	act3 := basecoin.Actor{ChainID: chainID, App: "fooz", Address: []byte{3, 3, 3, 3}}
 
+	// let's construct some tests to make the table a bit less verbose
+	set0 := []basecoin.Actor{}
+	set1 := []basecoin.Actor{act1}
+	set2 := []basecoin.Actor{act2}
+	set12 := []basecoin.Actor{act1, act2}
+	set21 := []basecoin.Actor{act2, act1}
+	set123 := []basecoin.Actor{act1, act2, act3}
+	set321 := []basecoin.Actor{act3, act2, act1}
+
+	// rigel: test cases look good, but also add reordering
 	testList := []struct {
 		valid  bool
 		seq    uint32
 		actors []basecoin.Actor
+		// rigel: you forgot to sign the tx, of course the get rejected...
+		signers []basecoin.Actor
 	}{
-		{false, 0, []basecoin.Actor{act1}},
+		// one signer
-		{true, 1, []basecoin.Actor{act1}},
+		{false, 0, set1, set1},   // seq 0 is no good
-		{false, 777, []basecoin.Actor{act1}},
+		{false, 1, set1, set0},   // sig is required
-		{true, 2, []basecoin.Actor{act1}},
+		{true, 1, set1, set1},    // sig and seq are good
-		{false, 0, []basecoin.Actor{act1, act2}},
+		{true, 2, set1, set1},    // increments each time
-		{true, 1, []basecoin.Actor{act1, act2}},
+		{false, 777, set1, set1}, // seq is too high
-		{true, 2, []basecoin.Actor{act1, act2}},
+
-		{true, 3, []basecoin.Actor{act1, act2}},
+		// independent from second signer
-		{false, 2, []basecoin.Actor{act1, act2}},
+		{false, 1, set2, set1},  // sig must match
-		{false, 2, []basecoin.Actor{act1, act2, act3}},
+		{true, 1, set2, set2},   // seq of set2 independent from set1
-		{true, 1, []basecoin.Actor{act1, act2, act3}},
+		{true, 2, set2, set321}, // extra sigs don't change the situation
+
+		// multisig has same requirements
+		{false, 0, set12, set12}, // need valid sequence number
+		{false, 1, set12, set2},  // they all must sign
+		{true, 1, set12, set12},  // this is proper, independent of act1 and act2
+		{true, 2, set21, set21},  // order of actors doesn't matter
+		{false, 2, set12, set12}, // but can't repeat sequence
+		{true, 3, set12, set321}, // no effect from extra sigs
+
+		// tripple sigs also work
+		{false, 2, set123, set123}, // must start with seq=1
+		{false, 1, set123, set12},  // all must sign
+		{true, 1, set123, set321},  // this works
+		{true, 2, set321, set321},  // other order is the same
+		{false, 2, set321, set321}, // no repetition
 	}
 
-	for _, test := range testList {
+	// rigel: don't wrap nil, it's bad, wrap a raw byte thing
+	raw := stack.NewRawTx([]byte{42})
+	for i, test := range testList {
+		// rigel: set the permissions
+		myCtx := ctx.WithPermissions(test.signers...)
 
-		tx := NewTx(test.seq, test.actors, basecoin.Tx{})
+		tx := NewTx(test.seq, test.actors, raw)
 		nonceTx, ok := tx.Unwrap().(Tx)
 		require.True(ok)
-		err := nonceTx.CheckIncrementSeq(ctx, store)
+
+		err := nonceTx.CheckIncrementSeq(myCtx, store)
 		if test.valid {
-			assert.Nil(err, "%v,%v: %+v", test.seq, test.actors, err)
+			assert.Nil(err, "%d: %+v", i, err)
 		} else {
-			assert.NotNil(err, "%v,%v", test.seq, test.actors)
+			assert.NotNil(err, "%d", i)
 		}
 	}
 }