-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Implement ADR 026 #7029
Implement ADR 026 #7029
Conversation
…e latest_timestamp to the message ClientState in proto/ibc/tendermint/tendermint.proto 2) Autogenerate x/ibc/07-tendermint/types/tendermint.pb.go by running 'make proto-gen'. Strangely, as a side effect x/distribution/types/genesis.pb.go, x/evidence/types/genesis.pb.go were also modified by the command 'make proto-gen' 3) Add Expired() function
Codecov Report
@@ Coverage Diff @@
## master #7029 +/- ##
==========================================
+ Coverage 54.66% 54.88% +0.21%
==========================================
Files 571 575 +4
Lines 39302 39435 +133
==========================================
+ Hits 21486 21644 +158
+ Misses 16039 16012 -27
- Partials 1777 1779 +2 |
…tStatus 2) Add allow_governance_override_after_misbehaviour flag to tendermint ClientStatus
This pull request introduces 1 alert when merging 0ec9447 into 4c762db - view on LGTM.com new alerts:
|
Note: Due to issue #6476, I could not run the command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, mostly solid, a few questions on the proposal handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dauTT! Left a few comments but implementation-wise looks great 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @dauTT !! There are still some implementation issues that I've marked.
I don't understand how you're expiration recovery mechanism would work without some way to update the clientTime before lite.Verify
is being called. WIll look into why your test case on this is passing
x/ibc/02-client/handler.go
Outdated
updateClientFlag = true | ||
} | ||
|
||
if tmClientState.AllowGovernanceOverrideAfterMisbehaviour && tmClientState.IsFrozen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect behaviour from my understanding.
Suppose AllowGovernanceOverrideAfterExpiry = false
and AllowGovernanceOverrideAfterMisbehaviour = true
And the client is frozen AND expired, then the correct behaviour is to reject the proposal since client is still expired even if it gets unfrozen.
Similarly, when AllowGovernanceOverrideAfterExpire = true
and AllowGovernanceOverrideAfterMisbehaviour = false
And the client is frozen AND expired, then the update proposal should fail since the client is still frozen.
cc: @cwgoes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will allow the proposal to pass in the above cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we change this above, we should end up with two separate paths (UpdateClient
will be called here, but not in the above path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So figured out why your expired test cases were passing. Update logic uses ctx.BlockTime()
for the current timestamp, which in your tests is always within trusting period of the last stored consensus state.
Once you fix the tests you should see them fail. As @cwgoes mentioned, you'll need some sort of flag in update logic to handle the case where we are overriding.
If override = true
, then you need to override the trusted time so that it passes the lite.Verify
checks.
Specifically you'll need to change this line:
https://github.com/cosmos/cosmos-sdk/blob/master/x/ibc/07-tendermint/types/update.go#L118
to something like:
Time: newHeader.Time
only when override = true
. If override = false
, keep as is.
EDIT: This solution will not work because all the time checks are taken care of in lite.Verify
if the updating header time is also expired. Can we assume that header.TIme is always going to be within Trusting period of ctx.BlockTime
? I assume not
x/ibc/proposal_handler_test.go
Outdated
clientkeeper := ibcKeeper.ClientKeeper | ||
|
||
// create test client | ||
_, err := clientkeeper.CreateClient(suite.ctx, clientID, tc.clientState, suite.consensusState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is possible for tc.clientState.Timestamp != suite.consensusState.Time
This is confusing, change both to be in sync for each test case as I have shown above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ready for review again. Still needs some more tests, but 95% done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a few comments but the logic itself is clean and simpler to understand
) | ||
} | ||
|
||
if cs.IsFrozen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, can we use switch for all the conditions here and move the contents to private functions? It's kinda hard to follow the logic, especially the last section were we should be checking for expired client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do a switch on a struct? Otherwise I'm not sure how to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried doing a switch on the client state but got:
cannot switch on cs (type ClientState) (struct containing []*ics23.ProofSpec cannot be compared)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch {
case cs.IsFrozen():
// logic
case !cs.AllowUpdateAfterExpiry, !cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()):
return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal")
case cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()):
// logic
default:
// error invalid case?
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, this will be much better. I modified the suggestion above a little in the latest commit, let me know if you think it needs any changes
return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal") | ||
} | ||
|
||
// the client is expired and either AllowUpdateAfterMisbehaviour or AllowUpdateAfterExpiry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the expiry should be checked with another condition? Using switch here would help readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is implicit based on the possible conditions. I can add an if condition, it would just be redundant. Do you have any proposed alternative layout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, just have one substantial request on tendermint proposal_handle_test.go
Will ACK once that request is addressed and the rest of tests are complete. Great work everyone!!
if !cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()) { | ||
return cs.CheckHeaderAndUpdateState(ctx, cdc, clientStore, header) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this case is hit, refreezing is still possible. Doesn't need to block the PR, but something to be noted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup was planning to update docs for this
|
||
consensusState := &ConsensusState{ | ||
Sequence: smHeader.Sequence, | ||
PublicKey: smHeader.NewPublicKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check that NewPublicKey != CurrentPublicKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be checked on regular updates as well? cc @cwgoes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a related note, should the sequence set in consensus state == header.GetSequence() or should it be header.GetSequence + 1 like in update.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can check that newPublicKey != currentPublicKey
, there's no reason to pass a proposal if you have access to the current public key.
This is a bit of a special case, since we're resetting the sequence, I think having it be equal is fine.
|
||
// to expire clients, time needs to be fast forwarded on both chainA and chainB. | ||
// this is to prevent headers from failing when attempting to update later. | ||
func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure of this test is confusing and difficult to understand imo. I suggest a clearer approach
malleate func() | ||
expPassUnfreeze bool // expected result using a header for unfreezing | ||
expPassUnexpire bool // expected result using a header for unexpiring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
malleate func() | |
expPassUnfreeze bool // expected result using a header for unfreezing | |
expPassUnexpire bool // expected result using a header for unexpiring | |
AllowUpdateAfterExpiry bool | |
AllowUpdateAfterMisbehaviour bool | |
expPassUnfreeze bool // expected result using a header for unfreezing | |
expPassUnexpire bool // expected result using a header for unexpiring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means you only need 4 test-case structs to test each possible configuration
// for each test case a header used for unexpiring clients and unfreezing | ||
// a client are each tested to ensure the work as expected and that | ||
// unexpiring headers cannot unfreeze clients. | ||
suite.Run(tc.name, func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here with each test case construct the clientState with the provided override flags.
- Test UpdateProposal against a valid clientstate (this is optional, you can remove this step)
- Freeze the client and provide the update header and see if you get the expected result
- Unfreeze and expire the client and provide the update header and see if you get the expected result
- Freeze and expire the client and provide the update header and see if you get the expected result
This would be much clearer imo, and require less test-case structs
There was a problem hiding this comment.
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 reduction. Each of the test cases in one of the four categories have varying results for the combo of expPassUnfreeze
and expPassUnexpire
? Trying to program this logic into the suite.Run
becomes messy and harder to reason about imo. I don't think I can cleanly reduce the test cases to 4 without dropping some testing logic (not testing unexpiry headers on frozen clients etc). I think it is important to test weaker headers on test cases expected to have stronger headers to ensure they don't pass.
I can abstract the clientState to be constructed on the override flags. I think any other improvement can be done in a followup (or you can push directly) because I don't follow.
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Aditya <adityasripal@gmail.com>
Co-authored-by: Aditya <adityasripal@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK modulo outstanding comments
This logic is definitely much cleaner, thanks @colin-axner 🎉.
|
||
consensusState := &ConsensusState{ | ||
Sequence: smHeader.Sequence, | ||
PublicKey: smHeader.NewPublicKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can check that newPublicKey != currentPublicKey
, there's no reason to pass a proposal if you have access to the current public key.
This is a bit of a special case, since we're resetting the sequence, I think having it be equal is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Final minor comments and pending switch
condition from the previous review
x/ibc/02-client/keeper/proposal.go
Outdated
return sdkerrors.Wrapf(types.ErrClientTypeNotFound, "cannot update client with ID %s", p.ClientId) | ||
} | ||
|
||
if clientType == exported.Localhost { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if clientType == exported.Localhost { | |
if clientType == exported.Localhost || p.ClientId == exported.ClientTypeLocalHost { |
x/ibc/02-client/types/proposal.go
Outdated
return nil, err | ||
} | ||
|
||
return &ClientUpdateProposal{title, description, clientID, any}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit break down and use named fields
x/ibc/02-client/types/proposal.go
Outdated
if err := header.ValidateBasic(); err != nil { | ||
return err | ||
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err := header.ValidateBasic(); err != nil { | |
return err | |
} | |
return nil | |
return header.ValidateBasic() |
} else { | ||
require.Error(t, err, tc.name) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var ( | ||
header exported.Header | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var ( | |
header exported.Header | |
) | |
var header exported.Header |
@@ -144,3 +144,25 @@ in the handshake callbacks. | |||
|
|||
Implementations which do not feel they would benefit from versioning can do | |||
basic string matching using a single compatible version. | |||
|
|||
## ClientUpdateProposal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, can you add ADR26 to the ibc/spec readme references?
The localhost client cannot be updated by a governance proposal. | ||
|
||
The solo machine client requires the boolean flag 'AllowUpdateAfterProposal' to be set | ||
to true in order to be updated by a proposal. This is set upon client creation and cannot | ||
be updated later. | ||
|
||
The tendermint client has two flags update flags, 'AllowUpdateAfterExpiry' and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, create h3 subsections (eg ### Localhost Client Proposal
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this should just be moved to each clients own spec
when we add them
if !cs.AllowUpdateAfterMisbehaviour { | ||
return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unfrozen") | ||
} | ||
|
||
// unfreeze the client | ||
cs.FrozenHeight = clienttypes.Height{} | ||
|
||
// if the client is frozen but not expired, do full validation of the header | ||
// NOTE: the client may be frozen again since the misbehaviour evidence may | ||
// not be expired yet | ||
if !cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()) { | ||
// if the client is expired we unexpire the client using softer validation, otherwise | ||
// full validation on the header is performed. | ||
if cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()) { | ||
return cs.unexpireClient(consensusState, tmHeader, ctx.BlockTime()) | ||
} else { | ||
// NOTE: the client may be frozen again since the misbehaviour evidence may | ||
// not be expired yet | ||
return cs.CheckHeaderAndUpdateState(ctx, cdc, clientStore, header) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return cs.unfreezeClient(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for now. unexpire
is needed because it is called twice
* Add allow_governance_override_after_expiry flag to tendermint NewCreateClientCmd * 1) Add LatestTimestamp to ClientState struct by adding a new attribute latest_timestamp to the message ClientState in proto/ibc/tendermint/tendermint.proto 2) Autogenerate x/ibc/07-tendermint/types/tendermint.pb.go by running 'make proto-gen'. Strangely, as a side effect x/distribution/types/genesis.pb.go, x/evidence/types/genesis.pb.go were also modified by the command 'make proto-gen' 3) Add Expired() function * Fix tests * 1) Add allow_governance_override_after_expiry flag to tendemint clientStatus 2) Add allow_governance_override_after_misbehaviour flag to tendermint ClientStatus * Cosmetic changes * Fix tests * Add Unfreeze function * Add new ClientUpdateProposal type * Add minor fixes * Add NewClientUpdateProposalHandler unit tests * Fix proto-lint-docker * Delete x/ibc/07-tendermint/tendermint_test.go * Follow convention to put signer last in msg function signature * 1) Add GetLatestTimestamp function to ClientStatus interface 2) Change Expired() signature to Expired(now time.Time) * 1) Add override flag to UpdateClient function 2) Fix tests * Refactor HandleClientUpdateProposal * 1) Extend exported Header interface with MarshalBinaryBare and UnmarshalBinaryBare methods 2) Move ClientUpdateProposal message to from ibc.proto to client.proto 3) Refactoring code 4) Add override flag to UpdateClient method 5) Fix tests * 1) Uncomment tests and clean up code 2) Add basic validation of the header (ValidateBasic) when the override flag is true * Cosmetic changes * Add TODO comments * Fix header MarshalBinaryBare, UnmarshalBinaryBare by using protobuf encoding/decoding * Fix proto comments * Fix override logic * undo gettimestamp for solo machine and localhost * add update after proposal func, some major refactoring in progress, various issues addressed * fix tendermint proposal update handling * run make proto-gen * remove timestamp from tendemint client * fix build issue for tm types * apply various review comments * add tests for 02-client functionality * self review fixes * typo * update tests slightly * update tendermint proposal handling tests * Update x/ibc/02-client/keeper/proposal.go Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * Update x/ibc/07-tendermint/types/proposal_handle.go Co-authored-by: Aditya <adityasripal@gmail.com> * Update x/ibc/07-tendermint/types/proposal_handle.go Co-authored-by: Aditya <adityasripal@gmail.com> * apply most of @fedekunze and some of @AdityaSripal suggestions * convert test to bools * update docs and increase code cov * fix build * fix typo, remove omitempty * add switch * apply @fedekunze latest suggestions * fix lint * Update x/ibc/02-client/keeper/proposal_test.go Co-authored-by: Christopher Goes <cwgoes@pluranimity.org> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Colin Axner <colinaxner@berkeley.edu> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
closes #6998
Implemented all the points of ADR 026:
TODO:
NewSubmitClientUpdateProposalCmd
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes