diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 09d5be6c9..2aa3aeb2e 100644 --- a/modules/nonce/tx.go +++ b/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 { diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index faafba216..9800d8ad1 100644 --- a/modules/nonce/tx_test.go +++ b/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}}, - {true, 1, []basecoin.Actor{act1}}, - {false, 777, []basecoin.Actor{act1}}, - {true, 2, []basecoin.Actor{act1}}, - {false, 0, []basecoin.Actor{act1, act2}}, - {true, 1, []basecoin.Actor{act1, act2}}, - {true, 2, []basecoin.Actor{act1, act2}}, - {true, 3, []basecoin.Actor{act1, act2}}, - {false, 2, []basecoin.Actor{act1, act2}}, - {false, 2, []basecoin.Actor{act1, act2, act3}}, - {true, 1, []basecoin.Actor{act1, act2, act3}}, + // one signer + {false, 0, set1, set1}, // seq 0 is no good + {false, 1, set1, set0}, // sig is required + {true, 1, set1, set1}, // sig and seq are good + {true, 2, set1, set1}, // increments each time + {false, 777, set1, set1}, // seq is too high + + // independent from second signer + {false, 1, set2, set1}, // sig must match + {true, 1, set2, set2}, // seq of set2 independent from set1 + {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) } } }