From 5ccf22bfb777c0626f2a0ff192911923499a2e83 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 04:37:22 -0400 Subject: [PATCH] frey changes --- errors/common.go | 19 ++++++++-- modules/nonce/replaycheck.go | 44 +++++++++++++++++++++-- modules/nonce/tx.go | 67 +++++++++++++++++++++++++----------- modules/nonce/tx_test.go | 22 +++++------- tests/cli/common.sh | 1 + 5 files changed, 114 insertions(+), 39 deletions(-) diff --git a/errors/common.go b/errors/common.go index bdce81d944..a6f37a71ab 100644 --- a/errors/common.go +++ b/errors/common.go @@ -15,12 +15,15 @@ var ( errUnauthorized = fmt.Errorf("Unauthorized") errInvalidSignature = fmt.Errorf("Invalid Signature") errTooLarge = fmt.Errorf("Input size too large") + errNoSigners = fmt.Errorf("There are no signers") errMissingSignature = fmt.Errorf("Signature missing") errTooManySignatures = fmt.Errorf("Too many signatures") errNoChain = fmt.Errorf("No chain id provided") errNoNonce = fmt.Errorf("Tx doesn't contain nonce") errNotMember = fmt.Errorf("nonce contains non-permissioned member") - errBadNonce = fmt.Errorf("Bad nonce seqence") + errBadNonce = fmt.Errorf("Bad nonce sequence") + errTxEmpty = fmt.Errorf("The provided Tx is empty") + errZeroSequence = fmt.Errorf("Sequence number cannot be zero") errWrongChain = fmt.Errorf("Wrong chain for tx") errUnknownTxType = fmt.Errorf("Tx type unknown") errInvalidFormat = fmt.Errorf("Invalid format") @@ -107,6 +110,10 @@ func IsUnauthorizedErr(err error) bool { return HasErrorCode(err, unauthorized) } +func ErrNoSigners() TMError { + return WithCode(errNoSigners, unauthorized) +} + func ErrMissingSignature() TMError { return WithCode(errMissingSignature, unauthorized) } @@ -139,12 +146,20 @@ func ErrNoNonce() TMError { return WithCode(errNoNonce, unauthorized) } func ErrBadNonce(got, expected uint32) TMError { - return WithCode(fmt.Errorf("Bad nonce seqence, got %d, expected %d", got, expected), unauthorized) + return WithCode(fmt.Errorf("Bad nonce sequence, got %d, expected %d", got, expected), unauthorized) } func ErrNotMember() TMError { return WithCode(errBadNonce, unauthorized) } +func ErrZeroSequence() TMError { + return WithCode(errZeroSequence, unauthorized) +} + +func ErrTxEmpty() TMError { + return WithCode(errTxEmpty, unauthorized) +} + func ErrWrongChain(chain string) TMError { msg := errors.Wrap(errWrongChain, chain) return WithCode(msg, unauthorized) diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go index 1412bfb165..8a401e79ae 100644 --- a/modules/nonce/replaycheck.go +++ b/modules/nonce/replaycheck.go @@ -32,7 +32,17 @@ func (r ReplayCheck) CheckTx(ctx basecoin.Context, store state.KVStore, if err != nil { return res, err } - return next.CheckTx(ctx, store, stx) + + res, err = next.CheckTx(ctx, store, stx) + if err != nil { + return res, err + } + + err = r.incrementNonceTx(ctx, store, tx) + if err != nil { + return res, err + } + return } // DeliverTx verifies tx is not being replayed - fulfills Middlware interface @@ -43,7 +53,17 @@ func (r ReplayCheck) DeliverTx(ctx basecoin.Context, store state.KVStore, if err != nil { return res, err } - return next.DeliverTx(ctx, store, stx) + + res, err = next.DeliverTx(ctx, store, stx) + if err != nil { + return res, err + } + + err = r.incrementNonceTx(ctx, store, tx) + if err != nil { + return res, err + } + return } // checkNonceTx varifies the nonce sequence @@ -57,9 +77,27 @@ func (r ReplayCheck) checkNonceTx(ctx basecoin.Context, store state.KVStore, } // check the nonce sequence number - err := nonceTx.CheckIncrementSeq(ctx, store) + err := nonceTx.CheckSeq(ctx, store) if err != nil { return tx, err } return nonceTx.Tx, nil } + +// incrementNonceTx increases the nonce sequence number +func (r ReplayCheck) incrementNonceTx(ctx basecoin.Context, store state.KVStore, + tx basecoin.Tx) error { + + // make sure it is a the nonce Tx (Tx from this package) + nonceTx, ok := tx.Unwrap().(Tx) + if !ok { + return errors.ErrNoNonce() + } + + // check the nonce sequence number + err := nonceTx.IncrementSeq(ctx, store) + if err != nil { + return err + } + return nil +} diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 2aa3aeb2ed..8e7df262b9 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -1,5 +1,9 @@ /* -Package nonce XXX +Package nonce - This module allows replay protection to be added to process stack. +This is achieved through the use of a sequence number for each unique set of signers. +Note that the sequence number for the single signing account "foo" will be unique +from the sequence number for a multi-sig account {"foo", "bar"} which would also be +unique from a different multi-sig account {"foo", "soup"} */ package nonce @@ -9,8 +13,6 @@ import ( "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/errors" "github.com/tendermint/basecoin/state" - - "github.com/tendermint/tmlibs/merkle" ) // nolint @@ -23,7 +25,7 @@ func init() { basecoin.TxMapper.RegisterImplementation(Tx{}, TypeNonce, ByteNonce) } -// Tx - XXX fill in +// Tx - Nonce transaction structure, contains list of signers and current sequence number type Tx struct { Sequence uint32 `json:"sequence"` Signers []basecoin.Actor `json:"signers"` @@ -46,23 +48,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?) + switch { + case n.Tx.Empty(): + return errors.ErrTxEmpty() + case n.Sequence == 0: + return errors.ErrZeroSequence() + case len(n.Signers) == 0: + return errors.ErrNoSigners() + } return n.Tx.ValidateBasic() } -// CheckIncrementSeq - XXX fill in -func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { +// CheckSeq - Check that the sequence number is one more than the state sequence number +// and further increment the sequence number +func (n Tx) CheckSeq(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) + seqKey := n.getSeqKey() // check the current state cur, err := getSeq(store, seqKey) @@ -79,12 +80,21 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { return errors.ErrNotMember() } } + return nil +} - // 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) +// IncrementSeq - increment the sequence for a group of actors +func (n Tx) IncrementSeq(ctx basecoin.Context, store state.KVStore) error { - //finally increment the sequence by 1 + seqKey := n.getSeqKey() + + // check the current state + cur, err := getSeq(store, seqKey) + if err != nil { + return err + } + + // increment the sequence by 1 err = setSeq(store, seqKey, cur+1) if err != nil { return err @@ -92,3 +102,18 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { return nil } + +// Generate the sequence key as the concatenated list of signers, sorted by address. +func (n Tx) getSeqKey() (seqKey []byte) { + + // First copy the list of signers to sort as sort is done in place + signers2sort := make([]basecoin.Actor, len(n.Signers)) + copy(signers2sort, n.Signers) + sort.Sort(basecoin.ByAddress(n.Signers)) + + for _, signer := range n.Signers { + seqKey = append(seqKey, signer.Address...) + } + //seqKey = merkle.SimpleHashFromBinary(n.Signers) + return +} diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index 9800d8ad12..ecd1fc40fe 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -18,14 +18,12 @@ func TestNonce(t *testing.T) { // generic args here... chainID := "my-chain" height := uint64(100) - // 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}} + act1 := basecoin.Actor{App: "fooz", Address: []byte{1, 2, 3, 4}} + act2 := basecoin.Actor{App: "fooz", Address: []byte{1, 1, 1, 1}} + act3 := basecoin.Actor{App: "fooz", Address: []byte{3, 3, 3, 3}} // let's construct some tests to make the table a bit less verbose set0 := []basecoin.Actor{} @@ -36,12 +34,10 @@ func TestNonce(t *testing.T) { 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... + valid bool + seq uint32 + actors []basecoin.Actor signers []basecoin.Actor }{ // one signer @@ -72,17 +68,17 @@ func TestNonce(t *testing.T) { {false, 2, set321, set321}, // no repetition } - // 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 + + // set the permissions myCtx := ctx.WithPermissions(test.signers...) tx := NewTx(test.seq, test.actors, raw) nonceTx, ok := tx.Unwrap().(Tx) require.True(ok) - err := nonceTx.CheckIncrementSeq(myCtx, store) + err := nonceTx.CheckSeq(myCtx, store) if test.valid { assert.Nil(err, "%d: %+v", i, err) } else { diff --git a/tests/cli/common.sh b/tests/cli/common.sh index c361c1df84..d186db220d 100644 --- a/tests/cli/common.sh +++ b/tests/cli/common.sh @@ -161,6 +161,7 @@ checkSendTx() { assertEquals "proper height" $2 $(echo $TX | jq .height) assertEquals "type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) CTX=$(echo $TX | jq .data.data.tx) + assertEquals "type=nonce" '"nonce"' $(echo $CTX | jq .type) assertEquals "type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) STX=$(echo $CTX | jq .data.tx) assertEquals "type=coin/send" '"coin/send"' $(echo $STX | jq .type)