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

a node that follows and checks #2340

Merged
merged 39 commits into from
Dec 16, 2019
Merged

a node that follows and checks #2340

merged 39 commits into from
Dec 16, 2019

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Nov 6, 2019

This is proposed as the beginning of auditing tools. There's no fast sync, so you would join the network as a node, and while you catch up, you do additional processing to check invariants. Our ABCI mux system allows us to put these checks in a mux app.

cc #2277

@kostko
Copy link
Member

kostko commented Nov 6, 2019

Probably also related to #1633. cc @abukosek

@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch 2 times, most recently from b1e98a9 to 15b8597 Compare November 7, 2019 00:31
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #2340 into master will increase coverage by <.01%.
The diff coverage is 61.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2340      +/-   ##
==========================================
+ Coverage   67.26%   67.26%   +<.01%     
==========================================
  Files         313      316       +3     
  Lines       29310    29564     +254     
==========================================
+ Hits        19714    19886     +172     
- Misses       7212     7251      +39     
- Partials     2384     2427      +43
Impacted Files Coverage Δ
go/registry/api/runtime.go 48.64% <ø> (ø) ⬆️
...o/consensus/tendermint/apps/scheduler/scheduler.go 71.15% <100%> (-0.46%) ⬇️
go/worker/keymanager/worker.go 58.25% <100%> (ø) ⬆️
go/keymanager/client/client.go 70.39% <100%> (+0.16%) ⬆️
go/worker/registration/worker.go 69.81% <100%> (+0.19%) ⬆️
go/oasis-node/cmd/debug/byzantine/registry.go 80% <100%> (+1.05%) ⬆️
go/consensus/tendermint/abci/mux.go 67.75% <100%> (+0.05%) ⬆️
go/registry/tests/tester.go 87.71% <100%> (+0.51%) ⬆️
go/worker/common/runtime_host.go 65.9% <33.33%> (-1.02%) ⬇️
go/oasis-node/cmd/registry/runtime/runtime.go 59.77% <37.5%> (+0.15%) ⬆️
... and 36 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 128de9d...91af13a. Read the comment docs.

@pro-wh pro-wh marked this pull request as ready for review November 7, 2019 01:36
@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from 15b8597 to ee85f8f Compare November 7, 2019 23:27
@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from ee85f8f to 0017437 Compare November 26, 2019 01:22
@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from 0017437 to a7bcdca Compare December 6, 2019 00:57
@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 6, 2019

new in this push:

  • checking not on every block. checking on some interval. and it's implemented in this way (which nobody asked for) where it randomly chooses one height in each interval
  • starting to port some sanity checks from genesis-time

@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 9, 2019

updates from Friday and additional notes:

  • mostly I've adapted genesis-time sanity checks by extracting functions that operate on abstract types. at genesis time, we call them with parts of the genesis document. at follow time, we load from the mux apps' state modules. there's some code sharing, but we'll have to be careful in the future to make sure we call these sanity checks from both places when we add new ones.
  • the mux apps' state modules tend to expose methods that load entire datasets into memory, e.g. all delegation records. that's consistent with the design of the genesis-time sanity checks, which operate on an all-in-memory genesis document. for now I'm going ahead and doing things that way

@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch 2 times, most recently from 340d387 to f7213e8 Compare December 10, 2019 21:37
@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 10, 2019

new in this push and from Monday:

  • fixes to increase unit test fidelity so that they pass followtool's sanity checks
  • changes to sanity checks tests to make the combined system not self-contradictory
  • specifying a key manager in a runtime record is now optional. key-manager-type runtimes now don't specify a (meta?) key manager. previously they would specify that they are their own key manager.
  • transfers of staking tokens to/from a runtime through roothash messages now adjust the staking token's total supply so that the staking mux app's state will still add up right after the transfer. this is intended to be an interim consistent behavior before we decommission runtime transfers for Remove staking-related roothash messages #2377.

@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from f7213e8 to 71df126 Compare December 10, 2019 22:13
@kostko
Copy link
Member

kostko commented Dec 11, 2019

specifying a key manager in a runtime record is now optional. key-manager-type runtimes now don't specify a (meta?) key manager. previously they would specify that they are their own key manager.

So this also fixes #2459 or it just makes the key manager optional?

go/genesis/api/api_test.go Outdated Show resolved Hide resolved
@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 11, 2019

So this also fixes #2459 or it just makes the key manager optional?

just makes the key manager optional

@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from 8357197 to abf746a Compare December 11, 2019 21:19
We previously left the total supply inconsistent when transferring
tokens to/from a runtime. Fix that to adjust the total supply
accordingly when transferring.

It was also proposed to move these transferred tokens to/from
per-runtime holding areas for bookkeeping. That remains a good
idea.

It's proposed in #2377 to dismantle this whole runtime transfer
system. So with any luck, none of this will be used after all.
We call Unix() and cast to unsigned in some parts of the code.
Changing the unspecific "very early" dummy time so that it doesn't
go negative and cause weird far-future times after casting.
@pro-wh pro-wh force-pushed the pro-wh/feature/followtool branch from abf746a to 7509fa3 Compare December 12, 2019 19:40
@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 12, 2019

in the latest push:

  • added more corrections to the node and to the checks to make sure we always pass the checks

ready for review

@pro-wh pro-wh requested a review from kostko December 12, 2019 20:49
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.

Rebase on master and get rid of protobuf updates, otherwise besides some minor nits I think this looks good. I think naming this something like "check invariants" or "invariant checker" would be better than "followtool" though.

go/keymanager/api/api.go Show resolved Hide resolved
go/consensus/tendermint/apps/followtool/api.go Outdated Show resolved Hide resolved
@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 16, 2019

renamed

@pro-wh
Copy link
Contributor Author

pro-wh commented Dec 16, 2019

ok all lights greenish. gonna merge

@pro-wh pro-wh merged commit d6a1665 into master Dec 16, 2019
@pro-wh pro-wh deleted the pro-wh/feature/followtool branch December 16, 2019 21:47
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.

2 participants