From 5da2b75fa0a4f3ca7ecda3554be66c7d61c482b0 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 18 Jul 2017 14:27:53 +0200 Subject: [PATCH] Clean up ibc PostPacket handling --- context.go | 11 +++++++ modules/ibc/errors.go | 62 +++++++++++++++++++++++++++++++-------- modules/ibc/handler.go | 13 ++++---- modules/ibc/middleware.go | 44 +++++++++++++++------------ modules/ibc/provider.go | 13 ++++++++ modules/ibc/store.go | 8 ++--- modules/ibc/tx.go | 31 +------------------- 7 files changed, 110 insertions(+), 72 deletions(-) diff --git a/context.go b/context.go index 24535dc0ad..d22102e941 100644 --- a/context.go +++ b/context.go @@ -50,6 +50,17 @@ func (a Actor) WithChain(chainID string) (b Actor) { return } +type Actors []Actor + +func (a Actors) AllHaveChain(chainID string) bool { + for _, b := range a { + if b.ChainID != chainID { + return false + } + } + return true +} + // 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/modules/ibc/errors.go b/modules/ibc/errors.go index df9d593ece..748c3a948f 100644 --- a/modules/ibc/errors.go +++ b/modules/ibc/errors.go @@ -11,21 +11,22 @@ import ( var ( errChainNotRegistered = fmt.Errorf("Chain not registered") errChainAlreadyExists = fmt.Errorf("Chain already exists") + errWrongDestChain = fmt.Errorf("This is not the destination") errNeedsIBCPermission = fmt.Errorf("Needs app-permission to send IBC") errCannotSetPermission = fmt.Errorf("Requesting invalid permission on IBC") - // errNotMember = fmt.Errorf("Not a member") - // errInsufficientSigs = fmt.Errorf("Not enough signatures") - // errNoMembers = fmt.Errorf("No members specified") - // errTooManyMembers = fmt.Errorf("Too many members specified") - // errNotEnoughMembers = fmt.Errorf("Not enough members specified") + errHeaderNotFound = fmt.Errorf("Header not found") + errPacketAlreadyExists = fmt.Errorf("Packet already handled") + errPacketOutOfOrder = fmt.Errorf("Packet out of order") + errInvalidProof = fmt.Errorf("Invalid merkle proof") - IBCCodeChainNotRegistered = abci.CodeType(1001) - IBCCodeChainAlreadyExists = abci.CodeType(1002) - IBCCodePacketAlreadyExists = abci.CodeType(1003) - IBCCodeUnknownHeight = abci.CodeType(1004) - IBCCodeInvalidCommit = abci.CodeType(1005) - IBCCodeInvalidProof = abci.CodeType(1006) - IBCCodeInvalidCall = abci.CodeType(1007) + IBCCodeChainNotRegistered = abci.CodeType(1001) + IBCCodeChainAlreadyExists = abci.CodeType(1002) + IBCCodeUnknownChain = abci.CodeType(1003) + IBCCodeInvalidPacketSequence = abci.CodeType(1004) + IBCCodeUnknownHeight = abci.CodeType(1005) + IBCCodeInvalidCommit = abci.CodeType(1006) + IBCCodeInvalidProof = abci.CodeType(1007) + IBCCodeInvalidCall = abci.CodeType(1008) ) func ErrNotRegistered(chainID string) error { @@ -42,6 +43,13 @@ func IsAlreadyRegistetedErr(err error) bool { return errors.IsSameError(errChainAlreadyExists, err) } +func ErrWrongDestChain(chainID string) error { + return errors.WithMessage(chainID, errWrongDestChain, IBCCodeUnknownChain) +} +func IsWrongDestChainErr(err error) bool { + return errors.IsSameError(errWrongDestChain, err) +} + func ErrNeedsIBCPermission() error { return errors.WithCode(errNeedsIBCPermission, IBCCodeInvalidCall) } @@ -55,3 +63,33 @@ func ErrCannotSetPermission() error { func IsCannotSetPermissionErr(err error) bool { return errors.IsSameError(errCannotSetPermission, err) } + +func ErrHeaderNotFound(h int) error { + msg := fmt.Sprintf("height %d", h) + return errors.WithMessage(msg, errHeaderNotFound, IBCCodeUnknownHeight) +} +func IsHeaderNotFoundErr(err error) bool { + return errors.IsSameError(errHeaderNotFound, err) +} + +func ErrPacketAlreadyExists() error { + return errors.WithCode(errPacketAlreadyExists, IBCCodeInvalidPacketSequence) +} +func IsPacketAlreadyExistsErr(err error) bool { + return errors.IsSameError(errPacketAlreadyExists, err) +} + +func ErrPacketOutOfOrder(seq uint64) error { + msg := fmt.Sprintf("expected %d", seq) + return errors.WithMessage(msg, errPacketOutOfOrder, IBCCodeInvalidPacketSequence) +} +func IsPacketOutOfOrderErr(err error) bool { + return errors.IsSameError(errPacketOutOfOrder, err) +} + +func ErrInvalidProof() error { + return errors.WithCode(errInvalidProof, IBCCodeInvalidProof) +} +func IsInvalidProofErr(err error) bool { + return errors.IsSameError(errInvalidProof, err) +} diff --git a/modules/ibc/handler.go b/modules/ibc/handler.go index 91eb3478af..488f556b71 100644 --- a/modules/ibc/handler.go +++ b/modules/ibc/handler.go @@ -92,6 +92,12 @@ func (h Handler) DeliverTx(ctx basecoin.Context, store state.KVStore, tx basecoi switch t := tx.Unwrap().(type) { case RegisterChainTx: + // check permission to attach, do it here, so no permission check + // by SetOption + info := LoadInfo(store) + if !info.Registrar.Empty() && !ctx.HasPermission(info.Registrar) { + return res, errors.ErrUnauthorized() + } return h.initSeed(ctx, store, t) case UpdateChainTx: return h.updateSeed(ctx, store, t) @@ -108,13 +114,6 @@ func (h Handler) DeliverTx(ctx basecoin.Context, store state.KVStore, tx basecoi func (h Handler) initSeed(ctx basecoin.Context, store state.KVStore, t RegisterChainTx) (res basecoin.Result, err error) { - // check permission to attach - // nothing set, means anyone can connect - info := LoadInfo(store) - if !info.Registrar.Empty() && !ctx.HasPermission(info.Registrar) { - return res, errors.ErrUnauthorized() - } - chainID := t.ChainID() s := NewChainSet(store) err = s.Register(chainID, ctx.BlockHeight(), t.Seed.Height()) diff --git a/modules/ibc/middleware.go b/modules/ibc/middleware.go index 8697951499..16f45060f0 100644 --- a/modules/ibc/middleware.go +++ b/modules/ibc/middleware.go @@ -1,8 +1,6 @@ package ibc import ( - "errors" - "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" @@ -68,40 +66,48 @@ func (m Middleware) verifyPost(ctx basecoin.Context, store state.KVStore, // make sure the chain is registered from := tx.FromChainID if !NewChainSet(store).Exists([]byte(from)) { - err = ErrNotRegistered(from) - return + return ictx, itx, ErrNotRegistered(from) + } + + // TODO: how to deal with routing/relaying??? + packet := tx.Packet + if packet.DestChain != ctx.ChainID() { + return ictx, itx, ErrWrongDestChain(packet.DestChain) + } + + // verify packet.Permissions all come from the other chain + if !packet.Permissions.AllHaveChain(tx.FromChainID) { + return ictx, itx, ErrCannotSetPermission() } // make sure this sequence number is the next in the list q := InputQueue(store, from) - packet := tx.Packet - if q.Tail() != packet.Sequence { - err = errors.New("Incorrect sequence number - out of order") // TODO - return + tail := q.Tail() + if packet.Sequence < tail { + return ictx, itx, ErrPacketAlreadyExists() + } + if packet.Sequence > tail { + return ictx, itx, ErrPacketOutOfOrder(tail) } // look up the referenced header space := stack.PrefixedStore(from, store) provider := newDBProvider(space) - // TODO: GetExactHeight helper? - seed, err := provider.GetByHeight(int(tx.FromChainHeight)) + seed, err := provider.GetExactHeight(int(tx.FromChainHeight)) if err != nil { return ictx, itx, err } - if seed.Height() != int(tx.FromChainHeight) { - err = errors.New("no such height") // TODO - return - } // verify the merkle hash.... root := seed.Header.AppHash - key := []byte("?????") // TODO! - tx.Proof.Verify(key, packet.Bytes(), root) - - // TODO: verify packet.Permissions + pBytes := packet.Bytes() + valid := tx.Proof.Verify(tx.Key, pBytes, root) + if !valid { + return ictx, itx, ErrInvalidProof() + } // add to input queue - q.Push(packet.Bytes()) + q.Push(pBytes) // return the wrapped tx along with the extra permissions ictx = ictx.WithPermissions(packet.Permissions...) diff --git a/modules/ibc/provider.go b/modules/ibc/provider.go index 28cbd309a2..7f140c3c26 100644 --- a/modules/ibc/provider.go +++ b/modules/ibc/provider.go @@ -80,3 +80,16 @@ func (d *dbProvider) GetByHash(hash []byte) (seed certifiers.Seed, err error) { err = wire.ReadBinaryBytes(b, &seed) return } + +// GetExactHeight is like GetByHeight, but returns an error instead of +// closest match if there is no exact match +func (d *dbProvider) GetExactHeight(h int) (seed certifiers.Seed, err error) { + seed, err = d.GetByHeight(h) + if err != nil { + return + } + if seed.Height() != h { + err = ErrHeaderNotFound(h) + } + return +} diff --git a/modules/ibc/store.go b/modules/ibc/store.go index 810377d897..fc37b04146 100644 --- a/modules/ibc/store.go +++ b/modules/ibc/store.go @@ -80,10 +80,10 @@ func (c ChainSet) Register(chainID string, ourHeight uint64, theirHeight int) er // Packet is a wrapped transaction and permission that we want to // send off to another chain. type Packet struct { - DestChain string `json:"dest_chain"` - Sequence uint64 `json:"sequence"` - Permissions []basecoin.Actor `json:"permissions"` - Tx basecoin.Tx `json:"tx"` + DestChain string `json:"dest_chain"` + Sequence uint64 `json:"sequence"` + Permissions basecoin.Actors `json:"permissions"` + Tx basecoin.Tx `json:"tx"` } // Bytes returns a serialization of the Packet diff --git a/modules/ibc/tx.go b/modules/ibc/tx.go index 5f4bfe6ccd..9c4f60207a 100644 --- a/modules/ibc/tx.go +++ b/modules/ibc/tx.go @@ -110,6 +110,7 @@ type PostPacketTx struct { FromChainHeight uint64 // The block height in which Packet was committed, to check Proof // this proof must match the header and the packet.Bytes() Proof *merkle.IAVLProof + Key []byte Packet Packet } @@ -123,33 +124,3 @@ func (p PostPacketTx) ValidateBasic() error { func (p PostPacketTx) Wrap() basecoin.Tx { return basecoin.Tx{p} } - -// proof := tx.Proof -// if proof == nil { -// sm.res.Code = IBCCodeInvalidProof -// sm.res.Log = "Proof is nil" -// return -// } -// packetBytes := wire.BinaryBytes(packet) - -// // Make sure packet's proof matches given (packet, key, blockhash) -// ok := proof.Verify(packetKeyEgress, packetBytes, header.AppHash) -// if !ok { -// sm.res.Code = IBCCodeInvalidProof -// sm.res.Log = fmt.Sprintf("Proof is invalid. key: %s; packetByes %X; header %v; proof %v", packetKeyEgress, packetBytes, header, proof) -// return -// } - -// // Execute payload -// switch payload := packet.Payload.(type) { -// case DataPayload: -// // do nothing -// case CoinsPayload: -// // Add coins to destination account -// acc := types.GetAccount(sm.store, payload.Address) -// if acc == nil { -// acc = &types.Account{} -// } -// acc.Balance = acc.Balance.Plus(payload.Coins) -// types.SetAccount(sm.store, payload.Address, acc) -// }