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

Tendermint state follower #1916

Merged
merged 6 commits into from
Aug 21, 2019
Merged

Tendermint state follower #1916

merged 6 commits into from
Aug 21, 2019

Conversation

pro-wh
Copy link
Contributor

@pro-wh pro-wh commented Jul 13, 2019

This has the passive Tendermint state follower for use in the Byzantine node.

blocks #1936

also has the start of the Byzantine node subcommand

@Yawning
Copy link
Contributor

Yawning commented Jul 15, 2019

While you're here and messing with the dependency system, can you change the enforcement to happen in the mux/ApplicationServer.Start() call? I would just do it myself, but I create enough churn in everyone's branches as it is, and you're more invested in the notion than I am.

Rationale: Being less dependent on initialization order is a good thing, and at some point we may even want dependency loops (though this will require other refactoring).

@pro-wh
Copy link
Contributor Author

pro-wh commented Jul 15, 2019

ok

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from 665108c to 77e3cbe Compare July 16, 2019 02:00
@willscott
Copy link
Contributor

It sounds like one thing we should settle on here is what specific byzantine behavior and tests we want to run regularly with the use of this code. Do we have thoughts on what we want to scope in / out for the first pass?

@willscott
Copy link
Contributor

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch 3 times, most recently from 53a7766 to b7afdc2 Compare July 18, 2019 00:59
@pro-wh pro-wh changed the title byzantine node Tendermint state follower Jul 18, 2019
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch 2 times, most recently from 857e03f to a96a087 Compare July 19, 2019 00:15
@codecov
Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #1916 into master will decrease coverage by 0.18%.
The diff coverage is 38.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
- Coverage   53.96%   53.78%   -0.19%     
==========================================
  Files         242      244       +2     
  Lines       23733    23726       -7     
==========================================
- Hits        12807    12760      -47     
- Misses       9544     9596      +52     
+ Partials     1382     1370      -12
Impacted Files Coverage Δ
go/ekiden/cmd/debug/dummy/dummy.go 14.28% <ø> (ø) ⬆️
go/registry/tendermint/tendermint.go 65.05% <0%> (ø) ⬆️
go/beacon/tendermint/tendermint.go 60% <0%> (ø) ⬆️
go/staking/tendermint/tendermint.go 60.48% <0%> (ø) ⬆️
go/keymanager/tendermint/tendermint.go 38.27% <0%> (ø) ⬆️
...ommon/crypto/signature/signers/file/file_signer.go 54.21% <0%> (ø) ⬆️
go/epochtime/tendermint_mock/tendermint_mock.go 59.48% <0%> (ø) ⬆️
go/roothash/tendermint/tendermint.go 55.09% <0%> (ø) ⬆️
go/ekiden/cmd/debug/byzantine/steps.go 0% <0%> (ø)
go/scheduler/tendermint/tendermint.go 61.22% <0%> (ø) ⬆️
... and 21 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 9d6955c...35d3d63. Read the comment docs.

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from a96a087 to ec74df8 Compare July 23, 2019 00:46
@pro-wh pro-wh marked this pull request as ready for review July 23, 2019 00:48
@pro-wh pro-wh requested review from kostko and Yawning as code owners July 23, 2019 00:48
@pro-wh
Copy link
Contributor Author

pro-wh commented Jul 23, 2019

At this point, the PR has been scoped down to just the tendermint follower, which all the main idea is already coded up, so reviewers can start taking a look. It's still broken though, with the benign Tendermint node not syncing with the network. It's something I'm doing wrong when trying to set it up. Some debugging code remains too.

go/ekiden/cmd/debug/byzantine/byzantine.go Outdated Show resolved Hide resolved
go/ekiden/cmd/debug/byzantine/steps.go Outdated Show resolved Hide resolved
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from ec74df8 to 803e9ff Compare July 25, 2019 00:33
@pro-wh pro-wh requested a review from ptrus July 25, 2019 16:18
@pro-wh
Copy link
Contributor Author

pro-wh commented Jul 25, 2019

@ptrus: this will interact with #1889 due to epochtime being injected into all the ABCI mux apps

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch 3 times, most recently from be5a07b to 5f810cd Compare August 1, 2019 20:35
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch 2 times, most recently from 7c449d9 to f65a5d8 Compare August 15, 2019 18:16
go/ekiden/cmd/debug/byzantine/byzantine.go Outdated Show resolved Hide resolved
go/ekiden/cmd/debug/byzantine/byzantine.go Outdated Show resolved Hide resolved
go/ekiden/cmd/debug/byzantine/steps.go Outdated Show resolved Hide resolved
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from f65a5d8 to e20f12e Compare August 16, 2019 20:25
@pro-wh pro-wh requested a review from kostko August 16, 2019 20:32
@pro-wh
Copy link
Contributor Author

pro-wh commented Aug 16, 2019

we now propagate errors. it's mostly converted from panics, so steps might not leave the process in a very good state when they fail

@kostko
Copy link
Member

kostko commented Aug 18, 2019

After reviewing #1959 I guess some of the new/start separation could be done here.

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from e20f12e to 98785a6 Compare August 19, 2019 23:36
@pro-wh
Copy link
Contributor Author

pro-wh commented Aug 19, 2019

updates:

  • error messages are not so aggressively abbreviated anymore

@pro-wh pro-wh requested a review from kostko August 19, 2019 23:39
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from 98785a6 to 35d3d63 Compare August 21, 2019 00:11
// GetEpochBlock implements epochtime Backend.
func (*fakeTimeBackend) GetEpochBlock(ctx context.Context, epoch api.EpochTime) (int64, error) {
panic("GetEpochBlock not supported")
// return int64(epoch) * 30, nil
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya ok

@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from 35d3d63 to e46d5a2 Compare August 21, 2019 17:50
@pro-wh pro-wh force-pushed the pro-wh/feature/byzantine branch from e46d5a2 to 7060382 Compare August 21, 2019 18:02
@pro-wh pro-wh merged commit 0ce88d9 into master Aug 21, 2019
@pro-wh pro-wh deleted the pro-wh/feature/byzantine branch August 21, 2019 18:28
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.

4 participants