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

Misleading slash handling code on provider #296

Closed danwt closed 2 years ago

danwt commented 2 years ago

The comment and error here say different things

https://github.com/cosmos/interchain-security/blob/f3a996699756bc223a168cb0b8886a355204c19e/x/ccv/provider/keeper/relay.go#L214-L216

jtremback commented 2 years ago

Here is the full code:

    // map VSC ID to infraction height for the given chain ID
    var infractionHeight uint64
    if data.ValsetUpdateId == 0 {
        infractionHeight = k.GetInitChainHeight(ctx, chainID)
    } else {
        infractionHeight = k.GetValsetUpdateBlockHeight(ctx, data.ValsetUpdateId)
    }

    // return if there isn't any initial chain height for the consumer chain
    if infractionHeight == 0 {
        return fmt.Errorf("cannot find infraction height matching the validator update id %d for chain %s", data.ValsetUpdateId, chainID)
    }

What this code is doing is getting the provider height for the given valset update id, or if the infraction happened before there was a valset update, getting the provider height when the consumer chain was initiated (this happens in OnChanOpenConfirm). Both GetInitChainHeight and GetValsetUpdateBlockHeight return 0 if they could not find anything (instead of using a found boolean). Hopefully a height of 0 is never possible (I don't believe it is).

So this error is returned in the event that GetInitChainHeight or GetValsetUpdateBlockHeight returned 0, which should not be possible, except for programmer error (I think), because if the channel is open, it means that OnChanOpenConfirm was called at some point.

I think this means that the error message is correct: cannot find infraction height matching the validator update id. The comment really only makes sense if you are thinking about one of the logical branches with GetInitChainHeight.

jtremback commented 2 years ago

TBH, using 0 as null here is hard to read and seems error prone