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

Clean up non-tm backends removal #1870

Closed
3 tasks done
ptrus opened this issue Jun 27, 2019 · 6 comments · Fixed by #1878
Closed
3 tasks done

Clean up non-tm backends removal #1870

ptrus opened this issue Jun 27, 2019 · 6 comments · Fixed by #1878
Assignees

Comments

@ptrus
Copy link
Member

ptrus commented Jun 27, 2019

Some cleanup things todo, leftover by: #1806

@ptrus ptrus self-assigned this Jun 27, 2019
@Yawning
Copy link
Contributor

Yawning commented Jun 28, 2019

remove backend flags wherever we are left with a single backend

Since there is a non-trivial amount of inter-dependency between components and the consensus implementation, my thought here is:

  • Add a consesus.backend flag that will set the beacon, keymanager, registry, roothash, scheduler, and staking backends (The consensus.backend flag can live under ekiden/cmd/common/flags).
  • Possibly fold epochtime under the consensus.backend banner, and give epochtime/tendermint_mock the go/beacon: Remove the insecure beacon #1872 treatment (If the timekeeping code wasn't scheduled for a major refactor/rewrite, I would push harder for this).

The only component that isn't concretely tied to all other components and the consensus layer is storage at this point. I think having a single flag that controls the backend selection for all of the linked components is the right thing to do, ie: consesus.backend=tendermint).

@Yawning
Copy link
Contributor

Yawning commented Jun 28, 2019

fix/re-enable discrepancy tests (if not done in: #1748)

All the nodes should have --beacon.debug.deterministic when running the discrepancy test.

@pro-wh
Copy link
Contributor

pro-wh commented Jun 28, 2019

remove backend flags wherever we are left with a single backend

Since there is a non-trivial amount of inter-dependency between components and the consensus implementation, my thought here is:

* Add a `consesus.backend` flag that will set the `beacon`, `keymanager`, `registry`, `roothash`, `scheduler`, and `staking` backends (The `consensus.backend` flag can live under `ekiden/cmd/common/flags`).

* Possibly fold `epochtime` under the `consensus.backend` banner, and give `epochtime/tendermint_mock` the #1872 treatment (If the timekeeping code wasn't scheduled for a major refactor/rewrite, I would push harder for this).

The only component that isn't concretely tied to all other components and the consensus layer is storage at this point. I think having a single flag that controls the backend selection for all of the linked components is the right thing to do, ie: consesus.backend=tendermint).

that'll be good. the tendermint stuff is really tightly coupled. I think yes on folding epochtime in, because all the other parts rely on it.

our backend APIs don't fit well with tendermint's processing model so it'll be less awkward when we don't have to handle that level of mixing and matching

@Yawning
Copy link
Contributor

Yawning commented Jun 28, 2019

  • Wire more things into the abci mux dependency thing. go/scheduler/tendermint/tendermint.go needs the tm beacon app as a "dependency", though if everything under consensus.backend=tendermint is understood to have implicit cross-dependencies, I'm not sure how useful the dependency mechanism is (since registering the apps should function regardless of ordering).

@Yawning
Copy link
Contributor

Yawning commented Jul 1, 2019

The other parts of this are going into the time keeping cleanup I assume?

@ptrus
Copy link
Member Author

ptrus commented Jul 1, 2019

yes (commented on the PR, forgot to also link here). The epochtime/time-keeping changes will be taken care of in: #1793

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 a pull request may close this issue.

3 participants