doc: add fault resolution ADR #2051
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughWalkthroughThe changes introduce an Architecture Decision Record (ADR) aimed at managing fault resolutions within a blockchain framework. It establishes a governance-based slashing mechanism to penalize validators for misbehavior on Opt-in consumer chains. The ADR introduces a new proposal type, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConsumerChain
participant ProviderChain
participant Governance
User->>ConsumerChain: Detect fault by validator(s)
User->>ProviderChain: Submit consumer-fault-resolution proposal
ProviderChain->>Governance: Forward proposal for voting
Governance->>Governance: Vote on fault resolution proposal
alt Proposal Approved
Governance->>ProviderChain: Approve fault resolution
ProviderChain->>ConsumerChain: Slash misbehaving validator(s)
else Proposal Rejected
Governance->>ProviderChain: Reject fault resolution
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
C:Docs
| However, this is a complex technology, and there is no framework that works for Cosmos chains to this day. | ||
|
|
||
|
|
||
| To address this risk in PSS, a governance-gated slashing solution can be used until fraud proof technology matures. |
There was a problem hiding this comment.
Note that the fraud proofs will only work for incorrect execution.
| This ADR proposes a fault resolution mechanism, which is a type of governance proposal that victims of faults can use to vote on the | ||
| slashing of validators that misbehave on Opt-in consumer chains. | ||
|
|
||
| In what follows, we describe the implementation of a fault resolution mechanism that handles incorrect executions on consumer chains, |
There was a problem hiding this comment.
Why only incorrect executions?
There was a problem hiding this comment.
Because we only provide a guideline for incorrect execution fault definition in the intersubjective faults CHIPs. The handling for other types of faults will be implemented iteratively.
There was a problem hiding this comment.
but that will not need any new implementation effort, right?
There was a problem hiding this comment.
I would recommend making this a bit more general. From a code perspective, this handles arbitrary fault resolutions
| The submission of a fault resolution fails if any of the following conditions are not met: | ||
|
|
||
| - the consumer chain is an Opt-in chain | ||
| - all listed validators were opted-in to the consumer chain in the past unbonding-period |
There was a problem hiding this comment.
| - all listed validators were opted-in to the consumer chain in the past unbonding-period | |
| - the provided validator set matches one of the consumer validator sets from the previous unbonding period | |
| - all the listed validators are part of the provided validator set |
There was a problem hiding this comment.
The provided validator set shouldn't necessarily match one of the consumer validator sets completely, right? i.e. it can be a subset.
There was a problem hiding this comment.
@sainoe yes this is right. Perhaps only a few validators committed the fault.
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Marius Poke <[email protected]>
There was a problem hiding this comment.
I think we should be careful not to do too much technical work for new types of fault resolutions. In general, I think we should assume that all evidence is in the description, and not add extra fields for this.
Since incorrect execution is a bit special, I do agree with Marius that we can have an extra field for e.g. the invalid state root or the infraction height.
However, I don't think we should do something like this for each type; just let people use the description field to outline the evidence in whatever way they deem suitable.
There was a problem hiding this comment.
LGTM, I just added one comment about the sentence that says that the ADR only has the implementation for incorrect execution... I don't understand what extra technical work is required for other types. I think it should be no extra implementation work, and the same type of proposal should be used for all resolutions, and because of this I'd suggest rewording the sentence a bit. But maybe I'm overlooking something
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (3)
docs/docs/adrs/adr-018-fault-resolutions.md (3)
16-20: Review the context section for clarity and completeness.The context provided about Partial Set Security and the subset problem is well-explained. However, consider adding a comma after "Opt-in chains" for better readability.
- This threat is particularly relevant for Opt-in chains since they might be secured by a relatively small subset of the provider's validator set. + This threat is particularly relevant for Opt-in chains, since they might be secured by a relatively small subset of the provider's validator set.
24-26: Simplify language to enhance clarity.Consider using simpler alternatives to avoid wordiness, such as replacing "in order to" with "to".
- * Incorrect executions to break protocol rules in order to steal funds. + * Incorrect executions to break protocol rules to steal funds. - * Liveness attacks to halt the chain or censor transactions. + * Liveness attacks to halt the chain or censor transactions. - * Oracle attacks to falsify information used by the chain logic. + * Oracle attacks to falsify information used by the chain logic.Tools
LanguageTool
[style] ~24-~24: Consider a shorter alternative to avoid wordiness.
Context: ...rect executions to break protocol rules in order to steal funds. * Liveness attacks to halt...(IN_ORDER_TO_PREMIUM)
59-59: Simplify language to enhance clarity.Consider using simpler alternatives to avoid wordiness, such as replacing "in order to" with "to".
- In addition, in order to prevent spamming, users must pay a default fee of `100ATOM` to submit a fault resolution to the provider. + Additionally, to prevent spamming, users must pay a default fee of `100ATOM` to submit a fault resolution to the provider.Tools
LanguageTool
[style] ~59-~59: Consider a shorter alternative to avoid wordiness.
Context: ...validators to be slashed. In addition, in order to prevent spamming, users must pay a defa...(IN_ORDER_TO_PREMIUM)
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (2)
docs/docs/adrs/adr-018-fault-resolutions.md (2)
7-8: Ensure proper spacing around lists and headings.Headings and lists should be surrounded by blank lines for proper markdown formatting.
- * 17th July 2024: Initial draft + + * 17th July 2024: Initial draft +Tools
Markdownlint
7-7: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
8-8: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
20-20: Add a comma for better readability.Consider adding a comma after "Opt-in chains" for better readability.
- This threat is particularly relevant for Opt-in chains since they might be secured by a relatively small subset of the provider's validator set. + This threat is particularly relevant for Opt-in chains, since they might be secured by a relatively small subset of the provider's validator set.
|
@coderabbitai pause |
|
|
||
| The proposal has the following fields: | ||
|
|
||
| - **Consumer Chain**: The consumer chain ID that the fault was related to. |
There was a problem hiding this comment.
Since this is not merged yet, it could be updated to reflect Permissionless and use "consumer id" here.
| Each fault type is mapped to a fault definition that precisely describes an intersubjective fault, such as an incorrect execution, and explains why it qualifies as a slashable fault. Refer to the [fault definitions section](https://forum.cosmos.network/t/preventing-intersubjective-faults-in-ics/14103#fault-definitions-4) in "Preventing Intersubjective faults in ICS" for more details. Note that the text of each fault definition is stored as a string constant in the provider code. | ||
|
|
||
|
|
||
| In addition, to prevent spamming, users must pay a default fee of `100ATOM` to submit a fault resolution to the provider. |
There was a problem hiding this comment.
This is the normal proposal fee, right?
| ```protobuf | ||
| messsage { | ||
| // timestamp recording the last time a validator opted in to the consumer chain | ||
| google.protobuf.Timestamp join_time = 1; |
There was a problem hiding this comment.
Maybe add a sentence on why join_time is needed. The way I see it, we could simply get by using the leave_time. If the validator is currently opted in, then the leave_time is irrelevant. If the validator is not currently opted in, then we can see if they left before unbonding period or not.
| sidebar_position: 20 | ||
| title: Fault Resolutions | ||
| --- | ||
| # ADR 019: Fault Resolutions |
There was a problem hiding this comment.
Should fault resolutions be optional (?) and a consumer chain can choose whether to use them or not when issuing a MsgCreateConsumer or a MsgUpdateConsumer. What's the relation with docs: ADR for Customizable Slashing and Jailing?
Description
Add ADR proposing a Fault Resolutions solution to address the risk of validators collusion in Opt-In chains.
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
docs:prefix in the PR titleReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
docs:prefix in the PR titlemake build-docs)Summary by CodeRabbit
consumer-fault-resolution, for slashing validators who misbehave on consumer chains.