Cleanup validator uptime computation #1270

mcortesi · 2020-12-15T16:37:02Z

Description

Several people had trouble following current uptime scoring logic. It looks like a pool for off by one errors. This PR attempts to do some cleanup to make it more maintainable.

Extracts the code to its own package where before code was distributed in 3+ files.
Add Tests
Add godocs to functions
Rename functions/variables to make code easier to understand
Introduce the type Window which simplifies reasoning about UP blocks
Modify the monitoringWindow and updateUptime functions so they work with the block whose signatures they intend to monitor, vs the block whose parent block signatures we want to monitor.

We can now summarize uptime computation as:

monitoringWindow(epoch): Range of blocks from an epoch where UP status will be monitor. Represents also the maximum number of blocks within an epoch a validator can be considered UP
currentLookbackWindow(blockNumber) Current range of blocks we consider to label a validator a UP. A valiadator is considered UP when it has sign at least a block in the currentLookbackWindow
validator.UpBlocks total number of blocks within an epoch a validator has been labeled as UP

Then the score is:

uptimeScore = validator.UpBlocks / monitoringWindow(epoch).Size()

Finally, for each block we process, the LastSignedBlock for each validator is updated, and then validator.UPBlocks is increased if: monitoringWindow.contains(blockNumber) && currentLookbackWindow.contains(lastSignedBlock)

Other changes

Improve epoch related functions
Add godocs to non related functions

Tested

Unit tests

Related issues

Backwards compatibility

Yes

consensus/istanbul/uptime/uptime.go

prestwich · 2020-12-15T17:29:24Z

consensus/istanbul/uptime/uptime.go

+	logger := um.logger.New("func", "Backend.updateValidatorScores", "epoch", epoch)
+	logger.Trace("Updating validator scores")
+
+	// The denominator is the (last block - first block + 1) of the val score tally window


Might be clearer if we explicitly say this is the maximum possible uptime score

agree that denominator doesn't really inform anything.

It's not the max uptime score (since score it's a ratio), but the total blocks that we are monitoring

So, what about:

// The totalMonitoredBlocks are the total number of block on which we monitor uptime for the epoch totalMonitoredBlocks := um.MonitoringWindowSize(epoch)

consensus/istanbul/uptime/uptime.go

oneeman

Definite improvement, but I think there's further we can go while we're at it. Added a bunch of comments, you can decide which of them make sense.

consensus/istanbul/uptime/uptime.go

oneeman · 2020-12-21T21:20:18Z

consensus/istanbul/uptime/uptime.go

+	// than couting up to the second to last one.
+	lastBlockToMonitor := epochLastBlock - 1
+
+	return firstBlockToMonitor, lastBlockToMonitor


To ask the question you mentioned to me previously, what happens if lastBlockToMontor < firstBlockToMonitor? (i.e. if lookbackWindowSize > epochSize - 2). Do we have safeguards to prevent that from happening?

i'll review this on when finishing #1136

consensus/istanbul/uptime/uptime.go

consensus/istanbul/utils.go

oneeman · 2020-12-21T22:40:49Z

consensus/istanbul/uptime/uptime_test.go

+		{"tally on first epoch", args{1, 10, 2}, 1 + 2, 9},
+		{"tally on second epoch", args{2, 10, 2}, 11 + 2, 19},
+		{"lookback window too big", args{1, 10, 10}, 11, 9},
+		// TODO: Add test cases.


Is this TODO intended to be completed before finalizing the PR? If not, I would remove it.

oneeman · 2020-12-21T22:44:04Z

consensus/istanbul/uptime/uptime_test.go

+	}{
+		{"tally on first epoch", args{1, 10, 2}, 1 + 2, 9},
+		{"tally on second epoch", args{2, 10, 2}, 11 + 2, 19},
+		{"lookback window too big", args{1, 10, 10}, 11, 9},


I'm skeptical about including a case with an invalid combination of lookback window size and epoch size. Is this a case that we are supposed to support or that should lead to a fatal error somewhere? If it's the latter, we should test elsewhere does it does lead to a fatal error, and not test it here.

i'll review this on when finishing #1136

consensus/istanbul/uptime/uptime_test.go

oneeman

Much improved. Left some minor comments here and there. Also, TestUptimeStorage is failing due to stray references to ScoreTally. That made me wonder: since the uptime store uses the db, would renaming a field cause problems when it comes to backwards compatibility?

Also, github says there are now merge conflicts (my guess is it's since 1.9.12 is merged to master now) 😬

consensus/istanbul/uptime/window.go

consensus/istanbul/uptime/uptime.go

consensus/istanbul/uptime/window.go

mcortesi · 2021-01-07T21:11:02Z

Much improved. Left some minor comments here and there. Also, TestUptimeStorage is failing due to stray references to ScoreTally. That made me wonder: since the uptime store uses the db, would renaming a field cause problems when it comes to backwards compatibility?

Also, github says there are now merge conflicts (my guess is it's since 1.9.12 is merged to master now) 😬

Fixed the stray reference.

About the field renaming, it doesn't cause problems. since RLP (the encoding) it's unaware of the names, only the structure of the type..

here's a test I've just did to verify this

func TestUptimeStorageChange(t *testing.T) {
	db := NewMemoryDatabase()
	epoch := uint64(0)

	// Create a test uptime to move around the database and make sure it's really new
	if entry := ReadAccumulatedEpochUptime(db, epoch); entry != nil {
		t.Fatalf("Non existent uptime returned: %v", entry)
	}

	expectedStoredUptime := &uptime.Uptime{
		Entries: []uptime.UptimeEntry{
			{UpBlocks: 0, LastSignedBlock: 1},
			{UpBlocks: 2, LastSignedBlock: 2},
			{UpBlocks: 8, LastSignedBlock: 8},
		},
		LatestBlock: 5,
	}

	type OldUptimeEntry struct {
		// Numbers of blocks validator is considered UP within monitored window
		UpBlocks        uint64
		LastSignedBlock uint64
	}
	type OldUptime struct {
		LatestBlock uint64
		Entries     []OldUptimeEntry
	}

	// Write and verify the uptime in the database
	uptime := &OldUptime{
		Entries: []OldUptimeEntry{
			{UpBlocks: 0, LastSignedBlock: 1},
			{UpBlocks: 2, LastSignedBlock: 2},
			{UpBlocks: 8, LastSignedBlock: 8},
		},
		LatestBlock: 5,
	}

	data, err := rlp.EncodeToBytes(uptime)
	if err != nil {
		t.Fatalf("Failed to RLP encode updated uptime %v", err)
	}
	if err := db.Put(uptimeKey(epoch), data); err != nil {
		t.Fatalf("Failed to store updated uptime: %v", err)
	}

	if entry := ReadAccumulatedEpochUptime(db, epoch); entry == nil {
		t.Fatalf("Stored uptime not found")
	} else if !reflect.DeepEqual(entry, expectedStoredUptime) {
		t.Fatalf("Retrieved uptime mismatch: have %v, want %v", entry, uptime)
	}
	// Delete the uptime and verify the execution
	DeleteAccumulatedEpochUptime(db, epoch)
	if entry := ReadAccumulatedEpochUptime(db, epoch); entry != nil {
		t.Fatalf("Deleted uptime returned: %v", entry)
	}
}

Instead of one method for first and last block on window, have a method to get the window [first, last] pair. - Add godocs to a few utils.go methods - Add tests to a few utils.go methods

- Centralize all uptime related logic into istanbul/uptime pkg. - Use interface for storage to ease testing later

We define a `Window` type and then uptime block are the sum of all blocks where block is within monitoredWindow, and lastSignedBlock for the validator is within currentLookback window. Additionally, updateUptime() function now receives the block is to monitor and the corresponding singatures, instead of receiving a block and the parent block signatures. So math is simplified. MonitoringWindow() now returns the blocks whose signatures we want to monitor and not the blocks whose parentBlock signatures we want to monitor.

mcortesi · 2021-01-07T21:16:36Z

Rebased to master to fix conflicts

oneeman

LGTM, very nice!

mcortesi requested review from oneeman and prestwich December 15, 2020 16:37

mcortesi requested review from kevjue, tkporter and trianglesphere as code owners December 15, 2020 16:37

mcortesi marked this pull request as draft December 15, 2020 16:37

prestwich reviewed Dec 15, 2020

View reviewed changes

oneeman suggested changes Dec 21, 2020

View reviewed changes

mcortesi marked this pull request as ready for review January 7, 2021 16:22

mcortesi force-pushed the mc/uptime-refactor branch from 4bc78f9 to 1923025 Compare January 7, 2021 17:07

oneeman reviewed Jan 7, 2021

View reviewed changes

consensus/istanbul/uptime/window.go Outdated Show resolved Hide resolved

mcortesi added 9 commits January 7, 2021 18:13

Rename "scoreTally" to "UptimeMonitorWindow"

a1a75be

Instead of one method for first and last block on window, have a method to get the window [first, last] pair. - Add godocs to a few utils.go methods - Add tests to a few utils.go methods

Extract uptime to its own package

e6ef6e4

- Centralize all uptime related logic into istanbul/uptime pkg. - Use interface for storage to ease testing later

First pass on PR comments

217e95d

Address PR comments

5a429ad

Address PR comment

be57b35

Address PR Comments

9217d8b

Fix test

cc980cf

pr comment fix

42f0da3

mcortesi force-pushed the mc/uptime-refactor branch from c41c105 to 42f0da3 Compare January 7, 2021 21:16

mcortesi requested review from oneeman and prestwich January 7, 2021 21:16

missing import after rebase

2a7ef52

oneeman approved these changes Jan 7, 2021

View reviewed changes

mcortesi merged commit 591e331 into master Jan 8, 2021

mcortesi deleted the mc/uptime-refactor branch January 8, 2021 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup validator uptime computation #1270

Cleanup validator uptime computation #1270

mcortesi commented Dec 15, 2020 •

edited

Loading

prestwich Dec 15, 2020

mcortesi Jan 7, 2021 •

edited

Loading

oneeman left a comment •

edited

Loading

oneeman Dec 21, 2020

mcortesi Jan 8, 2021

oneeman Dec 21, 2020

oneeman Dec 21, 2020

mcortesi Jan 8, 2021

oneeman left a comment •

edited

Loading

mcortesi commented Jan 7, 2021 •

edited

Loading

mcortesi commented Jan 7, 2021

oneeman left a comment

Cleanup validator uptime computation #1270

Cleanup validator uptime computation #1270

Conversation

mcortesi commented Dec 15, 2020 • edited Loading

Description

Other changes

Tested

Related issues

Backwards compatibility

prestwich Dec 15, 2020

Choose a reason for hiding this comment

mcortesi Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

oneeman left a comment • edited Loading

Choose a reason for hiding this comment

oneeman Dec 21, 2020

Choose a reason for hiding this comment

mcortesi Jan 8, 2021

Choose a reason for hiding this comment

oneeman Dec 21, 2020

Choose a reason for hiding this comment

oneeman Dec 21, 2020

Choose a reason for hiding this comment

mcortesi Jan 8, 2021

Choose a reason for hiding this comment

oneeman left a comment • edited Loading

Choose a reason for hiding this comment

mcortesi commented Jan 7, 2021 • edited Loading

mcortesi commented Jan 7, 2021

oneeman left a comment

Choose a reason for hiding this comment

mcortesi commented Dec 15, 2020 •

edited

Loading

mcortesi Jan 7, 2021 •

edited

Loading

oneeman left a comment •

edited

Loading

oneeman left a comment •

edited

Loading

mcortesi commented Jan 7, 2021 •

edited

Loading