From 50e4d31149a2155ee205622ce67b39eb48c26d20 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Tue, 11 Jul 2017 20:00:39 -0400 Subject: [PATCH 01/19] working nonce module --- app/app.go | 2 ++ cmd/basecli/commands/cmds.go | 4 +++ modules/nonce/replaycheck.go | 45 +++++++++++++++++++++++ modules/nonce/tx.go | 69 ++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+) create mode 100644 modules/nonce/replaycheck.go create mode 100644 modules/nonce/tx.go diff --git a/app/app.go b/app/app.go index 836e8c66fa..313c719338 100644 --- a/app/app.go +++ b/app/app.go @@ -15,6 +15,7 @@ import ( "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" "github.com/tendermint/basecoin/modules/fee" + "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/basecoin/stack" sm "github.com/tendermint/basecoin/state" "github.com/tendermint/basecoin/version" @@ -61,6 +62,7 @@ func DefaultHandler(feeDenom string) basecoin.Handler { base.Logger{}, stack.Recovery{}, auth.Signatures{}, + nonce.ReplayCheck{}, base.Chain{}, fee.NewSimpleFeeMiddleware(coin.Coin{feeDenom, 0}, fee.Bank), ).Use(d) diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index 631c371d6a..d069a838ab 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -17,6 +17,7 @@ import ( "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" "github.com/tendermint/basecoin/modules/fee" + "github.com/tendermint/basecoin/modules/nonce" ) //------------------------- @@ -74,6 +75,9 @@ func doSendTx(cmd *cobra.Command, args []string) error { return err } + // XXX - what is the nonceAccount here!!! + tx = nonce.NewTx(tx, viper.GetInt(FlagSequence), nonceAccount) + // Note: this is single sig (no multi sig yet) stx := auth.NewSig(tx) diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go new file mode 100644 index 0000000000..9e75f0b181 --- /dev/null +++ b/modules/nonce/replaycheck.go @@ -0,0 +1,45 @@ +package nonce + +import ( + "github.com/tendermint/basecoin" + "github.com/tendermint/basecoin/stack" + "github.com/tendermint/basecoin/state" +) + +//nolint +const ( + NameNonce = "nonce" +) + +// ReplayCheck parses out go-crypto signatures and adds permissions to the +// context for use inside the application +type ReplayCheck struct { + stack.PassOption +} + +// Name of the module - fulfills Middleware interface +func (ReplayCheck) Name() string { + return NameNonce +} + +var _ stack.Middleware = ReplayCheck{} + +// CheckTx verifies the signatures are correct - fulfills Middlware interface +func (ReplayCheck) CheckTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Checker) (res basecoin.Result, err error) { + sigs, tnext, err := getSigners(tx) + if err != nil { + return res, err + } + ctx2 := addSigners(ctx, sigs) + return next.CheckTx(ctx2, store, tnext) +} + +// DeliverTx verifies the signatures are correct - fulfills Middlware interface +func (ReplayCheck) DeliverTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Deliver) (res basecoin.Result, err error) { + sigs, tnext, err := getSigners(tx) + if err != nil { + return res, err + } + ctx2 := addSigners(ctx, sigs) + return next.DeliverTx(ctx2, store, tnext) +} diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go new file mode 100644 index 0000000000..741340cfdf --- /dev/null +++ b/modules/nonce/tx.go @@ -0,0 +1,69 @@ +/* +Package nonce XXX +*/ +package nonce + +import ( + "github.com/tendermint/basecoin/state" + + "github.com/tendermint/basecoin" +) + +// nolint +const ( + // for signatures + ByteSingleTx = 0x16 + ByteMultiSig = 0x17 +) + +/**** Registration ****/ + +//func init() { +//basecoin.TxMapper.RegisterImplementation(&Tx{}, TypeSingleTx, ByteSingleTx) +//} + +// Tx - XXX fill in +type Tx struct { + Tx basecoin.Tx `json:p"tx"` + Sequence uint32 + Signers []basecoin.Actor // or simple []data.Bytes (they are only pubkeys...) +} + +var _ basecoin.TxInner = &Tx{} + +// NewTx wraps the tx with a signable nonce +func NewTx(tx basecoin.Tx, sequence uint32, signers []basecoin.Actor) *Tx { + return &Tx{ + Tx: tx, + Sequence: sequence, + Signers: signers, + } +} + +//nolint +func (n *Tx) Wrap() basecoin.Tx { + return basecoin.Tx{s} +} +func (n *Tx) ValidateBasic() error { + return s.Tx.ValidateBasic() +} + +// CheckSequence - XXX fill in +func (n *Tx) CheckSequence(ctx basecoin.Context, store state.KVStore) error { + + // check the current state + h := hash(Sort(n.Signers)) + cur := loadSeq(store, h) + if n.Sequence != cur+1 { + return ErrBadNonce() + } + + // make sure they all signed + for _, s := range n.Signers { + if !ctx.HasPermission(s) { + return ErrNotMember() + } + } + + return nil +} From 16b039534d46099eeff9b1d0660502f74115783c Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Wed, 12 Jul 2017 05:02:16 -0400 Subject: [PATCH 02/19] working sequence number with errors --- app/app_test.go | 5 +- cmd/basecli/commands/cmds.go | 20 ++++++- context.go | 21 +++++++ .../cmd/countercli/commands/counter.go | 4 ++ docs/guide/counter/plugins/counter/counter.go | 9 ++- .../counter/plugins/counter/counter_test.go | 16 ++--- errors/common.go | 50 ++++++++++++---- modules/coin/errors.go | 7 --- modules/nonce/replaycheck.go | 44 ++++++++++---- modules/nonce/store.go | 30 ++++++++++ modules/nonce/tx.go | 60 ++++++++++++------- 11 files changed, 196 insertions(+), 70 deletions(-) create mode 100644 modules/nonce/store.go diff --git a/app/app_test.go b/app/app_test.go index cbb446bb05..9d182c7dd1 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -8,14 +8,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - abci "github.com/tendermint/abci/types" "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" "github.com/tendermint/basecoin/modules/fee" + "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" + + abci "github.com/tendermint/abci/types" wire "github.com/tendermint/go-wire" eyes "github.com/tendermint/merkleeyes/client" "github.com/tendermint/tmlibs/log" @@ -58,6 +60,7 @@ func (at *appTest) signTx(tx basecoin.Tx) basecoin.Tx { func (at *appTest) getTx(coins coin.Coins) basecoin.Tx { tx := at.baseTx(coins) tx = base.NewChainTx(at.chainID, 0, tx) + tx = nonce.NewTx(tx, 0, []basecoin.Actor{at.acctIn.Actor()}) return at.signTx(tx) } diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index d069a838ab..c451d35283 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -2,6 +2,7 @@ package commands import ( "encoding/hex" + "fmt" "strings" "github.com/pkg/errors" @@ -50,7 +51,7 @@ func init() { flags.Int(FlagSequence, -1, "Sequence number for this transaction") } -// runDemo is an example of how to make a tx +// doSendTx is an example of how to make a tx func doSendTx(cmd *cobra.Command, args []string) error { // load data from json or flags var tx basecoin.Tx @@ -65,6 +66,15 @@ func doSendTx(cmd *cobra.Command, args []string) error { return err } + sendTx, ok := tx.Unwrap().(coin.SendTx) + if !ok { + return errors.New("tx not SendTx") + } + var nonceAccount []basecoin.Actor + for _, input := range sendTx.Inputs { + nonceAccount = append(nonceAccount, input.Address) + } + // TODO: make this more flexible for middleware tx, err = WrapFeeTx(tx) if err != nil { @@ -75,8 +85,12 @@ func doSendTx(cmd *cobra.Command, args []string) error { return err } - // XXX - what is the nonceAccount here!!! - tx = nonce.NewTx(tx, viper.GetInt(FlagSequence), nonceAccount) + //add the nonce tx layer to the tx + seq := viper.GetInt(FlagSequence) + if seq < 0 { + return fmt.Errorf("sequence must be greater than 0") + } + tx = nonce.NewTx(tx, uint32(seq), nonceAccount) // XXX - what is the nonceAccount here!!! // Note: this is single sig (no multi sig yet) stx := auth.NewSig(tx) diff --git a/context.go b/context.go index d67fa0d9ae..1308e797c3 100644 --- a/context.go +++ b/context.go @@ -2,6 +2,8 @@ package basecoin import ( "bytes" + "fmt" + "sort" wire "github.com/tendermint/go-wire" "github.com/tendermint/go-wire/data" @@ -19,6 +21,7 @@ type Actor struct { Address data.Bytes `json:"addr"` // arbitrary app-specific unique id } +// NewActor - create a new actor func NewActor(app string, addr []byte) Actor { return Actor{App: app, Address: addr} } @@ -48,3 +51,21 @@ type Context interface { ChainID() string BlockHeight() uint64 } + +//////////////////////////////// Sort Interface +// USAGE sort.Sort(ByAddress()) + +func (a Actor) String() string { + return fmt.Sprintf("%x", a.Address) +} + +// ByAddress implements sort.Interface for []Actor based on +// the Address field. +type ByAddress []Actor + +// Verify the sort interface at compile time +var _ sort.Interface = ByAddress{} + +func (a ByAddress) Len() int { return len(a) } +func (a ByAddress) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a ByAddress) Less(i, j int) bool { return bytes.Compare(a[i].Address, a[j].Address) == -1 } diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index 9e420b008c..0e340d01f0 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -83,6 +83,10 @@ func readCounterTxFlags() (tx basecoin.Tx, err error) { return tx, err } +<<<<<<< HEAD tx = counter.NewTx(viper.GetBool(FlagValid), feeCoins, viper.GetInt(bcmd.FlagSequence)) +======= + tx = counter.NewTx(viper.GetBool(FlagValid), feeCoins) +>>>>>>> working sequence number with errors return tx, nil } diff --git a/docs/guide/counter/plugins/counter/counter.go b/docs/guide/counter/plugins/counter/counter.go index b1a7b86088..7a4eb9e4cb 100644 --- a/docs/guide/counter/plugins/counter/counter.go +++ b/docs/guide/counter/plugins/counter/counter.go @@ -36,15 +36,14 @@ func init() { type Tx struct { Valid bool `json:"valid"` Fee coin.Coins `json:"fee"` - Sequence int `json:"sequence"` + Sequence int `json:""` } // NewTx - return a new counter transaction struct wrapped as a basecoin transaction -func NewTx(valid bool, fee coin.Coins, sequence int) basecoin.Tx { +func NewTx(valid bool, fee coin.Coins) basecoin.Tx { return Tx{ - Valid: valid, - Fee: fee, - Sequence: sequence, + Valid: valid, + Fee: fee, }.Wrap() } diff --git a/docs/guide/counter/plugins/counter/counter_test.go b/docs/guide/counter/plugins/counter/counter_test.go index 60c5101fd1..919c42dc75 100644 --- a/docs/guide/counter/plugins/counter/counter_test.go +++ b/docs/guide/counter/plugins/counter/counter_test.go @@ -40,8 +40,8 @@ func TestCounterPlugin(t *testing.T) { require.Equal(t, "Success", log) // Deliver a CounterTx - DeliverCounterTx := func(valid bool, counterFee coin.Coins, inputSequence int) abci.Result { - tx := NewTx(valid, counterFee, inputSequence) + DeliverCounterTx := func(valid bool, counterFee coin.Coins) abci.Result { + tx := NewTx(valid, counterFee) tx = base.NewChainTx(chainID, 0, tx) stx := auth.NewSig(tx) auth.Sign(stx, acct.Key) @@ -49,19 +49,19 @@ func TestCounterPlugin(t *testing.T) { return bcApp.DeliverTx(txBytes) } - // Test a basic send, no fee (doesn't update sequence as no money spent) - res := DeliverCounterTx(true, nil, 1) + // Test a basic send, no fee + res := DeliverCounterTx(true, nil) assert.True(res.IsOK(), res.String()) // Test an invalid send, no fee - res = DeliverCounterTx(false, nil, 1) + res = DeliverCounterTx(false, nil) assert.True(res.IsErr(), res.String()) - // Test the fee (increments sequence) - res = DeliverCounterTx(true, coin.Coins{{"gold", 100}}, 1) + // Test an invalid send, with supported fee + res = DeliverCounterTx(true, coin.Coins{{"gold", 100}}) assert.True(res.IsOK(), res.String()) // Test unsupported fee - res = DeliverCounterTx(true, coin.Coins{{"silver", 100}}, 2) + res = DeliverCounterTx(true, coin.Coins{{"silver", 100}}) assert.True(res.IsErr(), res.String()) } diff --git a/errors/common.go b/errors/common.go index 81d786beee..13aaf3d488 100644 --- a/errors/common.go +++ b/errors/common.go @@ -12,12 +12,16 @@ import ( var ( errDecoding = fmt.Errorf("Error decoding input") + errNoAccount = fmt.Errorf("No such account") errUnauthorized = fmt.Errorf("Unauthorized") errInvalidSignature = fmt.Errorf("Invalid Signature") errTooLarge = fmt.Errorf("Input size too large") 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") errWrongChain = fmt.Errorf("Wrong chain for tx") errUnknownTxType = fmt.Errorf("Tx type unknown") errInvalidFormat = fmt.Errorf("Invalid format") @@ -26,6 +30,14 @@ var ( errUnknownKey = fmt.Errorf("Unknown key") ) +var ( + internalErr = abci.CodeType_InternalError + encodingErr = abci.CodeType_EncodingError + unauthorized = abci.CodeType_Unauthorized + unknownRequest = abci.CodeType_UnknownRequest + unknownAddress = abci.CodeType_BaseUnknownAddress +) + // some crazy reflection to unwrap any generated struct. func unwrap(i interface{}) interface{} { v := reflect.ValueOf(i) @@ -71,76 +83,90 @@ func IsUnknownKeyErr(err error) bool { } func ErrInternal(msg string) TMError { - return New(msg, abci.CodeType_InternalError) + return New(msg, internalErr) } // IsInternalErr matches any error that is not classified func IsInternalErr(err error) bool { - return HasErrorCode(err, abci.CodeType_InternalError) + return HasErrorCode(err, internalErr) } func ErrDecoding() TMError { - return WithCode(errDecoding, abci.CodeType_EncodingError) + return WithCode(errDecoding, encodingErr) } func IsDecodingErr(err error) bool { return IsSameError(errDecoding, err) } func ErrUnauthorized() TMError { - return WithCode(errUnauthorized, abci.CodeType_Unauthorized) + return WithCode(errUnauthorized, unauthorized) } // IsUnauthorizedErr is generic helper for any unauthorized errors, // also specific sub-types func IsUnauthorizedErr(err error) bool { - return HasErrorCode(err, abci.CodeType_Unauthorized) + return HasErrorCode(err, unauthorized) } func ErrMissingSignature() TMError { - return WithCode(errMissingSignature, abci.CodeType_Unauthorized) + return WithCode(errMissingSignature, unauthorized) } func IsMissingSignatureErr(err error) bool { return IsSameError(errMissingSignature, err) } func ErrTooManySignatures() TMError { - return WithCode(errTooManySignatures, abci.CodeType_Unauthorized) + return WithCode(errTooManySignatures, unauthorized) } func IsTooManySignaturesErr(err error) bool { return IsSameError(errTooManySignatures, err) } func ErrInvalidSignature() TMError { - return WithCode(errInvalidSignature, abci.CodeType_Unauthorized) + return WithCode(errInvalidSignature, unauthorized) } func IsInvalidSignatureErr(err error) bool { return IsSameError(errInvalidSignature, err) } func ErrNoChain() TMError { - return WithCode(errNoChain, abci.CodeType_Unauthorized) + return WithCode(errNoChain, unauthorized) } func IsNoChainErr(err error) bool { return IsSameError(errNoChain, err) } +func ErrNoNonce() TMError { + return WithCode(errNoNonce, unauthorized) +} +func ErrBadNonce() TMError { + return WithCode(errBadNonce, unauthorized) +} +func ErrNotMember() TMError { + return WithCode(errBadNonce, unauthorized) +} + +func ErrNoAccount() TMError { + return WithCode(errNoAccount, unknownAddress) +} + func ErrWrongChain(chain string) TMError { msg := errors.Wrap(errWrongChain, chain) - return WithCode(msg, abci.CodeType_Unauthorized) + return WithCode(msg, unauthorized) } func IsWrongChainErr(err error) bool { return IsSameError(errWrongChain, err) } func ErrTooLarge() TMError { - return WithCode(errTooLarge, abci.CodeType_EncodingError) + return WithCode(errTooLarge, encodingErr) } func IsTooLargeErr(err error) bool { return IsSameError(errTooLarge, err) } func ErrExpired() TMError { - return WithCode(errExpired, abci.CodeType_Unauthorized) + return WithCode(errExpired, unauthorized) } func IsExpiredErr(err error) bool { return IsSameError(errExpired, err) diff --git a/modules/coin/errors.go b/modules/coin/errors.go index bc3d7c5b78..d245354bff 100644 --- a/modules/coin/errors.go +++ b/modules/coin/errors.go @@ -60,13 +60,6 @@ func IsInvalidCoinsErr(err error) bool { return errors.IsSameError(errInvalidCoins, err) } -func ErrInvalidSequence() errors.TMError { - return errors.WithCode(errInvalidSequence, invalidInput) -} -func IsInvalidSequenceErr(err error) bool { - return errors.IsSameError(errInvalidSequence, err) -} - func ErrInsufficientFunds() errors.TMError { return errors.WithCode(errInsufficientFunds, invalidInput) } diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go index 9e75f0b181..1412bfb165 100644 --- a/modules/nonce/replaycheck.go +++ b/modules/nonce/replaycheck.go @@ -2,6 +2,7 @@ package nonce import ( "github.com/tendermint/basecoin" + "github.com/tendermint/basecoin/errors" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" ) @@ -11,8 +12,7 @@ const ( NameNonce = "nonce" ) -// ReplayCheck parses out go-crypto signatures and adds permissions to the -// context for use inside the application +// ReplayCheck uses the sequence to check for replay attacks type ReplayCheck struct { stack.PassOption } @@ -24,22 +24,42 @@ func (ReplayCheck) Name() string { var _ stack.Middleware = ReplayCheck{} -// CheckTx verifies the signatures are correct - fulfills Middlware interface -func (ReplayCheck) CheckTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Checker) (res basecoin.Result, err error) { - sigs, tnext, err := getSigners(tx) +// CheckTx verifies tx is not being replayed - fulfills Middlware interface +func (r ReplayCheck) CheckTx(ctx basecoin.Context, store state.KVStore, + tx basecoin.Tx, next basecoin.Checker) (res basecoin.Result, err error) { + + stx, err := r.checkNonceTx(ctx, store, tx) if err != nil { return res, err } - ctx2 := addSigners(ctx, sigs) - return next.CheckTx(ctx2, store, tnext) + return next.CheckTx(ctx, store, stx) } -// DeliverTx verifies the signatures are correct - fulfills Middlware interface -func (ReplayCheck) DeliverTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Deliver) (res basecoin.Result, err error) { - sigs, tnext, err := getSigners(tx) +// DeliverTx verifies tx is not being replayed - fulfills Middlware interface +func (r ReplayCheck) DeliverTx(ctx basecoin.Context, store state.KVStore, + tx basecoin.Tx, next basecoin.Deliver) (res basecoin.Result, err error) { + + stx, err := r.checkNonceTx(ctx, store, tx) if err != nil { return res, err } - ctx2 := addSigners(ctx, sigs) - return next.DeliverTx(ctx2, store, tnext) + return next.DeliverTx(ctx, store, stx) +} + +// checkNonceTx varifies the nonce sequence +func (r ReplayCheck) checkNonceTx(ctx basecoin.Context, store state.KVStore, + tx basecoin.Tx) (basecoin.Tx, error) { + + // make sure it is a the nonce Tx (Tx from this package) + nonceTx, ok := tx.Unwrap().(Tx) + if !ok { + return tx, errors.ErrNoNonce() + } + + // check the nonce sequence number + err := nonceTx.CheckIncrementSeq(ctx, store) + if err != nil { + return tx, err + } + return nonceTx.Tx, nil } diff --git a/modules/nonce/store.go b/modules/nonce/store.go new file mode 100644 index 0000000000..51f6cdb1dd --- /dev/null +++ b/modules/nonce/store.go @@ -0,0 +1,30 @@ +package nonce + +import ( + "fmt" + + wire "github.com/tendermint/go-wire" + + "github.com/tendermint/basecoin/errors" + "github.com/tendermint/basecoin/state" +) + +func getSeq(store state.KVStore, key []byte) (seq uint32, err error) { + // fmt.Printf("load: %X\n", key) + data := store.Get(key) + if len(data) == 0 { + return seq, errors.ErrNoAccount() + } + err = wire.ReadBinaryBytes(data, &seq) + if err != nil { + msg := fmt.Sprintf("Error reading sequence for %X", key) + return seq, errors.ErrInternal(msg) + } + return seq, nil +} + +func setSeq(store state.KVStore, key []byte, seq uint32) error { + bin := wire.BinaryBytes(seq) + store.Set(key, bin) + return nil // real stores can return error... +} diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 741340cfdf..95773a0895 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -4,66 +4,82 @@ Package nonce XXX package nonce import ( - "github.com/tendermint/basecoin/state" + "sort" "github.com/tendermint/basecoin" + "github.com/tendermint/basecoin/errors" + "github.com/tendermint/basecoin/state" + + "github.com/tendermint/tmlibs/merkle" ) // nolint const ( - // for signatures - ByteSingleTx = 0x16 - ByteMultiSig = 0x17 + ByteNonce = 0x69 //TODO overhaul byte assign system don't make no sense! + TypeNonce = "nonce" ) -/**** Registration ****/ - -//func init() { -//basecoin.TxMapper.RegisterImplementation(&Tx{}, TypeSingleTx, ByteSingleTx) -//} +func init() { + basecoin.TxMapper.RegisterImplementation(&Tx{}, TypeNonce, ByteNonce) +} // Tx - XXX fill in type Tx struct { Tx basecoin.Tx `json:p"tx"` Sequence uint32 Signers []basecoin.Actor // or simple []data.Bytes (they are only pubkeys...) + seqKey []byte //key to store the sequence number } var _ basecoin.TxInner = &Tx{} // NewTx wraps the tx with a signable nonce -func NewTx(tx basecoin.Tx, sequence uint32, signers []basecoin.Actor) *Tx { - return &Tx{ +func NewTx(tx basecoin.Tx, sequence uint32, signers []basecoin.Actor) basecoin.Tx { + + //Generate the sequence key as the hash of the list of signers, sorted by address + sort.Sort(basecoin.ByAddress(signers)) + seqKey := merkle.SimpleHashFromBinary(signers) + + return (Tx{ Tx: tx, Sequence: sequence, Signers: signers, - } + seqKey: seqKey, + }).Wrap() } //nolint -func (n *Tx) Wrap() basecoin.Tx { - return basecoin.Tx{s} +func (n Tx) Wrap() basecoin.Tx { + return basecoin.Tx{n} } -func (n *Tx) ValidateBasic() error { - return s.Tx.ValidateBasic() +func (n Tx) ValidateBasic() error { + return n.Tx.ValidateBasic() } -// CheckSequence - XXX fill in -func (n *Tx) CheckSequence(ctx basecoin.Context, store state.KVStore) error { +// CheckIncrementSeq - XXX fill in +func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { // check the current state - h := hash(Sort(n.Signers)) - cur := loadSeq(store, h) + cur, err := getSeq(store, n.seqKey) + if err != nil { + return err + } if n.Sequence != cur+1 { - return ErrBadNonce() + return errors.ErrBadNonce() } // make sure they all signed for _, s := range n.Signers { if !ctx.HasPermission(s) { - return ErrNotMember() + return errors.ErrNotMember() } } + //finally increment the sequence by 1 + err = setSeq(store, n.seqKey, cur+1) + if err != nil { + return err + } + return nil } From 6e07dbe7c391c1b92319340d6d23eda5cbd599bd Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Wed, 12 Jul 2017 06:10:17 -0400 Subject: [PATCH 03/19] nonce testing --- cmd/basecli/commands/cmds.go | 2 +- errors/common.go | 9 ++---- modules/nonce/store.go | 4 +-- modules/nonce/tx.go | 29 ++++++++--------- modules/nonce/tx_test.go | 60 ++++++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 26 deletions(-) create mode 100644 modules/nonce/tx_test.go diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index c451d35283..3bd6e26a6c 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -90,7 +90,7 @@ func doSendTx(cmd *cobra.Command, args []string) error { if seq < 0 { return fmt.Errorf("sequence must be greater than 0") } - tx = nonce.NewTx(tx, uint32(seq), nonceAccount) // XXX - what is the nonceAccount here!!! + tx = nonce.NewTx(uint32(seq), nonceAccount, tx) // Note: this is single sig (no multi sig yet) stx := auth.NewSig(tx) diff --git a/errors/common.go b/errors/common.go index 13aaf3d488..bdce81d944 100644 --- a/errors/common.go +++ b/errors/common.go @@ -12,7 +12,6 @@ import ( var ( errDecoding = fmt.Errorf("Error decoding input") - errNoAccount = fmt.Errorf("No such account") errUnauthorized = fmt.Errorf("Unauthorized") errInvalidSignature = fmt.Errorf("Invalid Signature") errTooLarge = fmt.Errorf("Input size too large") @@ -139,17 +138,13 @@ func IsNoChainErr(err error) bool { func ErrNoNonce() TMError { return WithCode(errNoNonce, unauthorized) } -func ErrBadNonce() TMError { - return WithCode(errBadNonce, unauthorized) +func ErrBadNonce(got, expected uint32) TMError { + return WithCode(fmt.Errorf("Bad nonce seqence, got %d, expected %d", got, expected), unauthorized) } func ErrNotMember() TMError { return WithCode(errBadNonce, unauthorized) } -func ErrNoAccount() TMError { - return WithCode(errNoAccount, unknownAddress) -} - func ErrWrongChain(chain string) TMError { msg := errors.Wrap(errWrongChain, chain) return WithCode(msg, unauthorized) diff --git a/modules/nonce/store.go b/modules/nonce/store.go index 51f6cdb1dd..ae10ccaf66 100644 --- a/modules/nonce/store.go +++ b/modules/nonce/store.go @@ -10,10 +10,10 @@ import ( ) func getSeq(store state.KVStore, key []byte) (seq uint32, err error) { - // fmt.Printf("load: %X\n", key) data := store.Get(key) if len(data) == 0 { - return seq, errors.ErrNoAccount() + //if the key is not stored, its a new key with a sequence of zero! + return 0, nil } err = wire.ReadBinaryBytes(data, &seq) if err != nil { diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 95773a0895..09d5be6c9c 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -20,31 +20,24 @@ const ( ) func init() { - basecoin.TxMapper.RegisterImplementation(&Tx{}, TypeNonce, ByteNonce) + basecoin.TxMapper.RegisterImplementation(Tx{}, TypeNonce, ByteNonce) } // Tx - XXX fill in type Tx struct { - Tx basecoin.Tx `json:p"tx"` - Sequence uint32 - Signers []basecoin.Actor // or simple []data.Bytes (they are only pubkeys...) - seqKey []byte //key to store the sequence number + Sequence uint32 `json:"sequence"` + Signers []basecoin.Actor `json:"signers"` + Tx basecoin.Tx `json:"tx"` } var _ basecoin.TxInner = &Tx{} // NewTx wraps the tx with a signable nonce -func NewTx(tx basecoin.Tx, sequence uint32, signers []basecoin.Actor) basecoin.Tx { - - //Generate the sequence key as the hash of the list of signers, sorted by address - sort.Sort(basecoin.ByAddress(signers)) - seqKey := merkle.SimpleHashFromBinary(signers) - +func NewTx(sequence uint32, signers []basecoin.Actor, tx basecoin.Tx) basecoin.Tx { return (Tx{ - Tx: tx, Sequence: sequence, Signers: signers, - seqKey: seqKey, + Tx: tx, }).Wrap() } @@ -59,13 +52,17 @@ func (n Tx) ValidateBasic() error { // CheckIncrementSeq - XXX fill in func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { + //Generate the sequence key as the hash of the list of signers, sorted by address + sort.Sort(basecoin.ByAddress(n.Signers)) + seqKey := merkle.SimpleHashFromBinary(n.Signers) + // check the current state - cur, err := getSeq(store, n.seqKey) + cur, err := getSeq(store, seqKey) if err != nil { return err } if n.Sequence != cur+1 { - return errors.ErrBadNonce() + return errors.ErrBadNonce(n.Sequence, cur+1) } // make sure they all signed @@ -76,7 +73,7 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { } //finally increment the sequence by 1 - err = setSeq(store, n.seqKey, cur+1) + err = setSeq(store, seqKey, cur+1) if err != nil { return err } diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go new file mode 100644 index 0000000000..faafba216c --- /dev/null +++ b/modules/nonce/tx_test.go @@ -0,0 +1,60 @@ +package nonce + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/tendermint/basecoin" + "github.com/tendermint/basecoin/stack" + "github.com/tendermint/basecoin/state" + + "github.com/tendermint/tmlibs/log" +) + +func TestNonce(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + // generic args here... + chainID := "my-chain" + height := uint64(100) + ctx := stack.NewContext(chainID, height, log.NewNopLogger()) + store := state.NewMemKVStore() + + 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}} + + testList := []struct { + valid bool + seq uint32 + actors []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}}, + } + + for _, test := range testList { + + tx := NewTx(test.seq, test.actors, basecoin.Tx{}) + nonceTx, ok := tx.Unwrap().(Tx) + require.True(ok) + err := nonceTx.CheckIncrementSeq(ctx, store) + if test.valid { + assert.Nil(err, "%v,%v: %+v", test.seq, test.actors, err) + } else { + assert.NotNil(err, "%v,%v", test.seq, test.actors) + } + } +} From 1f2e939ea1eefb6fd64e7f8ad04178729e6f18d9 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 12 Jul 2017 15:19:37 +0200 Subject: [PATCH 04/19] Add signing fix, expands nonce tests --- modules/nonce/tx.go | 12 +++++++ modules/nonce/tx_test.go | 70 +++++++++++++++++++++++++++++----------- 2 files changed, 63 insertions(+), 19 deletions(-) diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 09d5be6c9c..2aa3aeb2ed 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 faafba216c..9800d8ad12 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) } } } From 5ccf22bfb777c0626f2a0ff192911923499a2e83 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 04:37:22 -0400 Subject: [PATCH 05/19] 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) From 7bdf44c987d9749573b9f9c7b04d60e90582c0e8 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 10:34:57 -0400 Subject: [PATCH 06/19] increment fixes, test_unit working --- modules/nonce/replaycheck.go | 52 +++++++----------------------------- modules/nonce/tx.go | 19 +++---------- modules/nonce/tx_test.go | 2 +- 3 files changed, 14 insertions(+), 59 deletions(-) diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go index 8a401e79ae..b9f28d253c 100644 --- a/modules/nonce/replaycheck.go +++ b/modules/nonce/replaycheck.go @@ -28,46 +28,30 @@ var _ stack.Middleware = ReplayCheck{} func (r ReplayCheck) CheckTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Checker) (res basecoin.Result, err error) { - stx, err := r.checkNonceTx(ctx, store, tx) + stx, err := r.checkIncrementNonceTx(ctx, store, tx) if err != nil { return res, err } - 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 + return next.CheckTx(ctx, store, stx) } // DeliverTx verifies tx is not being replayed - fulfills Middlware interface +// NOTE It is okay to modify the sequence before running the wrapped TX because if the +// wrapped Tx fails, the state changes are not applied func (r ReplayCheck) DeliverTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx, next basecoin.Deliver) (res basecoin.Result, err error) { - stx, err := r.checkNonceTx(ctx, store, tx) + stx, err := r.checkIncrementNonceTx(ctx, store, tx) if err != nil { return res, err } - 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 + return next.DeliverTx(ctx, store, stx) } -// checkNonceTx varifies the nonce sequence -func (r ReplayCheck) checkNonceTx(ctx basecoin.Context, store state.KVStore, +// checkNonceTx varifies the nonce sequence, an increment sequence number +func (r ReplayCheck) checkIncrementNonceTx(ctx basecoin.Context, store state.KVStore, tx basecoin.Tx) (basecoin.Tx, error) { // make sure it is a the nonce Tx (Tx from this package) @@ -77,27 +61,9 @@ func (r ReplayCheck) checkNonceTx(ctx basecoin.Context, store state.KVStore, } // check the nonce sequence number - err := nonceTx.CheckSeq(ctx, store) + err := nonceTx.CheckIncrementSeq(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 8e7df262b9..bb3d197297 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -59,9 +59,11 @@ func (n Tx) ValidateBasic() error { return n.Tx.ValidateBasic() } -// CheckSeq - Check that the sequence number is one more than the state sequence number +// CheckIncrementSeq - 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 { +// NOTE It is okay to modify the sequence before running the wrapped TX because if the +// wrapped Tx fails, the state changes are not applied +func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { seqKey := n.getSeqKey() @@ -80,19 +82,6 @@ func (n Tx) CheckSeq(ctx basecoin.Context, store state.KVStore) error { return errors.ErrNotMember() } } - return nil -} - -// IncrementSeq - increment the sequence for a group of actors -func (n Tx) IncrementSeq(ctx basecoin.Context, store state.KVStore) error { - - 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) diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index ecd1fc40fe..2d4406207d 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -78,7 +78,7 @@ func TestNonce(t *testing.T) { nonceTx, ok := tx.Unwrap().(Tx) require.True(ok) - err := nonceTx.CheckSeq(myCtx, store) + err := nonceTx.CheckIncrementSeq(myCtx, store) if test.valid { assert.Nil(err, "%d: %+v", i, err) } else { From 463f2dca30c6bed6c3adee69c10331d064832526 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 16:47:19 -0400 Subject: [PATCH 07/19] changefile remove silent on bash test int int --- Makefile | 6 +++--- docs/guide/counter/cmd/countercli/commands/counter.go | 4 ---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 143395559e..84c4e11f40 100644 --- a/Makefile +++ b/Makefile @@ -27,9 +27,9 @@ test_unit: test_cli: tests/cli/shunit2 # sudo apt-get install jq - @./tests/cli/basictx.sh - @./tests/cli/counter.sh - @./tests/cli/restart.sh + ./tests/cli/basictx.sh + ./tests/cli/counter.sh + ./tests/cli/restart.sh # @./tests/cli/ibc.sh test_tutorial: docs/guide/shunit2 diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index 0e340d01f0..8364f41cbd 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -83,10 +83,6 @@ func readCounterTxFlags() (tx basecoin.Tx, err error) { return tx, err } -<<<<<<< HEAD - tx = counter.NewTx(viper.GetBool(FlagValid), feeCoins, viper.GetInt(bcmd.FlagSequence)) -======= tx = counter.NewTx(viper.GetBool(FlagValid), feeCoins) ->>>>>>> working sequence number with errors return tx, nil } From 7a37b9b9a967295b16501d12e6c8a2fd04c5cd76 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 16:58:13 -0400 Subject: [PATCH 08/19] unit test fixes after rebase --- app/app_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/app_test.go b/app/app_test.go index 9d182c7dd1..813c50afe2 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + abci "github.com/tendermint/abci/types" "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/base" @@ -16,8 +17,6 @@ import ( "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" - - abci "github.com/tendermint/abci/types" wire "github.com/tendermint/go-wire" eyes "github.com/tendermint/merkleeyes/client" "github.com/tendermint/tmlibs/log" @@ -60,7 +59,7 @@ func (at *appTest) signTx(tx basecoin.Tx) basecoin.Tx { func (at *appTest) getTx(coins coin.Coins) basecoin.Tx { tx := at.baseTx(coins) tx = base.NewChainTx(at.chainID, 0, tx) - tx = nonce.NewTx(tx, 0, []basecoin.Actor{at.acctIn.Actor()}) + tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) return at.signTx(tx) } @@ -68,6 +67,7 @@ func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin) basecoin.Tx { tx := at.baseTx(coins) tx = fee.NewFee(tx, toll, at.acctIn.Actor()) tx = base.NewChainTx(at.chainID, 0, tx) + tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) return at.signTx(tx) } From 23615c5d37d221729f600fa72b9539b1aa41dbf2 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Thu, 13 Jul 2017 19:29:53 -0400 Subject: [PATCH 09/19] almost done! --- cmd/basecli/commands/cmds.go | 1 + .../cmd/countercli/commands/counter.go | 14 +++++ docs/guide/counter/plugins/counter/counter.go | 2 + .../counter/plugins/counter/counter_test.go | 13 ++-- tests/cli/common.sh | 63 ++++++++++--------- tests/cli/counter.sh | 36 +++++------ 6 files changed, 76 insertions(+), 53 deletions(-) diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index 3bd6e26a6c..832151ca1d 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -66,6 +66,7 @@ func doSendTx(cmd *cobra.Command, args []string) error { return err } + //get the nonce accounts sendTx, ok := tx.Unwrap().(coin.SendTx) if !ok { return errors.New("tx not SendTx") diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index 8364f41cbd..3ab9e8e26d 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -1,6 +1,8 @@ package commands import ( + "fmt" + "github.com/spf13/cobra" "github.com/spf13/viper" @@ -11,6 +13,7 @@ import ( "github.com/tendermint/basecoin/docs/guide/counter/plugins/counter" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/coin" + "github.com/tendermint/basecoin/modules/nonce" ) //CounterTxCmd is the CLI command to execute the counter @@ -55,6 +58,10 @@ func counterTx(cmd *cobra.Command, args []string) error { return err } + //get the nonce accounts + var addr []byte + nonceAccount := []basecoin.Actor{basecoin.NewActor(counter.NameCounter, addr)} + // TODO: make this more flexible for middleware tx, err = bcmd.WrapFeeTx(tx) if err != nil { @@ -65,6 +72,13 @@ func counterTx(cmd *cobra.Command, args []string) error { return err } + //add the nonce tx layer to the tx + seq := viper.GetInt(FlagSequence) + if seq < 0 { + return fmt.Errorf("sequence must be greater than 0") + } + tx = nonce.NewTx(uint32(seq), nonceAccount, tx) + stx := auth.NewSig(tx) // Sign if needed and post. This it the work-horse diff --git a/docs/guide/counter/plugins/counter/counter.go b/docs/guide/counter/plugins/counter/counter.go index 7a4eb9e4cb..d00f0fa901 100644 --- a/docs/guide/counter/plugins/counter/counter.go +++ b/docs/guide/counter/plugins/counter/counter.go @@ -12,6 +12,7 @@ import ( "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" "github.com/tendermint/basecoin/modules/fee" + "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" ) @@ -101,6 +102,7 @@ func NewHandler(feeDenom string) basecoin.Handler { base.Logger{}, stack.Recovery{}, auth.Signatures{}, + nonce.ReplayCheck{}, base.Chain{}, fee.NewSimpleFeeMiddleware(coin.Coin{feeDenom, 0}, fee.Bank), ).Use(dispatcher) diff --git a/docs/guide/counter/plugins/counter/counter_test.go b/docs/guide/counter/plugins/counter/counter_test.go index 919c42dc75..b8b79a7432 100644 --- a/docs/guide/counter/plugins/counter/counter_test.go +++ b/docs/guide/counter/plugins/counter/counter_test.go @@ -7,10 +7,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" abci "github.com/tendermint/abci/types" + "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/app" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" + "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/go-wire" eyescli "github.com/tendermint/merkleeyes/client" "github.com/tendermint/tmlibs/log" @@ -40,9 +42,10 @@ func TestCounterPlugin(t *testing.T) { require.Equal(t, "Success", log) // Deliver a CounterTx - DeliverCounterTx := func(valid bool, counterFee coin.Coins) abci.Result { + DeliverCounterTx := func(valid bool, counterFee coin.Coins, sequence uint32) abci.Result { tx := NewTx(valid, counterFee) tx = base.NewChainTx(chainID, 0, tx) + tx = nonce.NewTx(sequence, []basecoin.Actor{acct.Actor()}, tx) stx := auth.NewSig(tx) auth.Sign(stx, acct.Key) txBytes := wire.BinaryBytes(stx.Wrap()) @@ -50,18 +53,18 @@ func TestCounterPlugin(t *testing.T) { } // Test a basic send, no fee - res := DeliverCounterTx(true, nil) + res := DeliverCounterTx(true, nil, 1) assert.True(res.IsOK(), res.String()) // Test an invalid send, no fee - res = DeliverCounterTx(false, nil) + res = DeliverCounterTx(false, nil, 2) assert.True(res.IsErr(), res.String()) // Test an invalid send, with supported fee - res = DeliverCounterTx(true, coin.Coins{{"gold", 100}}) + res = DeliverCounterTx(true, coin.Coins{{"gold", 100}}, 2) assert.True(res.IsOK(), res.String()) // Test unsupported fee - res = DeliverCounterTx(true, coin.Coins{{"silver", 100}}) + res = DeliverCounterTx(true, coin.Coins{{"silver", 100}}, 3) assert.True(res.IsErr(), res.String()) } diff --git a/tests/cli/common.sh b/tests/cli/common.sh index d186db220d..6242086f07 100644 --- a/tests/cli/common.sh +++ b/tests/cli/common.sh @@ -46,7 +46,7 @@ quickTearDown() { prepareClient() { echo "Preparing client keys..." ${CLIENT_EXE} reset_all - assertTrue $? + assertTrue "line=${LINENO}, prepare client" $? for i in "${!ACCOUNTS[@]}"; do newKey ${ACCOUNTS[$i]} @@ -60,7 +60,7 @@ prepareClient() { initServer() { echo "Setting up genesis..." SERVE_DIR=$1/server - assertNotNull "no chain" $2 + assertNotNull "line=${LINENO}, no chain" $2 CHAIN=$2 SERVER_LOG=$1/${SERVER_EXE}.log @@ -100,26 +100,26 @@ initClient() { PORT=${2:-46657} # hard-code the expected validator hash ${CLIENT_EXE} init --chain-id=$1 --node=tcp://localhost:${PORT} --valhash=EB168E17E45BAEB194D4C79067FFECF345C64DE6 - assertTrue "initialized light-client" $? + assertTrue "line=${LINENO}, initialized light-client" $? } # XXX Ex Usage1: newKey $NAME # XXX Ex Usage2: newKey $NAME $PASSWORD # Desc: Generates key for given username and password newKey(){ - assertNotNull "keyname required" "$1" + assertNotNull "line=${LINENO}, keyname required" "$1" KEYPASS=${2:-qwertyuiop} (echo $KEYPASS; echo $KEYPASS) | ${CLIENT_EXE} keys new $1 >/dev/null 2>/dev/null - assertTrue "created $1" $? - assertTrue "$1 doesn't exist" "${CLIENT_EXE} keys get $1" + assertTrue "line=${LINENO}, created $1" $? + assertTrue "line=${LINENO}, $1 doesn't exist" "${CLIENT_EXE} keys get $1" } # XXX Ex Usage: getAddr $NAME # Desc: Gets the address for a key name getAddr() { - assertNotNull "keyname required" "$1" + assertNotNull "line=${LINENO}, keyname required" "$1" RAW=$(${CLIENT_EXE} keys get $1) - assertTrue "no key for $1" $? + assertTrue "line=${LINENO}, no key for $1" $? # print the addr echo $RAW | cut -d' ' -f2 } @@ -129,12 +129,12 @@ getAddr() { checkAccount() { # make sure sender goes down ACCT=$(${CLIENT_EXE} query account $1) - if ! assertTrue "account must exist" $?; then + if ! assertTrue "line=${LINENO}, account must exist" $?; then return 1 fi if [ -n "$DEBUG" ]; then echo $ACCT; echo; fi - assertEquals "proper money" "$2" $(echo $ACCT | jq .data.coins[0].amount) + assertEquals "line=${LINENO}, proper money" "$2" $(echo $ACCT | jq .data.coins[0].amount) return $? } @@ -143,8 +143,8 @@ checkAccount() { txSucceeded() { if (assertTrue "sent tx ($3): $2" $1); then TX=$2 - assertEquals "good check ($3): $TX" "0" $(echo $TX | jq .check_tx.code) - assertEquals "good deliver ($3): $TX" "0" $(echo $TX | jq .deliver_tx.code) + assertEquals "line=${LINENO}, good check ($3): $TX" "0" $(echo $TX | jq .check_tx.code) + assertEquals "line=${LINENO}, good deliver ($3): $TX" "0" $(echo $TX | jq .deliver_tx.code) else return 1 fi @@ -155,18 +155,19 @@ txSucceeded() { # and that the first input was from this sender for this amount checkSendTx() { TX=$(${CLIENT_EXE} query tx $1) - assertTrue "found tx" $? + assertTrue "line=${LINENO}, found tx" $? if [ -n "$DEBUG" ]; then echo $TX; echo; fi - 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) + assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) + assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) + NTX=$(echo $TX | jq .data.data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + CTX=$(echo $NTX | jq .data.tx) + assertEquals "line=${LINENO}, 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) - assertEquals "proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) - assertEquals "proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) + assertEquals "line=${LINENO}, type=coin/send" '"coin/send"' $(echo $STX | jq .type) + assertEquals "line=${LINENO}, proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) + assertEquals "line=${LINENO}, proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) return $? } @@ -179,17 +180,19 @@ checkSendFeeTx() { assertTrue "found tx" $? if [ -n "$DEBUG" ]; then echo $TX; echo; fi - 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=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) + assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) + assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) + NTX=$(echo $TX | jq .data.data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + CTX=$(echo $NTX | jq .data.tx) + assertEquals "line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) FTX=$(echo $CTX | jq .data.tx) - assertEquals "type=fee/tx" '"fee/tx"' $(echo $FTX | jq .type) - assertEquals "proper fee" "$5" $(echo $FTX | jq .data.fee.amount) + assertEquals "line=${LINENO}, type=fee/tx" '"fee/tx"' $(echo $FTX | jq .type) + assertEquals "line=${LINENO}, proper fee" "$5" $(echo $FTX | jq .data.fee.amount) STX=$(echo $FTX | jq .data.tx) - assertEquals "type=coin/send" '"coin/send"' $(echo $STX | jq .type) - assertEquals "proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) - assertEquals "proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) + assertEquals "line=${LINENO}, type=coin/send" '"coin/send"' $(echo $STX | jq .type) + assertEquals "line=${LINENO}, proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) + assertEquals "line=${LINENO}, proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) return $? } diff --git a/tests/cli/counter.sh b/tests/cli/counter.sh index 30fb29ab40..1cd62c03de 100755 --- a/tests/cli/counter.sh +++ b/tests/cli/counter.sh @@ -21,21 +21,21 @@ test00GetAccount() { SENDER=$(getAddr $RICH) RECV=$(getAddr $POOR) - assertFalse "requires arg" "${CLIENT_EXE} query account" + assertFalse "Line=${LINENO}, requires arg" "${CLIENT_EXE} query account" checkAccount $SENDER "9007199254740992" ACCT2=$(${CLIENT_EXE} query account $RECV 2>/dev/null) - assertFalse "has no genesis account" $? + assertFalse "Line=${LINENO}, has no genesis account" $? } test01SendTx() { SENDER=$(getAddr $RICH) RECV=$(getAddr $POOR) - assertFalse "missing dest" "${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 2>/dev/null" - assertFalse "bad password" "echo foo | ${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 --to=$RECV --name=$RICH 2>/dev/null" - TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 --to=$RECV --name=$RICH 2>/dev/null) + assertFalse "Line=${LINENO}, missing dest" "${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 2>/dev/null" + assertFalse "Line=${LINENO}, bad password" "echo foo | ${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 --to=$RECV --name=$RICH 2>/dev/null" + TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=992mycoin --sequence=1 --to=$RECV --name=$RICH) txSucceeded $? "$TX" "$RECV" HASH=$(echo $TX | jq .hash | tr -d \") TX_HEIGHT=$(echo $TX | jq .height) @@ -49,7 +49,7 @@ test01SendTx() { test02GetCounter() { COUNT=$(${CLIENT_EXE} query counter 2>/dev/null) - assertFalse "no default count" $? + assertFalse "Line=${LINENO}, no default count" $? } # checkCounter $COUNT $BALANCE @@ -57,15 +57,15 @@ test02GetCounter() { checkCounter() { # make sure sender goes down ACCT=$(${CLIENT_EXE} query counter) - if assertTrue "count is set" $?; then - assertEquals "proper count" "$1" $(echo $ACCT | jq .data.counter) - assertEquals "proper money" "$2" $(echo $ACCT | jq .data.total_fees[0].amount) + if assertTrue "Line=${LINENO}, count is set" $?; then + assertEquals "Line=${LINENO}, proper count" "$1" $(echo $ACCT | jq .data.counter) + assertEquals "Line=${LINENO}, proper money" "$2" $(echo $ACCT | jq .data.total_fees[0].amount) fi } test03AddCount() { SENDER=$(getAddr $RICH) - assertFalse "bad password" "echo hi | ${CLIENT_EXE} tx counter --countfee=100mycoin --sequence=2 --name=${RICH} 2>/dev/null" + assertFalse "Line=${LINENO}, bad password" "echo hi | ${CLIENT_EXE} tx counter --countfee=100mycoin --sequence=2 --name=${RICH} 2>/dev/null" TX=$(echo qwertyuiop | ${CLIENT_EXE} tx counter --countfee=10mycoin --sequence=2 --name=${RICH} --valid) txSucceeded $? "$TX" "counter" @@ -80,14 +80,14 @@ test03AddCount() { # make sure tx is indexed TX=$(${CLIENT_EXE} query tx $HASH --trace) - if assertTrue "found tx" $?; then - assertEquals "proper height" $TX_HEIGHT $(echo $TX | jq .height) - assertEquals "type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - CTX=$(echo $TX | jq .data.data.tx) - assertEquals "type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - CNTX=$(echo $CTX | jq .data.tx) - assertEquals "type=cntr/count" '"cntr/count"' $(echo $CNTX | jq .type) - assertEquals "proper fee" "10" $(echo $CNTX | jq .data.fee[0].amount) + if assertTrue "Line=${LINENO}, found tx" $?; then + assertEquals "Line=${LINENO}, proper height" $TX_HEIGHT $(echo $TX | jq .height) + assertEquals "Line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) + CTX=$(echo $TX | jq .data.data.tx) + assertEquals "Line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) + CNTX=$(echo $CTX | jq .data.tx) + assertEquals "Line=${LINENO}, type=cntr/count" '"cntr/count"' $(echo $CNTX | jq .type) + assertEquals "Line=${LINENO}, proper fee" "10" $(echo $CNTX | jq .data.fee[0].amount) fi # test again with fees... From 6d35b1f934e01ff140131370de6fbfc79c85eb78 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Fri, 14 Jul 2017 12:29:26 +0200 Subject: [PATCH 10/19] Clean up nonce wrapper in cli --- cmd/basecli/commands/cmds.go | 41 ++++++++++--------- context.go | 4 ++ .../cmd/countercli/commands/counter.go | 16 ++------ 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index 832151ca1d..1a8118f0dd 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -66,16 +66,6 @@ func doSendTx(cmd *cobra.Command, args []string) error { return err } - //get the nonce accounts - sendTx, ok := tx.Unwrap().(coin.SendTx) - if !ok { - return errors.New("tx not SendTx") - } - var nonceAccount []basecoin.Actor - for _, input := range sendTx.Inputs { - nonceAccount = append(nonceAccount, input.Address) - } - // TODO: make this more flexible for middleware tx, err = WrapFeeTx(tx) if err != nil { @@ -85,13 +75,10 @@ func doSendTx(cmd *cobra.Command, args []string) error { if err != nil { return err } - - //add the nonce tx layer to the tx - seq := viper.GetInt(FlagSequence) - if seq < 0 { - return fmt.Errorf("sequence must be greater than 0") + tx, err = WrapNonceTx(tx) + if err != nil { + return err } - tx = nonce.NewTx(uint32(seq), nonceAccount, tx) // Note: this is single sig (no multi sig yet) stx := auth.NewSig(tx) @@ -106,6 +93,20 @@ func doSendTx(cmd *cobra.Command, args []string) error { return txcmd.OutputTx(bres) } +// WrapNonceTx grabs the sequence number from the flag and wraps +// the tx with this nonce. Grabs the permission from the signer, +// as we still only support single sig on the cli +func WrapNonceTx(tx basecoin.Tx) (res basecoin.Tx, err error) { + //add the nonce tx layer to the tx + seq := viper.GetInt(FlagSequence) + if seq < 0 { + return res, fmt.Errorf("sequence must be greater than 0") + } + signers := []basecoin.Actor{GetSignerAct()} + tx = nonce.NewTx(uint32(seq), signers, tx) + return tx, nil +} + // WrapFeeTx checks for FlagFee and if present wraps the tx with a // FeeTx of the given amount, paid by the signer func WrapFeeTx(tx basecoin.Tx) (res basecoin.Tx, err error) { @@ -118,7 +119,7 @@ func WrapFeeTx(tx basecoin.Tx) (res basecoin.Tx, err error) { if toll.IsZero() { return tx, nil } - return fee.NewFee(tx, toll, getSignerAddr()), nil + return fee.NewFee(tx, toll, GetSignerAct()), nil } // WrapChainTx will wrap the tx with a ChainTx from the standard flags @@ -132,7 +133,9 @@ func WrapChainTx(tx basecoin.Tx) (res basecoin.Tx, err error) { return res, nil } -func getSignerAddr() (res basecoin.Actor) { +// GetSignerAct returns the address of the signer of the tx +// (as we still only support single sig) +func GetSignerAct() (res basecoin.Actor) { // this could be much cooler with multisig... signer := txcmd.GetSigner() if !signer.Empty() { @@ -157,7 +160,7 @@ func readSendTxFlags() (tx basecoin.Tx, err error) { // craft the inputs and outputs ins := []coin.TxInput{{ - Address: getSignerAddr(), + Address: GetSignerAct(), Coins: amountCoins, }} outs := []coin.TxOutput{{ diff --git a/context.go b/context.go index 1308e797c3..65e9bf8abd 100644 --- a/context.go +++ b/context.go @@ -38,6 +38,10 @@ func (a Actor) Equals(b Actor) bool { bytes.Equal(a.Address, b.Address) } +func (a Actor) Empty() bool { + return a.ChainID == "" && a.App == "" && len(a.Address) == 0 +} + // Context is an interface, so we can implement "secure" variants that // rely on private fields to control the actions type Context interface { diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index 3ab9e8e26d..995ec23f30 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -1,8 +1,6 @@ package commands import ( - "fmt" - "github.com/spf13/cobra" "github.com/spf13/viper" @@ -13,7 +11,6 @@ import ( "github.com/tendermint/basecoin/docs/guide/counter/plugins/counter" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/coin" - "github.com/tendermint/basecoin/modules/nonce" ) //CounterTxCmd is the CLI command to execute the counter @@ -58,10 +55,6 @@ func counterTx(cmd *cobra.Command, args []string) error { return err } - //get the nonce accounts - var addr []byte - nonceAccount := []basecoin.Actor{basecoin.NewActor(counter.NameCounter, addr)} - // TODO: make this more flexible for middleware tx, err = bcmd.WrapFeeTx(tx) if err != nil { @@ -71,13 +64,10 @@ func counterTx(cmd *cobra.Command, args []string) error { if err != nil { return err } - - //add the nonce tx layer to the tx - seq := viper.GetInt(FlagSequence) - if seq < 0 { - return fmt.Errorf("sequence must be greater than 0") + tx, err = bcmd.WrapNonceTx(tx) + if err != nil { + return err } - tx = nonce.NewTx(uint32(seq), nonceAccount, tx) stx := auth.NewSig(tx) From e5db61a63a740744a82490734e55f3b821c6598d Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Fri, 14 Jul 2017 12:36:51 +0200 Subject: [PATCH 11/19] Cleanup counter tx check, add replay protection to cli tests --- tests/cli/basictx.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/cli/basictx.sh b/tests/cli/basictx.sh index 0ecc4d9711..b35dd0be4f 100755 --- a/tests/cli/basictx.sh +++ b/tests/cli/basictx.sh @@ -64,6 +64,12 @@ test02SendTxWithFee() { # Make sure tx is indexed checkSendFeeTx $HASH $TX_HEIGHT $SENDER "90" "10" + + # assert replay protection + TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=90mycoin --fee=10mycoin --sequence=2 --to=$RECV --name=$RICH) + assertFalse "replay: $TX" $? + checkAccount $SENDER "9007199254739900" + checkAccount $RECV "1082" } From 9fd250209eb7c46321a166fc50ed9fd6257f03dd Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Fri, 14 Jul 2017 12:47:17 +0200 Subject: [PATCH 12/19] Cli now returns errors on non-zero code from DeliverTx --- cmd/basecli/commands/cmds.go | 19 ++++++++++++++++++- .../cmd/countercli/commands/counter.go | 3 +++ tests/cli/basictx.sh | 2 +- tests/cli/counter.sh | 10 +++++++++- 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index 1a8118f0dd..2ab581c8bb 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -9,11 +9,13 @@ import ( "github.com/spf13/cobra" "github.com/spf13/viper" - "github.com/tendermint/basecoin" "github.com/tendermint/light-client/commands" txcmd "github.com/tendermint/light-client/commands/txs" cmn "github.com/tendermint/tmlibs/common" + ctypes "github.com/tendermint/tendermint/rpc/core/types" + + "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/base" "github.com/tendermint/basecoin/modules/coin" @@ -88,11 +90,26 @@ func doSendTx(cmd *cobra.Command, args []string) error { if err != nil { return err } + if err = ValidateResult(bres); err != nil { + return err + } // Output result return txcmd.OutputTx(bres) } +// ValidateResult returns an appropriate error if the server rejected the +// tx in CheckTx or DeliverTx +func ValidateResult(res *ctypes.ResultBroadcastTxCommit) error { + if res.CheckTx.IsErr() { + return fmt.Errorf("CheckTx: (%d): %s", res.CheckTx.Code, res.CheckTx.Log) + } + if res.DeliverTx.IsErr() { + return fmt.Errorf("DeliverTx: (%d): %s", res.DeliverTx.Code, res.DeliverTx.Log) + } + return nil +} + // WrapNonceTx grabs the sequence number from the flag and wraps // the tx with this nonce. Grabs the permission from the signer, // as we still only support single sig on the cli diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index 995ec23f30..a3746b8671 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -76,6 +76,9 @@ func counterTx(cmd *cobra.Command, args []string) error { if err != nil { return err } + if err = bcmd.ValidateResult(bres); err != nil { + return err + } // Output result return txcmd.OutputTx(bres) diff --git a/tests/cli/basictx.sh b/tests/cli/basictx.sh index b35dd0be4f..80d5c71f4e 100755 --- a/tests/cli/basictx.sh +++ b/tests/cli/basictx.sh @@ -66,7 +66,7 @@ test02SendTxWithFee() { checkSendFeeTx $HASH $TX_HEIGHT $SENDER "90" "10" # assert replay protection - TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=90mycoin --fee=10mycoin --sequence=2 --to=$RECV --name=$RICH) + TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=90mycoin --fee=10mycoin --sequence=2 --to=$RECV --name=$RICH 2>/dev/null) assertFalse "replay: $TX" $? checkAccount $SENDER "9007199254739900" checkAccount $RECV "1082" diff --git a/tests/cli/counter.sh b/tests/cli/counter.sh index 1cd62c03de..4c56eeaf88 100755 --- a/tests/cli/counter.sh +++ b/tests/cli/counter.sh @@ -83,7 +83,9 @@ test03AddCount() { if assertTrue "Line=${LINENO}, found tx" $?; then assertEquals "Line=${LINENO}, proper height" $TX_HEIGHT $(echo $TX | jq .height) assertEquals "Line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - CTX=$(echo $TX | jq .data.data.tx) + NTX=$(echo $TX | jq .data.data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + CTX=$(echo $NTX | jq .data.tx) assertEquals "Line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) CNTX=$(echo $CTX | jq .data.tx) assertEquals "Line=${LINENO}, type=cntr/count" '"cntr/count"' $(echo $CNTX | jq .type) @@ -99,6 +101,12 @@ test03AddCount() { # make sure the account was debited 11 checkAccount $SENDER "9007199254739979" + + # make sure we cannot replay the counter, no state change + TX=$(echo qwertyuiop | ${CLIENT_EXE} tx counter --countfee=10mycoin --sequence=2 --name=${RICH} --valid 2>/dev/null) + assertFalse "replay: $TX" $? + checkCounter "2" "17" + checkAccount $SENDER "9007199254739979" } # Load common then run these tests with shunit2! From 007230e583332ad76b147a8cf22e0dd47b758d4f Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Fri, 14 Jul 2017 13:52:07 -0400 Subject: [PATCH 13/19] swap the nonce & chain check order in stack --- app/app.go | 2 +- app/app_test.go | 4 ++-- cmd/basecli/commands/cmds.go | 4 ++-- docs/guide/counter/cmd/countercli/commands/counter.go | 4 ++-- docs/guide/counter/plugins/counter/counter.go | 2 +- docs/guide/counter/plugins/counter/counter_test.go | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/app.go b/app/app.go index 313c719338..bdcdbbbd80 100644 --- a/app/app.go +++ b/app/app.go @@ -62,8 +62,8 @@ func DefaultHandler(feeDenom string) basecoin.Handler { base.Logger{}, stack.Recovery{}, auth.Signatures{}, - nonce.ReplayCheck{}, base.Chain{}, + nonce.ReplayCheck{}, fee.NewSimpleFeeMiddleware(coin.Coin{feeDenom, 0}, fee.Bank), ).Use(d) } diff --git a/app/app_test.go b/app/app_test.go index 813c50afe2..629ec750c1 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -58,16 +58,16 @@ func (at *appTest) signTx(tx basecoin.Tx) basecoin.Tx { func (at *appTest) getTx(coins coin.Coins) basecoin.Tx { tx := at.baseTx(coins) - tx = base.NewChainTx(at.chainID, 0, tx) tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin) basecoin.Tx { tx := at.baseTx(coins) tx = fee.NewFee(tx, toll, at.acctIn.Actor()) - tx = base.NewChainTx(at.chainID, 0, tx) tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index 2ab581c8bb..cf97f9581c 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -73,11 +73,11 @@ func doSendTx(cmd *cobra.Command, args []string) error { if err != nil { return err } - tx, err = WrapChainTx(tx) + tx, err = WrapNonceTx(tx) if err != nil { return err } - tx, err = WrapNonceTx(tx) + tx, err = WrapChainTx(tx) if err != nil { return err } diff --git a/docs/guide/counter/cmd/countercli/commands/counter.go b/docs/guide/counter/cmd/countercli/commands/counter.go index a3746b8671..68fb867006 100644 --- a/docs/guide/counter/cmd/countercli/commands/counter.go +++ b/docs/guide/counter/cmd/countercli/commands/counter.go @@ -60,11 +60,11 @@ func counterTx(cmd *cobra.Command, args []string) error { if err != nil { return err } - tx, err = bcmd.WrapChainTx(tx) + tx, err = bcmd.WrapNonceTx(tx) if err != nil { return err } - tx, err = bcmd.WrapNonceTx(tx) + tx, err = bcmd.WrapChainTx(tx) if err != nil { return err } diff --git a/docs/guide/counter/plugins/counter/counter.go b/docs/guide/counter/plugins/counter/counter.go index d00f0fa901..d99a3c41ea 100644 --- a/docs/guide/counter/plugins/counter/counter.go +++ b/docs/guide/counter/plugins/counter/counter.go @@ -102,8 +102,8 @@ func NewHandler(feeDenom string) basecoin.Handler { base.Logger{}, stack.Recovery{}, auth.Signatures{}, - nonce.ReplayCheck{}, base.Chain{}, + nonce.ReplayCheck{}, fee.NewSimpleFeeMiddleware(coin.Coin{feeDenom, 0}, fee.Bank), ).Use(dispatcher) } diff --git a/docs/guide/counter/plugins/counter/counter_test.go b/docs/guide/counter/plugins/counter/counter_test.go index b8b79a7432..ff880c5099 100644 --- a/docs/guide/counter/plugins/counter/counter_test.go +++ b/docs/guide/counter/plugins/counter/counter_test.go @@ -44,8 +44,8 @@ func TestCounterPlugin(t *testing.T) { // Deliver a CounterTx DeliverCounterTx := func(valid bool, counterFee coin.Coins, sequence uint32) abci.Result { tx := NewTx(valid, counterFee) - tx = base.NewChainTx(chainID, 0, tx) tx = nonce.NewTx(sequence, []basecoin.Actor{acct.Actor()}, tx) + tx = base.NewChainTx(chainID, 0, tx) stx := auth.NewSig(tx) auth.Sign(stx, acct.Key) txBytes := wire.BinaryBytes(stx.Wrap()) From 3d5cf393b938a1041e8ff8217f610d0d808e1dbf Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Fri, 14 Jul 2017 16:29:43 -0400 Subject: [PATCH 14/19] PR changes --- app/app_test.go | 20 ++++++------ cmd/basecli/commands/cmds.go | 9 +++--- docs/guide/counter/plugins/counter/counter.go | 5 ++- errors/common.go | 20 ------------ modules/nonce/errors.go | 32 +++++++++++++++++++ modules/nonce/replaycheck.go | 10 ++++-- modules/nonce/tx.go | 6 ++-- modules/nonce/tx_test.go | 32 ++++++++++++++++--- tests/cli/common.sh | 16 +++++----- tests/cli/counter.sh | 8 ++--- 10 files changed, 99 insertions(+), 59 deletions(-) create mode 100644 modules/nonce/errors.go diff --git a/app/app_test.go b/app/app_test.go index 629ec750c1..2342bf2414 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -56,17 +56,17 @@ func (at *appTest) signTx(tx basecoin.Tx) basecoin.Tx { return stx.Wrap() } -func (at *appTest) getTx(coins coin.Coins) basecoin.Tx { +func (at *appTest) getTx(coins coin.Coins, sequence uint32) basecoin.Tx { tx := at.baseTx(coins) - tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = nonce.NewTx(sequence, []basecoin.Actor{at.acctIn.Actor()}, tx) tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } -func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin) basecoin.Tx { +func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin, sequence uint32) basecoin.Tx { tx := at.baseTx(coins) tx = fee.NewFee(tx, toll, at.acctIn.Actor()) - tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = nonce.NewTx(sequence, []basecoin.Actor{at.acctIn.Actor()}, tx) tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } @@ -213,22 +213,22 @@ func TestTx(t *testing.T) { //Bad Balance at.acctIn.Coins = coin.Coins{{"mycoin", 2}} at.initAccount(at.acctIn) - res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), true) + res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), true) assert.True(res.IsErr(), "ExecTx/Bad CheckTx: Expected error return from ExecTx, returned: %v", res) - res, diffIn, diffOut := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), false) + res, diffIn, diffOut := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), false) assert.True(res.IsErr(), "ExecTx/Bad DeliverTx: Expected error return from ExecTx, returned: %v", res) assert.True(diffIn.IsZero()) assert.True(diffOut.IsZero()) //Regular CheckTx at.reset() - res, _, _ = at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), true) + res, _, _ = at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), true) assert.True(res.IsOK(), "ExecTx/Good CheckTx: Expected OK return from ExecTx, Error: %v", res) //Regular DeliverTx at.reset() amt := coin.Coins{{"mycoin", 3}} - res, diffIn, diffOut = at.exec(t, at.getTx(amt), false) + res, diffIn, diffOut = at.exec(t, at.getTx(amt, 1), false) assert.True(res.IsOK(), "ExecTx/Good DeliverTx: Expected OK return from ExecTx, Error: %v", res) assert.Equal(amt.Negative(), diffIn) assert.Equal(amt, diffOut) @@ -237,7 +237,7 @@ func TestTx(t *testing.T) { at.reset() amt = coin.Coins{{"mycoin", 4}} toll := coin.Coin{"mycoin", 1} - res, diffIn, diffOut = at.exec(t, at.feeTx(amt, toll), false) + res, diffIn, diffOut = at.exec(t, at.feeTx(amt, toll, 1), false) assert.True(res.IsOK(), "ExecTx/Good DeliverTx: Expected OK return from ExecTx, Error: %v", res) payment := amt.Plus(coin.Coins{toll}).Negative() assert.Equal(payment, diffIn) @@ -249,7 +249,7 @@ func TestQuery(t *testing.T) { assert := assert.New(t) at := newAppTest(t) - res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), false) + res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), false) assert.True(res.IsOK(), "Commit, DeliverTx: Expected OK return from DeliverTx, Error: %v", res) resQueryPreCommit := at.app.Query(abci.RequestQuery{ diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index cf97f9581c..3732f38fc0 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -120,8 +120,8 @@ func WrapNonceTx(tx basecoin.Tx) (res basecoin.Tx, err error) { return res, fmt.Errorf("sequence must be greater than 0") } signers := []basecoin.Actor{GetSignerAct()} - tx = nonce.NewTx(uint32(seq), signers, tx) - return tx, nil + res = nonce.NewTx(uint32(seq), signers, tx) + return } // WrapFeeTx checks for FlagFee and if present wraps the tx with a @@ -136,7 +136,8 @@ func WrapFeeTx(tx basecoin.Tx) (res basecoin.Tx, err error) { if toll.IsZero() { return tx, nil } - return fee.NewFee(tx, toll, GetSignerAct()), nil + res = fee.NewFee(tx, toll, GetSignerAct()) + return } // WrapChainTx will wrap the tx with a ChainTx from the standard flags @@ -147,7 +148,7 @@ func WrapChainTx(tx basecoin.Tx) (res basecoin.Tx, err error) { return res, errors.New("No chain-id provided") } res = base.NewChainTx(chain, uint64(expires), tx) - return res, nil + return } // GetSignerAct returns the address of the signer of the tx diff --git a/docs/guide/counter/plugins/counter/counter.go b/docs/guide/counter/plugins/counter/counter.go index d99a3c41ea..df07195f4c 100644 --- a/docs/guide/counter/plugins/counter/counter.go +++ b/docs/guide/counter/plugins/counter/counter.go @@ -35,9 +35,8 @@ func init() { // Tx - struct for all counter transactions type Tx struct { - Valid bool `json:"valid"` - Fee coin.Coins `json:"fee"` - Sequence int `json:""` + Valid bool `json:"valid"` + Fee coin.Coins `json:"fee"` } // NewTx - return a new counter transaction struct wrapped as a basecoin transaction diff --git a/errors/common.go b/errors/common.go index a6f37a71ab..1ef42177f9 100644 --- a/errors/common.go +++ b/errors/common.go @@ -19,20 +19,14 @@ var ( 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 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") errUnknownModule = fmt.Errorf("Unknown module") errExpired = fmt.Errorf("Tx expired") errUnknownKey = fmt.Errorf("Unknown key") -) -var ( internalErr = abci.CodeType_InternalError encodingErr = abci.CodeType_EncodingError unauthorized = abci.CodeType_Unauthorized @@ -142,20 +136,6 @@ func IsNoChainErr(err error) bool { return IsSameError(errNoChain, err) } -func ErrNoNonce() TMError { - return WithCode(errNoNonce, unauthorized) -} -func ErrBadNonce(got, expected uint32) TMError { - 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) } diff --git a/modules/nonce/errors.go b/modules/nonce/errors.go new file mode 100644 index 0000000000..f88629b457 --- /dev/null +++ b/modules/nonce/errors.go @@ -0,0 +1,32 @@ +//nolint +package nonce + +import ( + "fmt" + + abci "github.com/tendermint/abci/types" + + "github.com/tendermint/basecoin/errors" +) + +var ( + errNoNonce = fmt.Errorf("Tx doesn't contain nonce") + errNotMember = fmt.Errorf("nonce contains non-permissioned member") + errZeroSequence = fmt.Errorf("Sequence number cannot be zero") + + unauthorized = abci.CodeType_Unauthorized +) + +func ErrBadNonce(got, expected uint32) errors.TMError { + return errors.WithCode(fmt.Errorf("Bad nonce sequence, got %d, expected %d", got, expected), unauthorized) +} + +func ErrNoNonce() errors.TMError { + return errors.WithCode(errNoNonce, unauthorized) +} +func ErrNotMember() errors.TMError { + return errors.WithCode(errNotMember, unauthorized) +} +func ErrZeroSequence() errors.TMError { + return errors.WithCode(errZeroSequence, unauthorized) +} diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go index b9f28d253c..4494a2b109 100644 --- a/modules/nonce/replaycheck.go +++ b/modules/nonce/replaycheck.go @@ -2,7 +2,6 @@ package nonce import ( "github.com/tendermint/basecoin" - "github.com/tendermint/basecoin/errors" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" ) @@ -57,11 +56,16 @@ func (r ReplayCheck) checkIncrementNonceTx(ctx basecoin.Context, store state.KVS // make sure it is a the nonce Tx (Tx from this package) nonceTx, ok := tx.Unwrap().(Tx) if !ok { - return tx, errors.ErrNoNonce() + return tx, ErrNoNonce() + } + + err := nonceTx.ValidateBasic() + if err != nil { + return tx, err } // check the nonce sequence number - err := nonceTx.CheckIncrementSeq(ctx, store) + err = nonceTx.CheckIncrementSeq(ctx, store) if err != nil { return tx, err } diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index bb3d197297..819ea12452 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -52,7 +52,7 @@ func (n Tx) ValidateBasic() error { case n.Tx.Empty(): return errors.ErrTxEmpty() case n.Sequence == 0: - return errors.ErrZeroSequence() + return ErrZeroSequence() case len(n.Signers) == 0: return errors.ErrNoSigners() } @@ -73,13 +73,13 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { return err } if n.Sequence != cur+1 { - return errors.ErrBadNonce(n.Sequence, cur+1) + return ErrBadNonce(n.Sequence, cur+1) } // make sure they all signed for _, s := range n.Signers { if !ctx.HasPermission(s) { - return errors.ErrNotMember() + return ErrNotMember() } } diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index 2d4406207d..0f967cd9b8 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -17,13 +17,25 @@ func TestNonce(t *testing.T) { // generic args here... chainID := "my-chain" + chain2ID := "woohoo" + height := uint64(100) ctx := stack.MockContext(chainID, height) store := state.NewMemKVStore() - 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}} + appName1 := "fooz" + appName2 := "foot" + + //root actors for the tests + act1 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{1, 2, 3, 4}} + act2 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{1, 1, 1, 1}} + act3 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{3, 3, 3, 3}} + act1DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{1, 2, 3, 4}} + act2DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{1, 1, 1, 1}} + act3DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{3, 3, 3, 3}} + act1DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{1, 2, 3, 4}} + act2DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{1, 1, 1, 1}} + act3DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{3, 3, 3, 3}} // let's construct some tests to make the table a bit less verbose set0 := []basecoin.Actor{} @@ -34,6 +46,12 @@ func TestNonce(t *testing.T) { set123 := []basecoin.Actor{act1, act2, act3} set321 := []basecoin.Actor{act3, act2, act1} + //some more test cases for different chains and apps for each actor + set123Chain2 := []basecoin.Actor{act1DiffChain, act2DiffChain, act3DiffChain} + set123App2 := []basecoin.Actor{act1DiffApp, act2DiffApp, act3DiffApp} + set123MixedChains := []basecoin.Actor{act1, act2DiffChain, act3} + set123MixedApps := []basecoin.Actor{act1, act2DiffApp, act3} + testList := []struct { valid bool seq uint32 @@ -60,12 +78,18 @@ func TestNonce(t *testing.T) { {false, 2, set12, set12}, // but can't repeat sequence {true, 3, set12, set321}, // no effect from extra sigs - // tripple sigs also work + // triple 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 + + // signers from different chain and apps + {false, 3, set123, set123Chain2}, // sign with different chain actors + {false, 3, set123, set123App2}, // sign with different app actors + {false, 3, set123, set123MixedChains}, // sign with mixed chain actor + {false, 3, set123, set123MixedApps}, // sign with mixed app actors } raw := stack.NewRawTx([]byte{42}) diff --git a/tests/cli/common.sh b/tests/cli/common.sh index 6242086f07..b195184b43 100644 --- a/tests/cli/common.sh +++ b/tests/cli/common.sh @@ -160,11 +160,11 @@ checkSendTx() { assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - STX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + STX=$(echo $NTX | jq .data.tx) assertEquals "line=${LINENO}, type=coin/send" '"coin/send"' $(echo $STX | jq .type) assertEquals "line=${LINENO}, proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) assertEquals "line=${LINENO}, proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) @@ -182,11 +182,11 @@ checkSendFeeTx() { assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - FTX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + FTX=$(echo $NTX | jq .data.tx) assertEquals "line=${LINENO}, type=fee/tx" '"fee/tx"' $(echo $FTX | jq .type) assertEquals "line=${LINENO}, proper fee" "$5" $(echo $FTX | jq .data.fee.amount) STX=$(echo $FTX | jq .data.tx) diff --git a/tests/cli/counter.sh b/tests/cli/counter.sh index 4c56eeaf88..f21c9066d0 100755 --- a/tests/cli/counter.sh +++ b/tests/cli/counter.sh @@ -83,11 +83,11 @@ test03AddCount() { if assertTrue "Line=${LINENO}, found tx" $?; then assertEquals "Line=${LINENO}, proper height" $TX_HEIGHT $(echo $TX | jq .height) assertEquals "Line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "Line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - CNTX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + CNTX=$(echo $NTX | jq .data.tx) assertEquals "Line=${LINENO}, type=cntr/count" '"cntr/count"' $(echo $CNTX | jq .type) assertEquals "Line=${LINENO}, proper fee" "10" $(echo $CNTX | jq .data.fee[0].amount) fi From 84a376eecbb3e774ff9d17aadc542a0c8b7cbfe3 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Sun, 16 Jul 2017 20:38:51 +0200 Subject: [PATCH 15/19] Failing test to demo seqkey generation issue --- modules/nonce/tx.go | 1 + modules/nonce/tx_test.go | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index 819ea12452..b5eaddb508 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -101,6 +101,7 @@ func (n Tx) getSeqKey() (seqKey []byte) { sort.Sort(basecoin.ByAddress(n.Signers)) for _, signer := range n.Signers { + // rigel: use signer.Bytes()... instead of signer.Address seqKey = append(seqKey, signer.Address...) } //seqKey = merkle.SimpleHashFromBinary(n.Signers) diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index 0f967cd9b8..292c5ed230 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -90,6 +90,14 @@ func TestNonce(t *testing.T) { {false, 3, set123, set123App2}, // sign with different app actors {false, 3, set123, set123MixedChains}, // sign with mixed chain actor {false, 3, set123, set123MixedApps}, // sign with mixed app actors + + // Rigel: this is the problem I was refering to. + // The sig checks are proper. But the seqkey is not unique + // all of these demand 3, as that what is expected for set123 + {true, 1, set123Chain2, set123Chain2}, + {true, 1, set123App2, set123App2}, + {true, 1, set123MixedChains, set123MixedChains}, + {true, 1, set123MixedApps, set123MixedApps}, } raw := stack.NewRawTx([]byte{42}) @@ -109,4 +117,5 @@ func TestNonce(t *testing.T) { assert.NotNil(err, "%d", i) } } + } From 71276a53b5035a55395b0bc111c5968d002caaae Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Sun, 16 Jul 2017 20:48:41 +0200 Subject: [PATCH 16/19] A failing cli test demonstating the missing query nonce command --- tests/cli/basictx.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/cli/basictx.sh b/tests/cli/basictx.sh index 80d5c71f4e..009e1ddc02 100755 --- a/tests/cli/basictx.sh +++ b/tests/cli/basictx.sh @@ -70,6 +70,15 @@ test02SendTxWithFee() { assertFalse "replay: $TX" $? checkAccount $SENDER "9007199254739900" checkAccount $RECV "1082" + + # make sure we can query the proper nonce + NONCE=$(${CLIENT_EXE} query nonce $SENDER) + if [ -n "$DEBUG" ]; then echo $NONCE; echo; fi + # TODO: note that cobra returns error code 0 on parse failure, + # so currently this check passes even if there is no nonce query command + if assertTrue "no nonce query" $?; then + assertEquals "line=${LINENO}, proper nonce" "2" $(echo $NONCE | jq .data) + fi } From f43fceeb4ded6b8ffb0521d8dbc74bfdf470b75c Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Mon, 17 Jul 2017 22:50:10 -0400 Subject: [PATCH 17/19] fix seqkey uses chain and app --- context.go | 32 ++++++++++++++++++++++++-------- modules/nonce/tx.go | 5 ++--- modules/nonce/tx_test.go | 6 ++---- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/context.go b/context.go index 65e9bf8abd..4da626ed3d 100644 --- a/context.go +++ b/context.go @@ -38,6 +38,7 @@ func (a Actor) Equals(b Actor) bool { bytes.Equal(a.Address, b.Address) } +// Empty checks if the actor is not initialized func (a Actor) Empty() bool { return a.ChainID == "" && a.App == "" && len(a.Address) == 0 } @@ -57,19 +58,34 @@ type Context interface { } //////////////////////////////// Sort Interface -// USAGE sort.Sort(ByAddress()) +// USAGE sort.Sort(ByAll()) func (a Actor) String() string { return fmt.Sprintf("%x", a.Address) } -// ByAddress implements sort.Interface for []Actor based on -// the Address field. -type ByAddress []Actor +// ByAll implements sort.Interface for []Actor. +// It sorts be the ChainID, followed by the App, followed by the Address +type ByAll []Actor // Verify the sort interface at compile time -var _ sort.Interface = ByAddress{} +var _ sort.Interface = ByAll{} -func (a ByAddress) Len() int { return len(a) } -func (a ByAddress) Swap(i, j int) { a[i], a[j] = a[j], a[i] } -func (a ByAddress) Less(i, j int) bool { return bytes.Compare(a[i].Address, a[j].Address) == -1 } +func (a ByAll) Len() int { return len(a) } +func (a ByAll) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a ByAll) Less(i, j int) bool { + + if a[i].ChainID < a[j].ChainID { + return true + } + if a[i].ChainID > a[j].ChainID { + return false + } + if a[i].App < a[j].App { + return true + } + if a[i].App > a[j].App { + return false + } + return bytes.Compare(a[i].Address, a[j].Address) == -1 +} diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index b5eaddb508..a31536e5e8 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -98,11 +98,10 @@ 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)) + sort.Sort(basecoin.ByAll(n.Signers)) for _, signer := range n.Signers { - // rigel: use signer.Bytes()... instead of signer.Address - seqKey = append(seqKey, signer.Address...) + seqKey = append(seqKey, signer.Bytes()...) } //seqKey = merkle.SimpleHashFromBinary(n.Signers) return diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index 292c5ed230..218686deec 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -85,15 +85,13 @@ func TestNonce(t *testing.T) { {true, 2, set321, set321}, // other order is the same {false, 2, set321, set321}, // no repetition - // signers from different chain and apps + // signers with different chain-IDs and apps from actors {false, 3, set123, set123Chain2}, // sign with different chain actors {false, 3, set123, set123App2}, // sign with different app actors {false, 3, set123, set123MixedChains}, // sign with mixed chain actor {false, 3, set123, set123MixedApps}, // sign with mixed app actors - // Rigel: this is the problem I was refering to. - // The sig checks are proper. But the seqkey is not unique - // all of these demand 3, as that what is expected for set123 + // signers from different chain-IDs and apps, working {true, 1, set123Chain2, set123Chain2}, {true, 1, set123App2, set123App2}, {true, 1, set123MixedChains, set123MixedChains}, From cb00c00f0d262de4288f65da4ab83d857958078b Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Tue, 18 Jul 2017 05:26:25 -0400 Subject: [PATCH 18/19] getting query command to operate --- cmd/basecli/commands/query.go | 32 ++++++++++++++++++++++++++++++++ cmd/basecli/main.go | 1 + modules/nonce/tx.go | 16 ++++++++++------ tests/cli/basictx.sh | 1 + 4 files changed, 44 insertions(+), 6 deletions(-) diff --git a/cmd/basecli/commands/query.go b/cmd/basecli/commands/query.go index 3d5b4fb066..3ea3626ce4 100644 --- a/cmd/basecli/commands/query.go +++ b/cmd/basecli/commands/query.go @@ -13,6 +13,7 @@ import ( "github.com/tendermint/basecoin/modules/auth" "github.com/tendermint/basecoin/modules/coin" + "github.com/tendermint/basecoin/modules/nonce" "github.com/tendermint/basecoin/stack" ) @@ -41,6 +42,37 @@ func doAccountQuery(cmd *cobra.Command, args []string) error { return proofcmd.OutputProof(acc, proof.BlockHeight()) } +// NonceQueryCmd - command to query an nonce account +var NonceQueryCmd = &cobra.Command{ + Use: "nonce [address]", + Short: "Get details of a nonce sequence number, with proof", + RunE: lcmd.RequireInit(doNonceQuery), +} + +func doNonceQuery(cmd *cobra.Command, args []string) error { + addr, err := proofcmd.ParseHexKey(args, "address") + if err != nil { + return err + } + + act := []basecoin.Actor{basecoin.NewActor( + nonce.NameNonce, + addr, + )} + + key := stack.PrefixedKey(nonce.NameNonce, nonce.GetSeqKey(act)) + + var seq uint32 + proof, err := proofcmd.GetAndParseAppProof(key, &seq) + if lc.IsNoDataErr(err) { + return errors.Errorf("Sequence is empty for address %X ", addr) + } else if err != nil { + return err + } + + return proofcmd.OutputProof(seq, proof.BlockHeight()) +} + // BaseTxPresenter this decodes all basecoin tx type BaseTxPresenter struct { proofs.RawPresenter // this handles MakeKey as hex bytes diff --git a/cmd/basecli/main.go b/cmd/basecli/main.go index 4d96fc3f08..48dcf1dd59 100644 --- a/cmd/basecli/main.go +++ b/cmd/basecli/main.go @@ -39,6 +39,7 @@ func main() { proofs.TxCmd, proofs.KeyCmd, bcmd.AccountQueryCmd, + bcmd.NonceQueryCmd, ) // you will always want this for the base send command diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index a31536e5e8..7a70878b2d 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -92,17 +92,21 @@ 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) { + return GetSeqKey(n.Signers) +} + +// GetSeqKey - Generate the sequence key as the concatenated list of signers, sorted by address. +func GetSeqKey(signers []basecoin.Actor) (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.ByAll(n.Signers)) + signers2sort := make([]basecoin.Actor, len(signers)) + copy(signers2sort, signers) + sort.Sort(basecoin.ByAll(signers)) - for _, signer := range n.Signers { + for _, signer := range signers { seqKey = append(seqKey, signer.Bytes()...) } - //seqKey = merkle.SimpleHashFromBinary(n.Signers) + return } diff --git a/tests/cli/basictx.sh b/tests/cli/basictx.sh index 009e1ddc02..688ebdcdcb 100755 --- a/tests/cli/basictx.sh +++ b/tests/cli/basictx.sh @@ -73,6 +73,7 @@ test02SendTxWithFee() { # make sure we can query the proper nonce NONCE=$(${CLIENT_EXE} query nonce $SENDER) + echo $NONCE if [ -n "$DEBUG" ]; then echo $NONCE; echo; fi # TODO: note that cobra returns error code 0 on parse failure, # so currently this check passes even if there is no nonce query command From 8dc5fc718dda7117b4ae8a8d1faefd72c5a76bb3 Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Tue, 18 Jul 2017 05:43:17 -0400 Subject: [PATCH 19/19] tests working --- cmd/basecli/commands/query.go | 2 +- tests/cli/basictx.sh | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/basecli/commands/query.go b/cmd/basecli/commands/query.go index 3ea3626ce4..ce3fa338ca 100644 --- a/cmd/basecli/commands/query.go +++ b/cmd/basecli/commands/query.go @@ -56,7 +56,7 @@ func doNonceQuery(cmd *cobra.Command, args []string) error { } act := []basecoin.Actor{basecoin.NewActor( - nonce.NameNonce, + auth.NameSigs, addr, )} diff --git a/tests/cli/basictx.sh b/tests/cli/basictx.sh index 688ebdcdcb..009e1ddc02 100755 --- a/tests/cli/basictx.sh +++ b/tests/cli/basictx.sh @@ -73,7 +73,6 @@ test02SendTxWithFee() { # make sure we can query the proper nonce NONCE=$(${CLIENT_EXE} query nonce $SENDER) - echo $NONCE if [ -n "$DEBUG" ]; then echo $NONCE; echo; fi # TODO: note that cobra returns error code 0 on parse failure, # so currently this check passes even if there is no nonce query command