Track uptime & distribute rewards based on it #618

gakonst · 2019-11-20T01:33:58Z

Description

Will close #457. Idea is that we track a validator's uptime by the bitmap from my previous PR.

We can safely assume the bitmap represents 2/3rds of the validators, so we can multiply the number of 1s in the bitmap by 1.5 (3/2) to get an upper bound on the number of validators in that epoch.
We store each epoch's uptimes, but we could probably just store the latest epoch only
The "forgiving" factor means that a validator will get always a +1 point if they signed a block, and they will still get a +1, even if they did not sign it, as long as they had at least 1 signature in a sliding window before the current block.
We do not score for the first window blocks per epoch

Tested

Unit tests for the uptime tracking based on the excel sheet draft

Other changes

extra cli parameter for the window which defaults to 12

cmd/utils/flags.go

consensus/istanbul/backend/pos.go

asaj · 2019-11-20T01:44:21Z

consensus/istanbul/backend/pos.go

+	}
+
+	// the uptimes array is aligned with the validator set
+	validatorUptime := uptimes[validatorIndex].Score / (sb.EpochSize() - sb.LookbackWindow() + 1)


Won't this return 0? I think we actually want (uptimes[validatorIndex].Score * 10^24) / (sb.EpochSize() - sb.LookbackWindow() + 1)

yeah logic in the pos file was still WIP, should be fixed now

asaj · 2019-11-20T01:58:03Z

core/blockchain.go

+	var validatorsSize uint64
+	if len(uptime) == 0 {
+		// the number of validators is upper bound by 3/2 of the number of 1s in the bitmap
+		// TODO: due to this maybe we create an uptimeScore array which is 1 element bigger than required


So actually, since we're recording the uptime based on the signatures on the previous block, of which there may be more than 2F+1, we're likely to create an uptimeScore array much bigger than required

asaj · 2019-11-20T01:59:15Z

core/blockchain.go

+
+		signedValidatorsBitmap := extra.ParentAggregatedSeal.Bitmap
+		uptime := bc.GetAccumulatedUptime()
+		uptime = updateUptime(uptime, block.NumberU64(), signedValidatorsBitmap, bc.chainConfig.Istanbul.LookbackWindow)


Where is the per-epoch reset happening?

I changed the uptimes per epoch to be saved in different keys, so there is no direct need to save the data in order to calculate the uptime properly

core/blockchain.go

core/rawdb/accessors_chain.go

…ng window

kevjue · 2019-11-21T02:15:04Z

core/blockchain.go

+		if bitmap.Bit(i) == 1 {
+			// update their latest signed block and reward them
+			uptime[i].LastSignedBlock = blockNumber
+			if blockNumber >= firstBlockInEpoch+window-1 {


I think that this should be firstBlockInEpoch + window + 1. The aggregatedParentSignatures for the firstBlockInEpoch will be for a different valset than the current valset.

gakonst · 2019-11-21T06:27:09Z

core/blockchain.go

+			uptime[i].LastSignedBlock = blockNumber - 1
+		}
+
+		// If we are within the validator uptime tally window, then update the validator's score if it's last signed block is within


Suggested change

// If we are within the validator uptime tally window, then update the validator's score if it's last signed block is within

// If we are within the validator uptime tally window, then update the validator's score if its last signed block is within

gakonst · 2019-11-21T06:31:21Z

core/blockchain.go

+
+		// If we are within the validator uptime tally window, then update the validator's score if it's last signed block is within
+		// the lookback window
+		if valScoreTallyFirstBlockNum <= blockNumber && blockNumber <= valScoreTallyLastBlockNum {


LGTM, nit: we could maybe extract this and the valScoreTally{First, Last}BlockNum functions together to another function, isInLookbackWindow

gakonst · 2019-11-21T06:35:03Z

core/blockchain.go

+		if valScoreTallyFirstBlockNum <= blockNumber && blockNumber <= valScoreTallyLastBlockNum {
+			lastSignedBlock := uptime[i].LastSignedBlock
+			// TODO (kevjue) add comment about 2nd check
+			if signedBlockWindowFirstBlockNum <= lastSignedBlock && lastSignedBlock <= signedBlockWindowLastBlockNum {


Suggested change

if signedBlockWindowFirstBlockNum <= lastSignedBlock && lastSignedBlock <= signedBlockWindowLastBlockNum {

// increase our score as long as we signed at least one block within the lookback window,

// (even if we did not sign on the current block)

if signedBlockWindowFirstBlockNum <= lastSignedBlock && lastSignedBlock <= signedBlockWindowLastBlockNum {

gakonst · 2019-11-21T06:36:22Z

core/blockchain.go

+	if bc.engine.Protocol().Name == "istanbul" {
+		// The epoch's first block's aggregated parent signatures is for the previous epoch's valset.
+		// We can ignore updating the tally for that block.
+		if !istanbul.IsFirstBlockOfEpoch(block.NumberU64(), bc.chainConfig.Istanbul.Epoch) {


Nice catch! There's probably merit in us documenting all the edge cases we've encountered so far in this and my previous PR about parent seals regarding epoch transitions

core/blockchain_test.go

core/rawdb/accessors_chain.go

asaj

Looks great! Mostly nits

cmd/utils/flags.go

consensus/istanbul/backend/engine.go

consensus/istanbul/backend/pos.go

core/rawdb/accessors_chain.go

core/rawdb/accessors_chain_test.go

eth/backend.go

gakonst assigned asaj Nov 20, 2019

asaj reviewed Nov 20, 2019

View reviewed changes

asaj assigned gakonst Nov 20, 2019

gakonst added 4 commits November 20, 2019 22:36

feat: add methods to read/write the uptime to the database

1bde322

feat: track uptime as blocks arrive based on a sliding window

b6c389e

allow configuring the lookback window

c476eff

calculate uptime using the stored values

1bb1ac0

gakonst force-pushed the gakonst/uptime branch from 7026e7b to 1bb1ac0 Compare November 20, 2019 20:37

fix tests + lints

78a5d66

gakonst force-pushed the gakonst/uptime branch 2 times, most recently from 78a5d66 to a5f7ce0 Compare November 20, 2019 23:08

gakonst added 7 commits November 21, 2019 02:47

feat(rawdb): add methods to read/write the uptime to the database

0e1fb6f

feat(core/blockchain): track uptime as blocks arrive based on a slidi…

57df639

…ng window

feat: allow configuring the lookback window

c9ce086

feat(engine/pos): calculate uptime using the stored values

7cdad0c

fix(blockchain): fix bug in score calculation

3e3c442

fix(consensus-utils): fix bug in getting epoch number

2097c94

chore: cleanup

68dd946

gakonst force-pushed the gakonst/uptime branch from ca05652 to 68dd946 Compare November 21, 2019 00:50

timmoreton added the betanet-blocker label Nov 21, 2019

asaj assigned asaj and kevjue and unassigned asaj and gakonst Nov 21, 2019

kevjue reviewed Nov 21, 2019

View reviewed changes

asaj mentioned this pull request Nov 21, 2019

Update with new istanbul lookback window size flag celo-org/celo-monorepo#1820

Merged

Kevin Jue added 4 commits November 20, 2019 20:34

did a git pull

1cf9239

merged master

43c0177

fixed some types

a02426f

fixes

778cc8a

lint fixes and unit test fixes

19a2a44

asaj marked this pull request as ready for review November 21, 2019 06:11

asaj changed the title ~~[WIP] Track uptime & distribute rewards based on it~~ Track uptime & distribute rewards based on it Nov 21, 2019

Kevin Jue added 2 commits November 20, 2019 22:13

fixed lint

dacdcdf

more lint fixes, removed harded window of 2, updated comments

0dcf3ee

gakonst commented Nov 21, 2019

View reviewed changes

asaj approved these changes Nov 21, 2019

View reviewed changes

Asa Oines and others added 10 commits November 20, 2019 22:57

Address some comments

7f1adba

addressed PR comments

6868d26

git pull

8729e01

Address comments

26a8068

Merge

7bc314f

Fix tests

ebebbbd

fixed lint errors

7c4b421

Merge branch 'master' into gakonst/uptime

024a7b1

Repoint circle config

dc3d405

Repoint to master

5b4dbed

asaj merged commit 3ecd94a into master Nov 21, 2019

gakonst deleted the gakonst/uptime branch November 21, 2019 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track uptime & distribute rewards based on it #618

Track uptime & distribute rewards based on it #618

gakonst commented Nov 20, 2019 •

edited

Loading

asaj Nov 20, 2019

gakonst Nov 20, 2019

asaj Nov 20, 2019

asaj Nov 20, 2019

gakonst Nov 20, 2019

kevjue Nov 21, 2019 •

edited

Loading

gakonst Nov 21, 2019

gakonst Nov 21, 2019

gakonst Nov 21, 2019

gakonst Nov 21, 2019

asaj left a comment

	// If we are within the validator uptime tally window, then update the validator's score if it's last signed block is within
	// If we are within the validator uptime tally window, then update the validator's score if its last signed block is within

Track uptime & distribute rewards based on it #618

Track uptime & distribute rewards based on it #618

Conversation

gakonst commented Nov 20, 2019 • edited Loading

Description

Tested

Other changes

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

kevjue Nov 21, 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

asaj left a comment

Choose a reason for hiding this comment

gakonst commented Nov 20, 2019 •

edited

Loading

kevjue Nov 21, 2019 •

edited

Loading