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

messages from roothash mux app #2147

Merged
merged 26 commits into from
Oct 16, 2019
Merged

messages from roothash mux app #2147

merged 26 commits into from
Oct 16, 2019

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Sep 20, 2019

This adds a "roothash messages" field to the compute pipeline. A runtime would be able to send them, and they propagate into a block header.

Roothash messages are stored inline in the block header, rather than in storage.

This implements the simple merging algorithm, where up to one compute committee can send roothash messages.

This adds the staking general balance adjustment message, which causes the staking mux app to adjust an account's general balance.

There's a dummy acceptable transfer peer policy to control what runtimes can send these staking general balance adjustment messages. It accesses a set of approved runtime IDs in the genesis document.

TODO:

  • switch dummy acceptable transfer peer policy to deny all
  • remove dummy roothash message types
  • test unacceptable transfer peer
  • come up with a way to test without hacking up successfulRound

blocks #2077
context https://github.com/oasislabs/private-rfcs/pull/144

fixes #2230
fixes #2231

@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch 3 times, most recently from 8a43a38 to 2af53df Compare October 4, 2019 00:24
@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from 2af53df to 56d2f56 Compare October 5, 2019 00:53
@pro-wh pro-wh marked this pull request as ready for review October 7, 2019 18:21
@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from 56d2f56 to 976e4d9 Compare October 8, 2019 00:11
@kostko
Copy link
Member

kostko commented Oct 8, 2019

Roothash messages are stored inline in the block header, rather than in storage.

Would it make more sense for the messages to be in the Block rather than the Header?

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.

Who will get slashed for reverting a round due to roothash messages not being accepted?

go/roothash/api/block/header.go Outdated Show resolved Hide resolved
go/tendermint/apps/roothash/roothash.go Outdated Show resolved Hide resolved
go/tendermint/apps/roothash/roothash.go Show resolved Hide resolved
@pro-wh
Copy link
Contributor Author

pro-wh commented Oct 8, 2019

Who will get slashed for reverting a round due to roothash messages not being accepted?

maybe transaction scheduler, if we would add a rule that it must know that the sent roothash messages will be satisfactory.

@pro-wh
Copy link
Contributor Author

pro-wh commented Oct 8, 2019

Roothash messages are stored inline in the block header, rather than in storage.

Would it make more sense for the messages to be in the Block rather than the Header?

dunno. I can't tell why there's even a separation between block and header

@kostko
Copy link
Member

kostko commented Oct 8, 2019

dunno. I can't tell why there's even a separation between block and header

Yeah that's a good point, so previously bulk data was in the block while the header only included hashes. I guess this doesn't really make sense and maybe we should just remove one of the two to make stuff simpler.

From this standpoint just leave it in the header and maybe open an issue to remove/combine.

@kostko
Copy link
Member

kostko commented Oct 8, 2019

I've opened #2241.

@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch 3 times, most recently from a4b3ce8 to 1e9e0eb Compare October 9, 2019 00:00
@pro-wh
Copy link
Contributor Author

pro-wh commented Oct 9, 2019

Updates: now there's a prototype of loading an acceptable transfer peer policy from the genesis file. This adds a field that's a whitelist of runtime IDs.

@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch 2 times, most recently from 45dbf40 to 1f16a93 Compare October 9, 2019 01:11
@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from 1f16a93 to f5c67f6 Compare October 9, 2019 22:21
@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #2147 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2147   +/-   ##
=======================================
  Coverage   53.79%   53.79%           
=======================================
  Files         263      263           
  Lines       28049    28049           
=======================================
  Hits        15088    15088           
  Misses      11430    11430           
  Partials     1531     1531

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 dd842d9...546351e. Read the comment docs.

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.

It would be useful to have an end-to-end test for roothash messages, so that the whole pipeline is tested.

@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from f5c67f6 to 611cf05 Compare October 11, 2019 21:28
@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from 611cf05 to d2ca69e Compare October 11, 2019 23:18
@pro-wh pro-wh force-pushed the pro-wh/feature/roothashmessage branch from eeffcba to 99a3f87 Compare October 15, 2019 01:07
@pro-wh pro-wh requested a review from kostko October 16, 2019 00:12
@pro-wh
Copy link
Contributor Author

pro-wh commented Oct 16, 2019

added an e2e test. please review

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.

Just minor nits.

go/oasis-node/cmd/debug/byzantine/compute.go Show resolved Hide resolved
go/oasis-test-runner/scenario/e2e/roothash_messages.go Outdated Show resolved Hide resolved
go/staking/api/api.go Outdated Show resolved Hide resolved
go/tendermint/apps/staking/state.go Outdated Show resolved Hide resolved
// acceptableTransferPeersKeyFmt is the key format used for the acceptable transfer peers set.
//
// Value is a CBOR-serialized map from acceptable runtime IDs the boolean true.
acceptableTransferPeersKeyFmt = keyformat.New(0x55)
Copy link
Member

Choose a reason for hiding this comment

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

I assume the size of this map is expected to be small (on the order of number of runtimes)? Another option would be to have the entries be stored in the tree -- e.g., one key per entry:

keyformat.New(0x55, &signature.MapKey{})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that's the expectation. Thanks for posting another option. Let's keep that in mind so we can later support a large number of acceptable runtimes if we want to.

for _, result := range results {
ioRoots = append(ioRoots, result.IORoot)
stateRoots = append(stateRoots, result.StateRoot)

// Merge roothash messages.
// The rule is that at most one result can have sent roothash messages.
Copy link
Member

Choose a reason for hiding this comment

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

Added to list of slashing conditions for the transaction scheduler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

runtime/src/transaction/context.rs Show resolved Hide resolved
@pro-wh
Copy link
Contributor Author

pro-wh commented Oct 16, 2019

@pro-wh pro-wh merged commit 163b84a into master Oct 16, 2019
@pro-wh pro-wh deleted the pro-wh/feature/roothashmessage branch October 16, 2019 18:51
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.

broadcastTx panics during shutdown Registration panics if we stop the compute worker early
2 participants