search

cosmos / interchain-security

Interchain Security is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
156 stars 127 forks source link

Be more precise about what kinds of consumer initated slash requests should trigger an error and channel close #414

Closed danwt closed 1 year ago

danwt commented 2 years ago

Problem

There is some divergence between the spec and the code, and the code seems a bit suspicous w.r.t when it raises an error in HandleSlashPacket.

Closing criteria

Add clarity by documenting or changing the code and defining in the spec precise rules about what should raise an error.

Detail

Spec

https://github.com/cosmos/ibc/blob/c919d8ec8bec086be00b68d6f0e9ca4b0a0b8c5e/spec/app/ics-028-cross-chain-validation/methods.md?plain=1#L1667-L1704

Code

https://github.com/cosmos/interchain-security/blob/3f9100acd62a0350432a1a87c7caf06efadfd614/x/ccv/provider/keeper/relay.go#L206-L279

The code returns success,err=(bool,error) and it's not really clear when exactly an error should be returned. Note that a non-nil error will close the channel.

I think we could make the function return closeChannel,err=(bool,error) and then an error should be created whenever there is any problem preventing slashing, and closeChannel should be true only when the channel should be closed.

TODOs

shaspitz commented 2 years ago

https://github.com/cosmos/interchain-security/issues/417 is related, but 414 seems more focused on errors, whereas 417 is more focused on behavior. Lmk if you think they're duplicate and you'd like to close

danwt commented 2 years ago

Ah nice I wasn't aware of the other issue. I this one is a separate concern because as you say it is about errors (which impacts behavior because an error will close the channel), rather than the quantity of slashing done.

danwt commented 2 years ago

I think if !found || validator.IsUnbonded() should be broken into two top level ifs: if !found and if validator.IsUnbonded(). There should be at least a log warning if the validator is unbonded, or an error should be returned IMO. If the validator is unbonded either the consumer is broken (so you should remove it) or the provider is broken (so really the provider should halt)

shaspitz commented 2 years ago

I think if !found || validator.IsUnbonded() should be broken into two top level ifs: if !found and if validator.IsUnbonded(). There should be at least a log warning if the validator is unbonded, or an error should be returned IMO. If the validator is unbonded either the consumer is broken (so you should remove it) or the provider is broken (so really the provider should halt)

Is it possible than an unbonded validator would be a non-found validator? ie. a validator that's been removed from the validator set by the staking module, and no longer persisted in store. Maybe this is why the code was initially written in the way it is?

danwt commented 2 years ago

Is it possible than an unbonded validator would be a non-found validator?

I don't think so

shaspitz commented 2 years ago

I have a draft PR which will address the code clarity aspects of this issue. Functionality changes will be superseded by #546