-
Notifications
You must be signed in to change notification settings - Fork 625
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 004: Fee module locking in the presence of severe bugs #1060
Merged
colin-axner
merged 6 commits into
ics29-fee-middleware
from
colin/adr-locking-mechanism
Mar 29, 2022
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5257c0b
add adr 004
colin-axner 8cce535
add to README
colin-axner 0c7c472
Merge branch 'ics29-fee-middleware' into colin/adr-locking-mechanism
ea975fa
Merge branch 'ics29-fee-middleware' into colin/adr-locking-mechanism
colin-axner 95c02e3
Update docs/architecture/adr-004-ics29-lock-fee-module.md
colin-axner 8619430
Update docs/architecture/adr-004-ics29-lock-fee-module.md
colin-axner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we just using a single account for all fees? This reads like each fee is getting its own separate account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. Applied your suggestion