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

Circuit Breaker #926

Closed
gamarin2 opened this issue Apr 27, 2018 · 21 comments
Closed

Circuit Breaker #926

gamarin2 opened this issue Apr 27, 2018 · 21 comments

Comments

@gamarin2
Copy link
Contributor

gamarin2 commented Apr 27, 2018

As per discussions in validator meeting. Introduce a circuit breaker to temporarily de-activate certain message types.

When:

  • Network is live
  • Governance module is not compromised

Circuit breaker needs 1/6th of total voting power to be activated. Will be de-activated after X blocks or if de-activation is voted (can be de-activated with a proposal).

Message type of circuit breaker transactions should be gov (because we only assume that governance module is not compromised).

When submitting a circuit breaker transaction, one must specify:

  • Motive
  • Which message types to deactivate (gov messages cannot be deactivated).

Need to add the ability for proposals to de-activate circuit breakers if accepted. Also need a special de-activation message.

Pulling in @sunnya97 to refine.

@cwgoes
Copy link
Contributor

cwgoes commented Apr 27, 2018

Questions:

  • Should a circuit breaker, when enabled, disable all governance transactions except passing PlaintextProposals (perhaps to indicate a desired software change) or deactivating the breaker? Is there any reason we would want to retain the ability to pass other proposals (e.g. ParameterChangeProposals)?
  • Can the motive be an opaque commitment (e.g. the hash of a file)? That way sensitive information, such as a vulnerability disclosure, can be communicated securely out-of-band.
  • For the above reason, I also wonder if we want to only have the option to disable all message types, instead of picking them selectively - the "picking" reveals information.

@gamarin2
Copy link
Contributor Author

gamarin2 commented Apr 29, 2018

Should a circuit breaker, when enabled, disable all governance transactions except passing PlaintextProposals (perhaps to indicate a desired software change) or deactivating the breaker? Is there any reason we would want to retain the ability to pass other proposals (e.g. ParameterChangeProposals)?

Yes, I think it makes sense. I'd add SoftwareUpgradeProposal too. But this is for later, as we'll only have PlainTextProposal and SoftwareUpgradeProposal in the beginning.

Can the motive be an opaque commitment (e.g. the hash of a file)? That way sensitive information, such as a vulnerability disclosure, can be communicated securely out-of-band.

Yes. I think we should just let this field as open as possible and let standards emerge.

For the above reason, I also wonder if we want to only have the option to disable all message types, instead of picking them selectively - the "picking" reveals information.

No, I don't think we should. This is a tradeoff, and disabling all message types is pretty crippling for the network. There are two scenarios to consider:

  • Either the vulnerability is already disclosed and exploited by a malicious hacker, in which case it does not make sense to hide which message type is concerned
  • The vulnerability has been found by a white hat and not exploited. In this case, picking the message type might reveal some info, but it is still pretty vague. The circuit breaker proposal could also add other message types to hide the one that is concerned. I don't think it's worth shutting the whole blockchain for the sake of not giving any clue to potential attackers. Also, if the vulnerability is that bad, the circuit breaker proposal can still propose to shut down all message types, effectively doing what you're suggesting.

@cwgoes
Copy link
Contributor

cwgoes commented May 8, 2018

Per discussion with @ebuchman, we might want an additional requirement of a known identity (possibly AiB) signing off on the circuit breaker. This identity could be elected / changed through governance.

Can be implemented when transfers go live if we launch without transfers.

@ebuchman
Copy link
Member

ebuchman commented May 10, 2018

Latest thinking is to have two conditions for circuit breaking:

  • 1/3 of the voting power votes for it
  • 1/6 of the voting power votes for it and some set of AUTHORITIES signs off on it

The AUTHORITIES are an M-of-N muiltisig decided on by Governance.

We should add the details of this to the spec.

Relevant question is whether to have a new tx type for this or to re-use the governance voting system just with a new ProposalType and rules for what it means to pass

The AUTHORITIES can be chosen by governance in the first place. It must happen before transfers become active and until then we have M=0

@gamarin2
Copy link
Contributor Author

Thinking about it maybe the circuit breaker has to be implemented at SDK level? Because the app should reject tx types specified by Circuit Breaker proposal

@jackzampolin
Copy link
Member

This is currently still planned #prelaunch, do we have some spec on this? I know there have multiple conversations about this feature.

@cwgoes
Copy link
Contributor

cwgoes commented Oct 12, 2018

This is currently still planned #prelaunch, do we have some spec on this? I know there have multiple conversations about this feature.

Is it? I guess the label is still there, but I thought we had decided otherwise, e.g. https://github.com/cosmos/cosmos-sdk/blob/develop/docs/PRIORITIES.md#lower-priority.

If we want to do this prelaunch we need to discuss a much more exact specification.

@jackzampolin
Copy link
Member

I think we need to get this discussion started at the very least cc @ebuchman @jaekwon @gamarin2 @sunnya97 @rigelrozanski

@rigelrozanski
Copy link
Contributor

Yeah I think we can get away with not implementing this - like to know @jaekwon's thoughts here

@sunnya97
Copy link
Member

sunnya97 commented Oct 13, 2018

The big con of not implementing this prelaunch, is that then activating transfers would require a software upgrade.

@gamarin2
Copy link
Contributor Author

I think this can be done post-launch too. Originally circuit breaker was needed to avoid implementing urgent proposals. Circuit breaker is useful to maintain liveness if bugs are found in specific modules.

Thing is, we are only going to launch with modules we cannot afford to de-activate like staking or gov (we assume gov is not compromised for circuit breaker to work). If we were launching with IBC or transfers I would argue that circuit breaker is needed, but since we're not...

I think circuit breaker should come in the same upgrade that activates transfers.

@jackzampolin
Copy link
Member

Going to go ahead and close this issue as we have implemented.

@rigelrozanski
Copy link
Contributor

actually the circuit breaker is not implemented.

@jackzampolin
Copy link
Member

Should we rename this issue to CosmosCERT?

@jackzampolin jackzampolin added this to the v0.38.0 milestone Sep 5, 2019
@alexanderbez
Copy link
Contributor

Why's that @jackzampolin? Afaik, (d)CERT and the like are orthogonal to the circuit breaker.

@rigelrozanski
Copy link
Contributor

We decided to roll out dCERT in to phases. Less contentious proposal for dCERT does not include circuit breaker, afterwords the community may want to vote that would like the dCERT group to have circuit breaker capabilities

@alexanderbez
Copy link
Contributor

Do we plan on having this as an isolated module still (e.g. x/circuit)? It seems to make sense that we should.

@rigelrozanski
Copy link
Contributor

Hmm... maybe for potential client functionality. A lot of the core logic of the circuit breaker actually lives in baseapp however. But yeah I agree as much as possible should be separated out into a circuit breaker module

@okwme
Copy link
Contributor

okwme commented Aug 17, 2021

Should this be considered as a feature of the crisis module?

@tac0turtle
Copy link
Member

This is another amazing use case for ABCI 2.0 and vote extensions. If something needs to be halted validators can come to consensus on it through vote extensions instead of submitting a tx. The tx approach is a Valid use case and may want to be implemented until 0.38 is out

@tac0turtle
Copy link
Member

closing in favour of #14225 to discuss design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants