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

Modify IBC client governance unfreezing to reflect ADR changes #8405

Merged
merged 55 commits into from
Feb 16, 2021

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Jan 21, 2021

Description

closes: #8197
closes: #8299


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@colin-axner
Copy link
Contributor Author

I can document this in the specs, but some rationale for the current design

Cannot use substitute with different revision number: our consensus state key isn't fully lexicographical ordered since I believe we don't pack the revision numbers to 8 bytes. For now we can disallow this feature and add it later once range queries are easily supported. The likelyhood of an upgrade of a counterparty and a gov proposal occurring during the same time period seems very unlikely

Client implementations must copy consensus states/metadata: Clients will need the prefixed stores to copy metadata so pushing the burden on them to selectively choose what is needed is easiest and most flexible.

// number of the subject client is explicityly disallowed. We cannot support
// this until there is a way to range query for the all the consensus
// states which occurred between two IBC Revision heights.
// https://github.com/cosmos/cosmos-sdk/issues/7712
func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdateProposal) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of trying to enforce subject and substitute client types being equal here but just doing ClientType() doesn't add any more security from a malicious light client implementation, and if the light client implementation is malicious, it could just mess with other clients of its own type.

It is up to governance never to pass a vote where the subject belongs to a malicious light client implementation. Ideally, best practice (which I will document/recommend) would be to create an entirely new substitute client after the subject became frozen/expired. This way no other connection/channels are relying on the substitute.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

x/ibc/core/02-client/types/errors.go Outdated Show resolved Hide resolved
x/ibc/core/exported/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK modulo minor doc nits

docs/ibc/proposals.md Outdated Show resolved Hide resolved
docs/ibc/proposals.md Outdated Show resolved Hide resolved
evidence of such misbehaviour is likely to be submitted resulting in a frozen light client.

Frozen light clients cannot be updated under any circumstance except via a governance proposal.
Since validators can arbitarily agree to make state transitions that defy the written code, a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this sentence, what is the relation? Do you mean that the security model would be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was just trying to refer to the security model outlined in the ADR. Why this feature is safe and why it requires a governance proposal. Is there a way I could clarify this better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a quorum validators can sign arbitrary state roots which may not be valid executions of the state machine"

something like this is clearer I think

Comment on lines 23 to 27
Tendermint light clients may also become expired if the trusting period has passed since their
last update. If a chain undergoes an unplanned upgrade, there may be no commitment to that upgrade
signed by the validator set before the chain-id changes. In this situation, the validator set of
the last valid update for the light client is never expected to produce another valid header since
the chain-id has changed, which will ultimately lead the on-chain light client to become expired.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand the question

Comment on lines 23 to 27
Tendermint light clients may also become expired if the trusting period has passed since their
last update. If a chain undergoes an unplanned upgrade, there may be no commitment to that upgrade
signed by the validator set before the chain-id changes. In this situation, the validator set of
the last valid update for the light client is never expected to produce another valid header since
the chain-id has changed, which will ultimately lead the on-chain light client to become expired.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, do you mean that they could be expired just because no headers have been submitted even if there was no unplanned upgrade?

Comment on lines 23 to 27
Tendermint light clients may also become expired if the trusting period has passed since their
last update. If a chain undergoes an unplanned upgrade, there may be no commitment to that upgrade
signed by the validator set before the chain-id changes. In this situation, the validator set of
the last valid update for the light client is never expected to produce another valid header since
the chain-id has changed, which will ultimately lead the on-chain light client to become expired.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that should be clarified.

Co-authored-by: Christopher Goes <[email protected]>
Comment on lines 23 to 27
Tendermint light clients may also become expired if the trusting period has passed since their
last update. If a chain undergoes an unplanned upgrade, there may be no commitment to that upgrade
signed by the validator set before the chain-id changes. In this situation, the validator set of
the last valid update for the light client is never expected to produce another valid header since
the chain-id has changed, which will ultimately lead the on-chain light client to become expired.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case when a light client is expired is a different situation from when the chain undergoes an unplanned upgrades.

x/ibc/core/02-client/keeper/proposal_test.go Show resolved Hide resolved
x/ibc/light-clients/07-tendermint/types/proposal_handle.go Outdated Show resolved Hide resolved
x/ibc/light-clients/07-tendermint/types/proposal_handle.go Outdated Show resolved Hide resolved
)
}

// substitute clients are not allowed to be upgraded during the voting period
if substituteClientState.GetLatestHeight().GetRevisionNumber() != initialHeight.GetRevisionNumber() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: This logic may cause neighboring consensus states to have a time difference of more than the subject's trusting period.

I personally don't believe that is an issue.

@colin-axner
Copy link
Contributor Author

colin-axner commented Feb 12, 2021

@fedekunze I noticed the ibc cli command for this was missing from the gaiad tx gov submit-proposal outputs. How do I wire it up?

Edit: nevermind

@colin-axner
Copy link
Contributor Author

Tested simple case using 5m trusting period and 10m voting period. Client was unexpired and can process packets

…mos/cosmos-sdk into colin/8197-unfreeze-clients-gov-prop
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@colin-axner colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 16, 2021
@mergify mergify bot merged commit 47dd07d into master Feb 16, 2021
@mergify mergify bot deleted the colin/8197-unfreeze-clients-gov-prop branch February 16, 2021 15:31
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
…s#8405)

* update proto files

* make proto-gen

* update clienttypes

* update localhost and solo machine

* refactor tm client proposal handling

* copy metadata

* self review fixes

* update 02-client keeper tests

* fix 02-client type tests

* fix localhost and solomachine tests

* begin updating tm tests

* partially fix tm tests

* increase codecov

* add more tests

* add changelog

* update specs

* add docs

* fix test

* modify adr

* allow modified chain-ids

* add CLI command

* fix typos

* fix lint

* Apply suggestions from code review

Co-authored-by: Federico Kunze <[email protected]>

* update docs, rm example

* Update docs/ibc/proposals.md

Co-authored-by: Christopher Goes <[email protected]>

* update height checks to reflect chain-id changes cc @AdityaSripal

* Apply suggestions from code review

Co-authored-by: Christopher Goes <[email protected]>

* Apply suggestions from code review

Co-authored-by: Aditya <[email protected]>

* address most of @AdityaSripal suggestions

* update docs per review suggestions

* Update x/ibc/core/02-client/types/proposal.go

* add proposal handler

* register proposal type

* register proposal on codec

* fix routing

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Christopher Goes <[email protected]>
Co-authored-by: Aditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
4 participants