From 5257c0ba70187ef7e43b5e79d69d00a85cde5e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 3 Mar 2022 16:29:26 +0100 Subject: [PATCH 1/4] add adr 004 --- .../adr-004-ics29-lock-fee-module.md | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 docs/architecture/adr-004-ics29-lock-fee-module.md diff --git a/docs/architecture/adr-004-ics29-lock-fee-module.md b/docs/architecture/adr-004-ics29-lock-fee-module.md new file mode 100644 index 00000000000..228fa2e2998 --- /dev/null +++ b/docs/architecture/adr-004-ics29-lock-fee-module.md @@ -0,0 +1,55 @@ +# ADR 004: Lock fee module upon escrow out of balance + +## Changelog +* 03/03/2022: initial draft + +## Status + +Accepted + +## Context + +The fee module maintains an escrow account for each of the fees escrowed to incentivize packet relays. +It also tracks each packet fee escrowed separately from the escrow account. This is because the escrow account only maintains a total balance. It has no reference for which coins belonged to which packet fee. +In the presence of a severe bug, it is possible the escrow balance will become out of sync with the packet fees marked as escrowed. +The ICS29 module should be capable of elegantly handling such a scenario. + +## Decision + +We will allow for the ICS29 module to become "locked" if the escrow balance is determined to be out of sync with the packet fees marked as escrowed. +A "locked" fee module will not allow for packet escrows to occur nor will it distribute fees. All IBC callbacks will skip performing fee logic, similar to fee disabled channels. + +Manual intervention will be needed to unlock the fee module. + +### Sending side + +Special behaviour will have to be accounted for in `OnAcknowledgementPacket`. Since the counterparty will continue to send incentivized acknowledgements for fee enabled channels, the acknowledgement will need to be unmarshalled into an incentivized acknowledgement before calling the underlying application `OnAcknowledgePacket` callback. + +When distributing fees, a cached context should be used. If the escrow account balance would become negative, the current state changes should be discarded and the fee module should be locked using the uncached context. This prevents fees from being partially distributed for a given packetID. + +### Receiving side + +`OnRecvPacket` should remain unaffected by the fee module becoming locked since escrow accounts only affect the sending side. + +## Consequences + +### Positive + +The fee module can be elegantly disabled in the presence of severe bugs. + +### Negative + +Extra logic is added to account for edge cases which are only possible in the presence of bugs. + +### Neutral + +## References + +Issues: +- [#821](https://github.com/cosmos/ibc-go/issues/821) +- [#860](https://github.com/cosmos/ibc-go/issues/860) + +PR's: +- [#1031](https://github.com/cosmos/ibc-go/pull/1031) +- [#1029](https://github.com/cosmos/ibc-go/pull/1029) +- [#1056](https://github.com/cosmos/ibc-go/pull/1056) From 8cce53526f3078e08ba0d3e3a6aeab2c2ae69728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 3 Mar 2022 16:32:05 +0100 Subject: [PATCH 2/4] add to README --- docs/architecture/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 091ba899dc6..edfbea4f0c8 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -30,6 +30,7 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov | [001](./adr-001-coin-source-tracing.md) | ICS-20 coin denomination format | Accepted, Implemented | | [002](./adr-002-go-module-versioning.md) | Go module versioning | Accepted | | [003](./adr-003-ics27-acknowledgement.md) | ICS27 acknowledgement format | Accepted | +| [004](./adr-004-ics29-lock-fee-module.md) | ICS29 module locking upon escrow out of balance | Accepted | | [015](./adr-015-ibc-packet-receiver.md) | IBC Packet Routing | Accepted | | [025](./adr-025-ibc-passive-channels.md) | IBC passive channels | Deprecated | | [026](./adr-026-ibc-client-recovery-mechanisms.md) | IBC client recovery mechansisms | Accepted | From 95c02e37230c068fc1f9add5850b76b919eea6e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 29 Mar 2022 14:06:02 +0200 Subject: [PATCH 3/4] Update docs/architecture/adr-004-ics29-lock-fee-module.md Co-authored-by: Aditya --- docs/architecture/adr-004-ics29-lock-fee-module.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-004-ics29-lock-fee-module.md b/docs/architecture/adr-004-ics29-lock-fee-module.md index 228fa2e2998..da81874d7ce 100644 --- a/docs/architecture/adr-004-ics29-lock-fee-module.md +++ b/docs/architecture/adr-004-ics29-lock-fee-module.md @@ -9,7 +9,7 @@ Accepted ## Context -The fee module maintains an escrow account for each of the fees escrowed to incentivize packet relays. +The fee module maintains an escrow account for all fees escrowed to incentivize packet relays. It also tracks each packet fee escrowed separately from the escrow account. This is because the escrow account only maintains a total balance. It has no reference for which coins belonged to which packet fee. In the presence of a severe bug, it is possible the escrow balance will become out of sync with the packet fees marked as escrowed. The ICS29 module should be capable of elegantly handling such a scenario. From 86194300632f2c2d1a975c60650e58d237f7ebc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 29 Mar 2022 14:06:08 +0200 Subject: [PATCH 4/4] Update docs/architecture/adr-004-ics29-lock-fee-module.md Co-authored-by: Aditya --- docs/architecture/adr-004-ics29-lock-fee-module.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-004-ics29-lock-fee-module.md b/docs/architecture/adr-004-ics29-lock-fee-module.md index da81874d7ce..5b17717e669 100644 --- a/docs/architecture/adr-004-ics29-lock-fee-module.md +++ b/docs/architecture/adr-004-ics29-lock-fee-module.md @@ -23,7 +23,7 @@ Manual intervention will be needed to unlock the fee module. ### Sending side -Special behaviour will have to be accounted for in `OnAcknowledgementPacket`. Since the counterparty will continue to send incentivized acknowledgements for fee enabled channels, the acknowledgement will need to be unmarshalled into an incentivized acknowledgement before calling the underlying application `OnAcknowledgePacket` callback. +Special behaviour will have to be accounted for in `OnAcknowledgementPacket`. Since the counterparty will continue to send incentivized acknowledgements for fee enabled channels, the acknowledgement will still need to be unmarshalled into an incentivized acknowledgement before calling the underlying application `OnAcknowledgePacket` callback. When distributing fees, a cached context should be used. If the escrow account balance would become negative, the current state changes should be discarded and the fee module should be locked using the uncached context. This prevents fees from being partially distributed for a given packetID.