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.

Sign up for GitHub

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

Jump to bottom

runtime: Support consensus event queries #4904

Merged
merged 8 commits into from
Sep 9, 2022
Merged

runtime: Support consensus event queries #4904

merged 8 commits into from
Sep 9, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Aug 25, 2022

Based on #4893

@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from 8488bcc to 4446a50 Compare August 25, 2022 11:32
Base automatically changed from kostko/feature/verifier-state-at to master August 26, 2022 13:13
@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from 4446a50 to b16e00d Compare August 26, 2022 16:05
@kostko kostko marked this pull request as ready for review August 26, 2022 16:07
peternose
peternose reviewed Aug 29, 2022
View reviewed changes
runtime/src/consensus/tendermint/verifier.rs
@@ -282,6 +304,7 @@ impl Verifier {
let state_root = state_root_from_header(&verified_block.signed_header);
Ok(ConsensusState::from_protocol(
self.protocol.clone(),
state_root.version + 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this to from_protocol?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because it is specific to the Tendermint consensus backend so IMO it should be determined here. I agree that it is somewhat repetitive here and we could maybe move this into a function.

I've tried to explain this in the ConsensusState implementation:

pub struct ConsensusState {
    // An explicit height field is needed because the relationship between the underlying consensus
    // height and the corresponding state root is a consensus backend implementation detail.
    height: u64,

@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from b16e00d to 4b1ac56 Compare August 29, 2022 12:03
@codecov
Copy link

codecov bot commented Aug 29, 2022 •
edited
Loading

Codecov Report

Merging #4904 (fc568b0) into master (0fb3791) will decrease coverage by 0.05%.
The diff coverage is 53.89%.

❗ Current head fc568b0 differs from pull request most recent head 9cde8d9. Consider uploading reports for the commit 9cde8d9 to get more accurate results

@@            Coverage Diff             @@
##           master    #4904      +/-   ##
==========================================
- Coverage   66.81%   66.76%   -0.06%     
==========================================
  Files         464      464              
  Lines       51035    51127      +92     
==========================================
+ Hits        34099    34133      +34     
- Misses      12760    12815      +55     
- Partials     4176     4179       +3
Impacted Files Coverage Δ
go/common/version/version.go 80.26% <ø> (ø)
go/consensus/api/grpc.go 67.54% <0.00%> (-3.18%) ⬇️
go/consensus/tendermint/light/client.go 46.60% <0.00%> (-0.93%) ⬇️
go/consensus/tendermint/seed/seed.go 65.34% <0.00%> (-0.66%) ⬇️
go/runtime/host/protocol/types.go 54.54% <ø> (ø)
go/runtime/registry/host.go 68.75% <57.89%> (-4.35%) ⬇️
go/consensus/tendermint/full/light.go 68.57% <88.88%> (+9.31%) ⬆️
go/worker/client/committee/node.go 75.48% <100.00%> (-1.30%) ⬇️
go/storage/api/root_cache.go 60.00% <0.00%> (-8.00%) ⬇️
go/worker/common/p2p/dispatch.go 71.52% <0.00%> (-5.56%) ⬇️
... and 37 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch 5 times, most recently from 7e9186a to 8cd3594 Compare September 5, 2022 08:45
@kostko kostko mentioned this pull request Sep 5, 2022
Merged
Yawning
Yawning reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

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

Doesn't this need to bump the runtime host protocol?

go/staking/api/api_test.go
var txHash hash.Hash
txHash.Empty()

// NOTE: These cases should be synced with tests in runtime/src/consensus/staking.rs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Long term, instead of hardcoding crap like this, we should start sticking testvectors into JSON files that are used by both test cases.

go/runtime/host/logger.go Outdated Show resolved Hide resolved
@kostko
Copy link
Member Author

kostko commented Sep 6, 2022 •
edited
Loading

Doesn't this need to bump the runtime host protocol?

As this only adds methods, old runtimes should continue to work (which is desirable for an easy migration). It's just that newer runtimes would no longer work on old nodes.

Maybe we could bump the minor version, let me check how this is implemented.

@kostko
Copy link
Member Author

kostko commented Sep 6, 2022

Ah yes, I'll bump the minor RHP version.

@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from d2e2788 to e2fcb36 Compare September 6, 2022 14:06
@kostko
Copy link
Member Author

kostko commented Sep 6, 2022

Bumped.

@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from e2fcb36 to fc568b0 Compare September 7, 2022 09:08
@kostko kostko force-pushed the kostko/feature/rhp-consensus-events branch from fc568b0 to 9cde8d9 Compare September 9, 2022 13:40
@kostko kostko merged commit da6efaf into master Sep 9, 2022
@kostko kostko deleted the kostko/feature/rhp-consensus-events branch September 9, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peternose peternose peternose left review comments

@Yawning Yawning Yawning approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

@ptrus ptrus Awaiting requested review from ptrus ptrus is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kostko @Yawning @peternose