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

Remove separate storage nodes #4308

Merged
merged 17 commits into from
Jan 5, 2022
Merged

Remove separate storage nodes #4308

merged 17 commits into from
Jan 5, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Oct 11, 2021

This removes the separate storage committee and makes all compute nodes always imply storage as well. A notion of runtime mode is introduced which can be one of none, compute, keymanager, client and client-stateless. Storage receipts are removed and executor commitments are used as storage availability commitments as well. An additional Byzantine scenario is added where a faulty proposer generates bogus batches by including transactions that nobody else has.

The structure of proposals is changed so that executor workers don't need to download transactions again, but instead use the existing P2P transaction dispatch mechanism.

Also fixes #3905.

Note that this change is consensus-breaking since it changes the committee scheduler and registry.

TODO

  • Fix broken unit tests.
  • Reinstate unavailable storage failure code
  • Refactor executor commits such that roothash evidence does not need to include submitted messages.
  • Refactor signature scheme for proposals and executor commits.
  • Explicit transaction requests for peers in case of missing transaction hashes.
  • Cleanup and further simplifications.
  • Think about more Byzantine scenarios.
  • Support for converting genesis dump from consensus v4.
  • Long-term tests.
  • Changelog entries.

@kostko kostko added c:roothash Category: root hash service c:runtime Category: runtime c:breaking/consensus Category: breaking consensus changes c:breaking/cfg Category: breaks configuration c:breaking/runtime Category: breaking runtime changes labels Oct 11, 2021
@kostko kostko force-pushed the kostko/feature/executor-v2 branch 10 times, most recently from 03663e9 to 0669c4c Compare October 27, 2021 16:04
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #4308 (4a9c33f) into master (8b5073b) will decrease coverage by 0.01%.
The diff coverage is 77.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4308      +/-   ##
==========================================
- Coverage   68.85%   68.83%   -0.02%     
==========================================
  Files         413      414       +1     
  Lines       47070    46309     -761     
==========================================
- Hits        32408    31879     -529     
+ Misses      10681    10516     -165     
+ Partials     3981     3914      -67     
Impacted Files Coverage Δ
go/common/version/version.go 80.26% <ø> (ø)
go/consensus/tendermint/api/api.go 84.15% <ø> (ø)
...o/consensus/tendermint/apps/scheduler/scheduler.go 71.14% <ø> (-0.83%) ⬇️
go/consensus/tendermint/apps/scheduler/shuffle.go 79.45% <ø> (-0.81%) ⬇️
go/oasis-node/cmd/common/genesis/staking.go 68.57% <ø> (-0.45%) ⬇️
go/oasis-node/cmd/registry/node/node.go 43.90% <ø> (+0.35%) ⬆️
go/oasis-node/cmd/stake/stake.go 48.25% <ø> (-0.26%) ⬇️
go/oasis-node/cmd/storage/storage.go 5.07% <0.00%> (-0.16%) ⬇️
go/roothash/api/block/header.go 90.00% <0.00%> (+7.91%) ⬆️
go/runtime/registry/host.go 78.15% <ø> (+4.11%) ⬆️
... and 146 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 ded43ef...4a9c33f. Read the comment docs.

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.

There appears to be some terminology inconsistency between compute and executor. Should this be unified to the latter?

go/oasis-test-runner/scenario/e2e/byzantine_beacon.go Outdated Show resolved Hide resolved
.changelog/4308.cfg.md Show resolved Hide resolved
@@ -504,20 +492,6 @@ func VerifyRegisterNodeArgs( // nolint: gocyclo
return nil, nil, fmt.Errorf("%w: expiration period greater than allowed", ErrInvalidArgument)
}

// Make sure that a node has at least one valid role.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This was moved to node.ValidateBasic

go/roothash/api/commitment/executor.go Outdated Show resolved Hide resolved
go/roothash/api/commitment/executor_test.go Outdated Show resolved Hide resolved
ErrTxnSchedSigInvalid = p2pError.Permanent(errors.New(moduleName, 12, "roothash/commitment: txn scheduler signature invalid"))
ErrInvalidMessages = p2pError.Permanent(errors.New(moduleName, 13, "roothash/commitment: invalid messages"))
ErrBadStorageReceipts = errors.New(moduleName, 14, "roothash/commitment: bad storage receipts")
// Error code 12 is reserved for future use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reserved for future use, or reserved to never be used again? I guess we can reuse them after the next upgrade...

go/storage/api/metrics.go Show resolved Hide resolved
go/worker/compute/executor/committee/node.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/executor-v2 branch 2 times, most recently from a138445 to 73df582 Compare December 9, 2021 10:40
@kostko
Copy link
Member Author

kostko commented Dec 9, 2021

There appears to be some terminology inconsistency between compute and executor. Should this be unified to the latter?

Previously compute was a "super role" that a node would register for and the scheduler would then elect into subroles like executor, txnscheduler, merge but with the changed architecture compute basically equals executor (merge has been gone for a while and txnscheduler now rotates within the executor committee).

@kostko kostko marked this pull request as ready for review December 9, 2021 10:50
@kostko kostko force-pushed the kostko/feature/executor-v2 branch 2 times, most recently from 104d350 to 2499ebb Compare January 3, 2022 14:28
go/runtime/txpool/check_queue.go Show resolved Hide resolved
go/runtime/txpool/txpool.go Outdated Show resolved Hide resolved
go/runtime/txpool/txpool.go Outdated Show resolved Hide resolved
go/runtime/txpool/txpool.go Outdated Show resolved Hide resolved
go/runtime/txpool/txpool.go Outdated Show resolved Hide resolved
go/worker/client/service.go Outdated Show resolved Hide resolved
go/worker/client/service.go Outdated Show resolved Hide resolved
go/worker/common/committee/accessctl.go Outdated Show resolved Hide resolved
go/worker/common/p2p/p2p.go Outdated Show resolved Hide resolved
go/worker/compute/executor/committee/p2p.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/executor-v2 branch 3 times, most recently from c62a630 to 27a21ce Compare January 4, 2022 09:31
@kostko kostko force-pushed the kostko/feature/executor-v2 branch 3 times, most recently from b4021fc to 5f0194c Compare January 4, 2022 11:46
@kostko
Copy link
Member Author

kostko commented Jan 4, 2022

Multiple runs of long-term tests seem to pass now, running one more on the latest commit.

@kostko
Copy link
Member Author

kostko commented Jan 5, 2022

Latest long-term test run has been successful.

@kostko kostko merged commit 20ff2c5 into master Jan 5, 2022
@kostko kostko deleted the kostko/feature/executor-v2 branch January 5, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/cfg Category: breaks configuration c:breaking/consensus Category: breaking consensus changes c:breaking/runtime Category: breaking runtime changes c:roothash Category: root hash service c:runtime Category: runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove storage receipts from runtime genesis
3 participants