-
Notifications
You must be signed in to change notification settings - Fork 627
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
Efficient Consensus State Iteration #125
Conversation
} | ||
return true | ||
} | ||
IterateConsensusStateAscending(clientStore, pruneCb) |
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 will fetch earliest consensus state, we can implement pruning now without breaking changes.
Can similarly test for monotonic time as well
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 agree with the solution 👍 Clever fix!!
I do think this isn't a sustainable fix and is definitely a hack to support backwards compatibility. I think we should open a followup issue (or better yet, an ADR!) which explains in detail the sustainable fix so if IBC ever becomes non-backwards compatible we can be ready to swap out this solution for a more robust one that supports all light clients
I left various comments on areas for improvement in the code. I'd mostly like more code documentation explaining that this is a backwards compatible hack. I can see it being very confusing trying to understand why the code is constructed how it is and I think it is good to keep this code isolated so we can easily replace it when we are ready. The more it becomes entangled with existing logic, the harder it will be to replace. So we should have in code comments reminding us to keep it isolated
This PR implements (minimal) consensus state pruning. Future PRs will address monotonic time as well as more complete pruning solution |
Codecov Report
@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 65.22% 65.26% +0.04%
==========================================
Files 127 127
Lines 8413 8496 +83
==========================================
+ Hits 5487 5545 +58
- Misses 2561 2574 +13
- Partials 365 377 +12
|
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.
LGTM, left some suggestions and nits
csKey := iterator.Value() | ||
bz := clientStore.Get(csKey) | ||
if bz == nil { | ||
return nil, false | ||
} | ||
|
||
consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz) | ||
if err != nil { | ||
return nil, false | ||
} | ||
|
||
consensusState, ok := consensusStateI.(*ConsensusState) | ||
if !ok { | ||
return nil, false | ||
} |
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.
these errors are also missing codecov, but I believe de duplicating logic from the existing GetConsensusState
function could allow reusal of that test to ensure codecov
consState, err := GetConsensusState(clientStore, cdc, height) | ||
// this error should never occur | ||
if err != nil { | ||
pruneError = err |
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 maybe we should wrap this error with some message that says "please open an issue". I wouldn't want a relayer to get this issue trying to debug it since really there'd need to be something really wrong
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.
Hmm should I just log and ignore the issue. If this is caught in wild, it would suck that no updates can happen because clientstore can't prune properly. It should still just revert to old updating logic, while we implement a fix since this is a nice-to-have and not critical for IBC functioning
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'm ok with this. How do you plan to log the error?
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 could have used logger in context.
I decided against this because logging instead of returning error would make it easy for this to be missed in unit tests. I'd rather just ensure our test coverage makes us confident about this, rather than implementing with mistakes and not catching them until deployment
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.
Sounds like to correct decision to me. There is something severely wrong if this error occurs
… into aditya/efficient-consensus
This seems safe, but the extra layer of indirection is a bit wonky, I think we should try to implement a proper (backwards-incompatible) fix sometime in the future. Maybe it's worth keeping a list of "pending fixes" like this so we don't accumulate too technical debt and then lose track of what was necessary vs. what was merely done to avoid breaking backwards compatibility. |
I very much agree. I've added a |
This pull request introduces 1 alert when merging 30758d0 into db6f316 - view on LGTM.com new alerts:
|
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!! ACK pending CI fix and changelog entry
* fix: remove agoric-solo; ag-nchainz is in agoric-sdk More fixes for the general nchainz script and agoric.json. * refactor: remove metaprogramming from nchainz; use a config file * feat: factor nchainz run command into chains and clients
…#125) Bumps [github.com/multiformats/go-multiaddr](https://github.com/multiformats/go-multiaddr) from 0.4.0 to 0.4.1. - [Release notes](https://github.com/multiformats/go-multiaddr/releases) - [Commits](multiformats/go-multiaddr@v0.4.0...v0.4.1) --- updated-dependencies: - dependency-name: github.com/multiformats/go-multiaddr dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description
This approach implements a backward compatible fix for efficient consensus state iteration without changing the host path, which is what the counterparty chain expects as the consensus state key.
Enforce monotonic time(will address in subsequent PR)closes: #XXXX
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