epochtime refactor #1889

ptrus · 2019-07-02T14:04:17Z

Fixes: #1793

Done:

epochtime -> ticker
enable mock_ticker via flag
simplify mock_ticker (instead of setting whatever "epoch" only allow advancing ticks now)

TODO:

Remove all epoch references from ticker
- can be a separate package that uses ticker as a timesource & has epoch interval defined
potentially get rid of the notion of epoch altogether?
- this would require changing a lot of code, as epoch's are used everywhere
- instead, maybe we should for now leave: epoch==compute schedule interval
fix all the flags/etc

Proposed service changes:

beacon:
- go/registry: epochtime cleanup #1899
keymanager:
- not sure if any changes needed for now?
registry:
- on epoch change: remove expired nodes (= keep expiration per "global epoch")
- go/registry: epochtime cleanup #1899
scheduler: (separate PR probably)
- configurable intervals for committee scheduling
- shorten transaction leader
- increase storage committee interval

ptrus · 2019-07-02T14:04:59Z

~~hm i wanted to create a DRAFT PR~~ nvm since it can't be done without completly deleting this one somehow. whatever

ptrus · 2019-07-02T14:07:42Z

@Yawning please check commit: 035e2fe and if the TODO's make sense? i'm a bit torn with completely removing the notion of epoch right now, since so many things rely on it?

pro-wh · 2019-07-02T16:57:33Z

go/tendermint/apps/ticker_mock/api.go

+}
+
+// TxDoTick is a transaction for triggering a tick.
+type TxDoTick struct {


ℹ️ change from epochtime mock: you no longer specify how far to move forward

do we want that? with this it'll take multiple ticks to change committees

probably might make sense to add it back (performance wise) as otherwise it might take a lot of transactions to do enough ticks to trigger an epoch transition 👍 (also it's only used for debug/tests)

pro-wh · 2019-07-02T16:58:03Z

go/tendermint/apps/ticker_mock/api.go

+}
+
+// QueryGetTickResponse is a response to QueryGetTick.
+type QueryGetTickResponse struct {


ℹ️ change from epochtime mock: no longer pass a height parameter

i don't think it's needed/was used

pro-wh · 2019-07-02T17:00:12Z

go/tendermint/apps/ticker_mock/state.go

+
+const (
+	// Mock ticker state.
+	stateCurrentTick = "ticker_mock/current"


ℹ️ change from epochtime mock: what has previously been two keys is now unified into a single state item that expresses both the current tick and the are-we-going-to-tick info.

yes a bit simplified. It had to be 2 states before as the next tick could be any number

go/tendermint/apps/ticker_mock/state.go

go/ticker/tendermint/tendermint.go

go/ticker/tendermint_mock/tendermint_mock.go

go/worker/storage/storage.go

go/tendermint/apps/ticker_mock/api.go

pro-wh · 2019-07-02T22:02:43Z

go/epochtime/init.go

-)
-
-const (
-	cfgBackend            = "epochtime.backend"


cc @peterjgilbert for runtime-ethereum effects, where epochtime.backend is the only change to e2e test

Yawning · 2019-07-03T11:15:23Z

make beacon interval configurable (settable via a setting) ("beacon epoch"?)

This needs to be equal to the greatest common denominator of all of the scheduling intervals.

on epoch change: remove expired nodes (= keep expiration per "global epoch")

Likewise, this should be done at a rate equal to the greatest common denominator of all of the scheduling intervals, to avoid a node disappearing mid-interval.

simplify mock_ticker (instead of setting whatever "epoch" only allow advancing ticks now)

Might be slightly more complicated. What happens when say, the advance interval is > 1. Do all intervening epoch changes get triggered? Right now this isn't needed but it might be for correct functionality.

That said the primary place where this thing is required for the Go tests. The e2e migration test can likely be rewritten to remove the dependence on this backend.

(keymanager:) not sure if any changes needed for now?

#1812

i'm a bit torn with completely removing the notion of epoch right now, since so many things rely on it?

Specify a to-be-removed interval with the new semantics, and call that an epoch, and incrementally transition things over.

Yawning · 2019-07-03T11:17:07Z

While I'm here thinking about this, the interval(s) should likely be a consensus parameter rather than something passed in as a flag.

codecov · 2019-07-16T12:29:06Z

Codecov Report

Merging #1889 into master will decrease coverage by 0.38%.
The diff coverage is 64.44%.

@@            Coverage Diff             @@
##           master    #1889      +/-   ##
==========================================
- Coverage   54.97%   54.58%   -0.39%     
==========================================
  Files         235      232       -3     
  Lines       22067    21777     -290     
==========================================
- Hits        12131    11888     -243     
+ Misses       8637     8586      -51     
- Partials     1299     1303       +4

Impacted Files	Coverage Δ
go/scheduler/api/api.go	`86.04% <ø> (ø)`	⬆️
go/worker/storage/storage.go	`62.33% <ø> (ø)`
go/roothash/init.go	`80.95% <ø> (ø)`	⬆️
go/keymanager/init.go	`84.61% <ø> (ø)`	⬆️
go/roothash/tendermint/tendermint.go	`64.12% <ø> (ø)`	⬆️
go/ticker/tendermint/tendermint.go	`0% <0%> (ø)`
go/registry/tests/tester.go	`87.67% <100%> (ø)`	⬆️
go/beacon/tests/tester.go	`100% <100%> (ø)`	⬆️
go/tendermint/apps/beacon/beacon.go	`72.61% <100%> (ø)`	⬆️
go/keymanager/tendermint/tendermint.go	`38.27% <100%> (ø)`	⬆️
... and 58 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 2e4ac27...0667695. Read the comment docs.

ptrus · 2019-07-16T15:16:14Z

i think this is ready for another review @Yawning & @pro-wh

pro-wh

review in progress

pro-wh · 2019-07-19T00:20:46Z

.buildkite/scripts/common_e2e.sh

 #   EKIDEN_VALIDATOR_SOCKET
 #   EKIDEN_CLIENT_SOCKET
 #   EKIDEN_ENTITY_PRIVATE_KEY
 #
 # Optional named arguments:
 #
-#   epochtime_backend - epochtime backend (default: tendermint)
+#   ekiden_ticker_settable - settable ticker (default: 1)


Suggested change

# ekiden_ticker_settable - settable ticker (default: 1)

# ekiden_ticker_settable - use settable ticker instead of interval ticker (default: 1)

pro-wh · 2019-07-19T00:21:54Z

.buildkite/scripts/common_e2e.sh

        ${EKIDEN_BEACON_DETERMINISTIC:+--beacon.debug.deterministic} \
        --metrics.mode none \
        --storage.backend cachingclient \
        --storage.cachingclient.file ${data_dir}/storage-cache \
        --consensus.backend tendermint \
+        --tendermint.abci.epoch_interval 5 \


ℹ️ so overall 25 height per epoch now, down from 30

go/ticker/tests/mock_tester.go

pro-wh · 2019-07-19T00:28:08Z

go/ticker/tests/mock_tester.go

+		require.NoError(nerr, "GetEpoch")
+		if epoch != newEpoch {
+			// After epoch changed, do one more tick.
+			err = backend.DoTick(ctx)


why do we need this?

pro-wh · 2019-07-19T01:23:20Z

go/tendermint/abci/mux.go

@@ -785,40 +787,57 @@ func (s *ApplicationState) CheckTxTree() *iavl.MutableTree {
 	return s.checkTxTree
 }

+// GetEpoch returns the current scheduling epoch.
+func (s *ApplicationState) GetEpoch(timeSource ticker.Backend) (scheduler.EpochTime, error) {


what would this look like when there are multiple epoch intervals for different committee types? additional parameter maybe?

pro-wh · 2019-07-19T01:26:42Z

go/dummydebug/grpc.go

 	if err != nil {
 		return nil, err
 	}

-	return &dbgPB.SetEpochResponse{}, nil
+	// TODO: make it not get stuck


😮 should we do that?

pro-wh · 2019-07-19T01:30:59Z

go/tendermint/apps/registry/registry.go

-	if epochtime.EpochTime(node.Expiration) < epoch {
+	if node.Expiration == 0 {
+		// If Expiration == 0, register for next epoch.
+		node.Expiration = uint64(epoch) + 2


what led to this change?

pro-wh · 2019-07-19T01:31:50Z

go/go.mod

@@ -76,5 +76,5 @@ require (
 	golang.org/x/text v0.3.2 // indirect
 	google.golang.org/appengine v1.4.0 // indirect
 	google.golang.org/genproto v0.0.0-20190611190212-a7e196e89fd3 // indirect
-	google.golang.org/grpc v1.21.1
+	google.golang.org/grpc v1.22.0


pro-wh · 2019-07-19T01:33:01Z

go/go.sum

@@ -550,3 +554,4 @@ gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
+honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=


ℹ️ apparently this is Staticcheck https://github.com/dominikh/go-tools

pro-wh · 2019-07-19T01:33:54Z

go/registry/tests/tester.go

 	"github.com/oasislabs/ekiden/go/registry/api"
+	schedulerApi "github.com/oasislabs/ekiden/go/scheduler/api"


imported as scheduler elsewhere, and it seems scheduler scheduler.Backend works okay... hm

pro-wh · 2019-07-19T23:36:05Z

go/worker/registration/registration.go

@@ -151,10 +154,9 @@ func (r *Registration) registerNode(epoch epochtime.EpochTime) error {
 	}
 	identityPublic := r.identity.NodeSigner.Public()
 	nodeDesc := node.Node{
-		ID:         identityPublic,
-		EntityID:   r.entitySigner.Public(),
-		Expiration: uint64(epoch) + 2,


should we remove the field then?

kostko · 2019-08-21T19:40:28Z

What's the plan with this PR?

ptrus · 2020-06-29T16:40:59Z

Closing this one as it will probably be easier to start this from scratch now with all the changes happening in mean time - also the approach will likely be different.

ptrus requested review from kostko and Yawning as code owners July 2, 2019 14:04

ptrus closed this Jul 2, 2019

ptrus reopened this Jul 2, 2019

ptrus changed the title ~~WIP: epochtime refactor~~ "DRAFT": WIP: epochtime refactor Jul 2, 2019

pro-wh self-requested a review July 2, 2019 16:46

pro-wh reviewed Jul 2, 2019

View reviewed changes

ptrus mentioned this pull request Jul 8, 2019

go/registry: epochtime cleanup #1899

Merged

ptrus force-pushed the ptrus/feature/ticker-epochtime branch 2 times, most recently from df9fa93 to 689e2cd Compare July 16, 2019 12:29

ptrus requested a review from peterjgilbert as a code owner July 16, 2019 12:29

ptrus force-pushed the ptrus/feature/ticker-epochtime branch 5 times, most recently from f8faed7 to 507c75c Compare July 16, 2019 14:20

ptrus changed the title ~~"DRAFT": WIP: epochtime refactor~~ WIP: epochtime refactor Jul 16, 2019

ptrus force-pushed the ptrus/feature/ticker-epochtime branch from 507c75c to 2759d5e Compare July 16, 2019 14:41

ptrus changed the title ~~WIP: epochtime refactor~~ epochtime refactor Jul 16, 2019

go/epochtime: refactor to ticker

0667695

ptrus force-pushed the ptrus/feature/ticker-epochtime branch from 2759d5e to 0667695 Compare July 16, 2019 15:14

ptrus mentioned this pull request Jul 18, 2019

go/registry: update node info #1864

Merged

pro-wh reviewed Jul 19, 2019

View reviewed changes

pro-wh self-requested a review July 19, 2019 01:38

pro-wh reviewed Jul 19, 2019

View reviewed changes

pro-wh mentioned this pull request Jul 25, 2019

Tendermint state follower #1916

Merged

kostko marked this pull request as draft June 29, 2020 16:18

ptrus closed this Jun 29, 2020

ptrus deleted the ptrus/feature/ticker-epochtime branch June 29, 2020 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epochtime refactor #1889

epochtime refactor #1889

ptrus commented Jul 2, 2019 •

edited

Loading

ptrus commented Jul 2, 2019 •

edited

Loading

ptrus commented Jul 2, 2019 •

edited

Loading

pro-wh Jul 2, 2019

ptrus Jul 16, 2019 •

edited

Loading

pro-wh Jul 2, 2019

ptrus Jul 16, 2019

pro-wh Jul 2, 2019

ptrus Jul 16, 2019

pro-wh Jul 2, 2019

Yawning commented Jul 3, 2019

Yawning commented Jul 3, 2019

codecov bot commented Jul 16, 2019 •

edited

Loading

ptrus commented Jul 16, 2019

pro-wh left a comment

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

pro-wh Jul 19, 2019

kostko commented Aug 21, 2019

ptrus commented Jun 29, 2020

	# ekiden_ticker_settable - settable ticker (default: 1)
	# ekiden_ticker_settable - use settable ticker instead of interval ticker (default: 1)

		"github.com/oasislabs/ekiden/go/registry/api"
		schedulerApi "github.com/oasislabs/ekiden/go/scheduler/api"

epochtime refactor #1889

epochtime refactor #1889

Conversation

ptrus commented Jul 2, 2019 • edited Loading

ptrus commented Jul 2, 2019 • edited Loading

ptrus commented Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

ptrus Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yawning commented Jul 3, 2019

Yawning commented Jul 3, 2019

codecov bot commented Jul 16, 2019 • edited Loading

Codecov Report

ptrus commented Jul 16, 2019

pro-wh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostko commented Aug 21, 2019

ptrus commented Jun 29, 2020

ptrus commented Jul 2, 2019 •

edited

Loading

ptrus commented Jul 2, 2019 •

edited

Loading

ptrus commented Jul 2, 2019 •

edited

Loading

ptrus Jul 16, 2019 •

edited

Loading

codecov bot commented Jul 16, 2019 •

edited

Loading