Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADR: 015] IBC Packet Receiver #5230

Merged
merged 37 commits into from
Dec 11, 2019
Merged
Changes from 2 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7ace5b2
WIP: ADR IBC Packet Receiver
mossid Oct 22, 2019
9b1b06e
Update adr-015-ibc-packet-receiver.md
mossid Oct 22, 2019
d275aae
Remove FoldHandler, add consequences
mossid Nov 7, 2019
6bacbcf
Apply suggestions from code review @fedekunze @cwgoes
mossid Nov 7, 2019
465d1d8
Apply suggestions from code review
mossid Nov 8, 2019
291452c
Add wrapper function for acknowledgement
mossid Nov 8, 2019
4ae5461
fix typo
mossid Nov 8, 2019
5735e2b
Merge branch 'master' into joon/adr-ibc-packet-receiver
cwgoes Nov 8, 2019
35ec132
Apply suggestions from code review
mossid Nov 8, 2019
43fe290
Add other handler types requires verification
mossid Nov 13, 2019
330b043
Add PacketHandler type
mossid Nov 13, 2019
7ab51de
Use AnteDecorator, rename the helper to WriteAck
mossid Nov 14, 2019
fe499a6
fix typo
mossid Nov 14, 2019
c6f9154
Merge branch 'master' into joon/adr-ibc-packet-receiver
cwgoes Nov 14, 2019
e130c07
minor cleanup
fedekunze Nov 14, 2019
ab1dcd2
Update adr-015-ibc-packet-receiver.md
mossid Nov 15, 2019
66547af
Add vulnerability scenario(failing message injection)
mossid Nov 15, 2019
9cd9bc3
Apply suggestions from code review
fedekunze Nov 19, 2019
b168e3f
Add PacketDataI to this ADR
jackzampolin Nov 22, 2019
c3a812d
add port/chan checking, add ics20 example
mossid Nov 22, 2019
7f1b0d6
add foldhandler, remove safety assumptions
mossid Dec 3, 2019
961ea03
Apply suggestions from code review
mossid Dec 3, 2019
c60d4c6
Merge branch 'master' into joon/adr-ibc-packet-receiver
fedekunze Dec 4, 2019
4106830
address comments in progress
mossid Dec 4, 2019
4569be4
address missing pieces
mossid Dec 7, 2019
07a8987
fix missed line
mossid Dec 7, 2019
26db0bc
Apply suggestions from code review
cwgoes Dec 8, 2019
ed131a3
Merge branch 'master' into joon/adr-ibc-packet-receiver
cwgoes Dec 8, 2019
e6692f7
Apply suggestions from code review
mossid Dec 9, 2019
46e018c
address comments
mossid Dec 9, 2019
4ce753c
Merge branch 'master' into joon/adr-ibc-packet-receiver
fedekunze Dec 9, 2019
6e4c052
address comments in progress
mossid Dec 10, 2019
0d236ff
Merge branch 'master' into joon/adr-ibc-packet-receiver
alexanderbez Dec 11, 2019
7ebb6ad
Add newline
alexanderbez Dec 11, 2019
d62de0c
Fix linting and typos
alexanderbez Dec 11, 2019
a22aeee
Merge branch 'master' into joon/adr-ibc-packet-receiver
cwgoes Dec 11, 2019
37342cf
Merge branch 'master' into joon/adr-ibc-packet-receiver
cwgoes Dec 11, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions docs/architecture/adr-015-ibc-packet-receiver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# ADR 015: IBC Packet Receiver

## Changelog

- 2019 Oct 22: Initial Draft

## Context

[ICS 26](https://github.com/cosmos/ics/tree/master/spec/ics-026-routing-module) defines function `handlePacketRecv`.
mossid marked this conversation as resolved.
Show resolved Hide resolved
`handlePacketRecv` executes per-module `onRecvPacket`, verifies the packet merkle proof, and pushes the acknowledgement bytes
mossid marked this conversation as resolved.
Show resolved Hide resolved
to the state.
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
mossid marked this conversation as resolved.
Show resolved Hide resolved

The mechanism is simillar with the transaction handling logic in `baseapp`. After authentication, the handler is executed, and
mossid marked this conversation as resolved.
Show resolved Hide resolved
the authentication state change must be committed regardless of the result of the handler execution.

ICS 26 also requires acknowledgement writing which has to be done after the handler execution and also must be commited
cwgoes marked this conversation as resolved.
Show resolved Hide resolved
regardless of the result of the handler execution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context doesn't really define any problem...at least I can't see one immediately. I would state the problem clearly (purpose of the ADR) and how it relates to the ante-handler.

## Decision

We will define an `AnteHandler` for IBC packet receiving. The `AnteHandler` will iterate over the messages included in the
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
mossid marked this conversation as resolved.
Show resolved Hide resolved
transaction, type asserts to check whether the message is type of IBC packet receiving, and verifies merkle proof.
mossid marked this conversation as resolved.
Show resolved Hide resolved

```go
// Pseudocode
mossid marked this conversation as resolved.
Show resolved Hide resolved
func AnteHandler(ctx sdk.Context, tx sdk.Tx, bool) (sdk.Context, sdk.Result, bool) {
mossid marked this conversation as resolved.
Show resolved Hide resolved
for _, msg := range tx.GetMsgs() {
if msg, ok := msg.(MsgPacket); ok {
if err := VerifyPacket(msg.Packet, msg.Proofs, msg.ProofHeight); err != nil {
return ctx, err.Result(), true
}
}
}
return ctx, sdk.Result{}, false
}
```

The `AnteHandler` will be inserted to the top level application, next to the signature authentication logic provided by `auth.NewAnteHandler`.
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
jackzampolin marked this conversation as resolved.
Show resolved Hide resolved

We will define a new type of handler, named `FoldHandler`. `FoldHandler` will be called after all of the messages are executed.
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

```go
type FoldHandler func(ctx sdk.Context, tx sdk.Tx, result sdk.Result) (sdk.Result, bool)

// Pseudocode
func (app *BaseApp) runTx(tx sdk.Tx) sdk.Result {
anteCtx, msCache := app.cacheTxContext(ctx, tx)
newCtx, result, abort := app.AnteHandler(ctx, tx)
if abort {
return result
}
msCache.Write()

runMsgCtx, msCache := app.cacheTxContext(ctx, tx)
result = app.runMsgs(runMsgCtx, msgs)
if result.IsOk() {
msCache.Write()
}

// BEGIN modification made in this proposal

foldCtx, msCache := app.cacheTxContext(ctx, tx)
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
result, abort = app.FoldHandler(ctx, tx, result)
if abort {
return result
}
msCache.Write()
return result

// END modification made in this proposal
}
```

The IBC module will expose `FoldHandler` as defined below:

```go
// Pseudocode
func FoldHandler(ctx sdk.Context, tx sdk.Tx, result sdk.Result) (sdk.Result, bool) {
// TODO: consider multimsg
}
```

## Status

Proposed

## Consequences

> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future.

### Positive

{positive consequences}
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

### Negative

{negative consequences}
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

### Neutral

{neutral consequences}

## References

- Relevant comment: https://github.com/cosmos/ics/issues/289#issuecomment-544533583