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: Verify RAK in consensus state when serving requests #4741

Merged
merged 2 commits into from
May 16, 2022
Merged

runtime: Verify RAK in consensus state when serving requests #4741

merged 2 commits into from
May 16, 2022

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 6, 2022

No description provided.

@ptrus ptrus added the s:ready-ci Status: ready for CI label May 6, 2022
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch from 30e4876 to 2a95793 Compare May 6, 2022 11:28
@codecov
Copy link

codecov bot commented May 9, 2022 •
edited
Loading

Codecov Report

Merging #4741 (513420e) into master (84e8ba4) will decrease coverage by 0.15%.
The diff coverage is 37.83%.

❗ Current head 513420e differs from pull request most recent head 33553b1. Consider uploading reports for the commit 33553b1 to get more accurate results

@@            Coverage Diff             @@
##           master    #4741      +/-   ##
==========================================
- Coverage   66.92%   66.77%   -0.16%     
==========================================
  Files         441      441              
  Lines       49340    49348       +8     
==========================================
- Hits        33023    32952      -71     
- Misses      12262    12317      +55     
- Partials     4055     4079      +24
Impacted Files Coverage Δ
go/oasis-node/cmd/common/signer/signer.go 51.11% <0.00%> (-0.58%) ⬇️
go/runtime/registry/config.go 69.88% <0.00%> (ø)
go/common/crypto/signature/signers/remote/grpc.go 33.33% <63.63%> (+2.03%) ⬆️
go/runtime/host/sandbox/sandbox.go 60.82% <0.00%> (-9.28%) ⬇️
go/storage/api/root_cache.go 60.00% <0.00%> (-8.00%) ⬇️
go/worker/storage/committee/utils.go 92.30% <0.00%> (-7.70%) ⬇️
go/storage/api/metrics.go 81.66% <0.00%> (-5.00%) ⬇️
go/worker/storage/p2p/sync/client.go 86.95% <0.00%> (-4.35%) ⬇️
go/storage/mkvs/lookup.go 75.00% <0.00%> (-4.17%) ⬇️
go/runtime/host/protocol/connection.go 68.42% <0.00%> (-3.76%) ⬇️
... and 20 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 ad7b953...33553b1. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch 11 times, most recently from c4fe95a to 44e9a51 Compare May 10, 2022 06:50
Yawning
Yawning reviewed May 10, 2022
View reviewed changes
runtime/src/protocol.rs Outdated Show resolved Hide resolved
runtime/src/consensus/registry.rs Outdated
/// Information for this node's participation in the old PVSS based random beacon protocol.
/// Deprecated.
#[cbor(optional, default)]
pub beacon: Option<Vec<u8>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh, there's a PR that gets rid of this on the Go side, but apparently "no breaking changes" or something, so the branch is rotting.

#4668

@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch from 44e9a51 to 034ea80 Compare May 10, 2022 07:47
@ptrus ptrus marked this pull request as ready for review May 10, 2022 08:32
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch from 034ea80 to 723d4d5 Compare May 11, 2022 10:05
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch 3 times, most recently from dce48eb to d1c93b7 Compare May 12, 2022 06:59
kostko
kostko reviewed May 15, 2022
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Mostly looks good, I would just make sure that the RAK is advertised for the correct runtime ID/version combination.

runtime/src/consensus/registry.rs Outdated Show resolved Hide resolved
runtime/src/consensus/registry.rs Outdated Show resolved Hide resolved
runtime/src/consensus/tendermint/verifier.rs Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch from d1c93b7 to 237e215 Compare May 16, 2022 07:30
@ptrus
Copy link
Member Author

ptrus commented May 16, 2022

Going to do one more CI run with all SGX tests enabled on this branch, before merging.

kostko
kostko reviewed May 16, 2022
View reviewed changes
runtime/src/consensus/registry.rs Outdated Show resolved Hide resolved
runtime/src/consensus/tendermint/verifier.rs Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch 2 times, most recently from a2230d7 to cc6ad12 Compare May 16, 2022 08:50
@ptrus ptrus force-pushed the ptrus/feature/runtime-verify-rak branch from cc6ad12 to 33553b1 Compare May 16, 2022 13:50
@ptrus ptrus merged commit 92f25b0 into master May 16, 2022
@ptrus ptrus deleted the ptrus/feature/runtime-verify-rak branch May 16, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yawning Yawning Yawning left review comments

@kostko kostko kostko 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

Assignees
No one assigned
Labels
s:ready-ci Status: ready for CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ptrus @kostko @Yawning