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

Incoming runtime messages (ADR 0011) #4415

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Incoming runtime messages (ADR 0011) #4415

merged 2 commits into from
Jan 28, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Jan 7, 2022 •
edited

This implements incoming runtime messages as per ADR 0011.

TODO

  • Add queries.
  • Update executor commitments.
  • Update runtime host protocol.
  • Update Rust runtime support library.
  • Update commit processing pipeline.
  • Update consensus roothash service to clear processed messages from queue.
  • Tests.

@kostko kostko added c:breaking/consensus Category: breaking consensus changes c:breaking/runtime Category: breaking runtime changes labels Jan 7, 2022
@kostko
Copy link
Member Author

kostko commented Jan 7, 2022

Task linked: CU-1zfmwm8 Incoming runtime messages

@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 5 times, most recently from ca6c571 to 8d3baef Compare January 12, 2022 12:23
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 4 times, most recently from a8fa0d8 to 9b5a58c Compare January 17, 2022 14:03
@kostko kostko marked this pull request as ready for review January 17, 2022 14:04
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 7 times, most recently from 4541af5 to 0bb7684 Compare January 18, 2022 12:05
pro-wh
pro-wh reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

5/42

docs/adr/0011-incoming-runtime-messages.md Show resolved Hide resolved
go/consensus/tendermint/apps/roothash/roothash.go
@@ -508,9 +516,6 @@ func (app *rootHashApplication) processRoundTimeout(ctx *tmapi.Context, state *r
return fmt.Errorf("failed to finalize block: %w", err)
}

if err = state.SetRuntimeState(ctx, rtState); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ this is moved into the above tryFinalizeBlock. all two callsites of tryFinalizeBlock would do this

but this breaks some symmetry, where we obtain rtState at this level, but we do the 'cleanup' procedures one level down inside tryFinalizeBlock

@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch from 0bb7684 to 0f54266 Compare January 19, 2022 14:48
@codecov
Copy link

codecov bot commented Jan 19, 2022 •
edited

Codecov Report

Merging #4415 (6a038f4) into master (22988e8) will decrease coverage by 0.05%.
The diff coverage is 60.77%.

❗ Current head 6a038f4 differs from pull request most recent head c95e5a1. Consider uploading reports for the commit c95e5a1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4415      +/-   ##
==========================================
- Coverage   68.72%   68.67%   -0.06%     
==========================================
  Files         414      415       +1     
  Lines       46280    46565     +285     
==========================================
+ Hits        31806    31978     +172     
- Misses      10560    10633      +73     
- Partials     3914     3954      +40
Impacted Files Coverage Δ
go/registry/api/runtime.go 43.94% <ø> (ø)
go/roothash/api/block/header.go 90.00% <ø> (ø)
go/roothash/api/grpc.go 41.42% <0.00%> (+5.32%) ⬆️
go/runtime/host/protocol/types.go 53.84% <ø> (ø)
go/consensus/tendermint/api/context.go 88.63% <40.00%> (-3.13%) ⬇️
go/consensus/tendermint/apps/roothash/roothash.go 74.05% <54.23%> (-0.32%) ⬇️
go/consensus/tendermint/roothash/roothash.go 67.76% <54.76%> (-1.75%) ⬇️
go/worker/compute/executor/committee/node.go 68.40% <60.00%> (-2.53%) ⬇️
...o/consensus/tendermint/apps/staking/state/state.go 63.62% <63.63%> (+0.29%) ⬆️
go/common/namespace.go 90.00% <66.66%> (+0.34%) ⬆️
... and 49 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 ff3641f...c95e5a1. Read the comment docs.

@kostko kostko mentioned this pull request Jan 20, 2022
Merged
5 tasks
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 2 times, most recently from 37b80a7 to 3a18290 Compare January 26, 2022 08:55
ptrus
ptrus reviewed Jan 27, 2022
View reviewed changes
go/consensus/tendermint/apps/roothash/roothash.go
)
// Make the round fail.
err = fmt.Errorf("finalized round contained invalid incoming message hash")
// TODO: All nodes contributing to this round should be penalized.
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue opened for this?

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, there is an internal task about adding slashing/suspension that I plan to work on next and I thought that should be part of it. Will open an issue as well.

go/registry/api/runtime.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch from 3a18290 to b54d2dd Compare January 28, 2022 08:58
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 2 times, most recently from d483836 to 6a038f4 Compare January 28, 2022 09:05
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch from 6a038f4 to c95e5a1 Compare January 28, 2022 09:54
@kostko kostko enabled auto-merge January 28, 2022 10:02
@kostko kostko merged commit c31ac9d into master Jan 28, 2022
@kostko kostko deleted the kostko/feature/incoming-rt-messages branch January 28, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pro-wh pro-wh pro-wh left review comments

@ptrus ptrus ptrus approved these changes

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

@Yawning Yawning Awaiting requested review from Yawning

Assignees
No one assigned
Labels
c:breaking/consensus Category: breaking consensus changes c:breaking/runtime Category: breaking runtime changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kostko @ptrus @pro-wh