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

Use trusted block next validators when building header #771

Merged
merged 4 commits into from
Mar 26, 2021

Conversation

ancazamfir
Copy link
Collaborator

Closes: #770

Description


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@ancazamfir ancazamfir requested a review from romac as a code owner March 26, 2021 10:27
@romac romac self-requested a review March 26, 2021 10:47
@ancazamfir ancazamfir marked this pull request as draft March 26, 2021 10:56
@codecov-io
Copy link

Codecov Report

Merging #771 (9d80ce4) into master (b1b37f5) will increase coverage by 30.9%.
The diff coverage is n/a.

❗ Current head 9d80ce4 differs from pull request most recent head 587f4dc. Consider uploading reports for the commit 587f4dc to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           master    #771      +/-   ##
=========================================
+ Coverage    13.6%   44.5%   +30.9%     
=========================================
  Files          69     166      +97     
  Lines        3752   11386    +7634     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    5078    +4565     
- Misses       2618    6308    +3690     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 22.2% <ø> (ø)
..._transfer/relay_application_logic/send_transfer.rs 85.7% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_consensus.rs 55.8% <ø> (ø)
modules/src/ics02_client/client_def.rs 32.3% <ø> (ø)
modules/src/ics02_client/client_state.rs 65.1% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 90.4% <ø> (ø)
... and 266 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c788d20...587f4dc. Read the comment docs.

@ancazamfir ancazamfir marked this pull request as ready for review March 26, 2021 12:34
@ancazamfir
Copy link
Collaborator Author

The first solution/ commit does not work as the next_validators() in the LightBlock doesn't have the proposer set and on-chain validation fails the update because of this. So I changed to pull the header at trusted_height + 1 and pass the trusted_height in build_header(). Also had a version that decrements the trusted_header.height() inside build_header(). I thought that look just a bit uglier. Can't think of a better solution until we refactor the light client.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

LGTM

@romac romac changed the title use trusted block next validators when building header se trusted block next validators when building header Mar 26, 2021
@romac romac changed the title se trusted block next validators when building header Use trusted block next validators when building header Mar 26, 2021
@ancazamfir ancazamfir merged commit d9922b5 into master Mar 26, 2021
@ancazamfir ancazamfir deleted the anca/770_next_vals branch March 26, 2021 14:56
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ms#771)

* Use h+1 for trusted as next_validators doesn't have proposer set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header trusted validators should be taken from trusted header's next validators
4 participants