Use BlockHeight to calculate uptime/downtime instead of timestamps. #2908

mikkeljuhl · 2018-03-28T14:16:34Z

Use BlockHeight to calculate uptime/downtime instead of timestamps. timePassed is calculated by checking how many blocks have passed * 10 minutes, instead of using the timestamps.
This is more reliable, if people should change their system time or things to that effect.

This is a move towards making a GlobalAverageUptime.

@ChrisSchinnerl @DavidVorick

ChrisSchinnerl

This looks pretty good to me overall. There are only a few things that could be refactored. We should wait for @DavidVorick to see if he also approves of those changes.

ChrisSchinnerl · 2018-03-28T15:06:49Z

modules/renter.go

@@ -119,8 +119,9 @@ type HostDBEntry struct {

 // HostDBScan represents a single scan event.
 type HostDBScan struct {
-	Timestamp time.Time `json:"timestamp"`
-	Success   bool      `json:"success"`
+	Timestamp   time.Time         `json:"timestamp"`


@DavidVorick is there still a need to keep the Timestamp?

I'm going to go ahead and say yes, we should keep the timestamp in the scan, just in case we find some use for it later. We may (or may not) phase it out eventually, but I can think of a few possible reasons we may want it at some point.

ChrisSchinnerl · 2018-03-28T15:08:30Z

modules/renter/hostdb/hostweight.go

@@ -311,6 +312,7 @@ func (hdb *HostDB) uptimeAdjustments(entry modules.HostDBEntry) float64 {
 	downtime := entry.HistoricDowntime
 	uptime := entry.HistoricUptime
 	recentTime := entry.ScanHistory[0].Timestamp


If we decide to get rid of the Timestamp field we can probably also get rid of that line and check the order later using the blockheight instead. We should wait for @DavidVorick to give some input on this though.

ChrisSchinnerl · 2018-03-28T15:11:35Z

modules/renter/hostdb/hostweight.go

 		if recentSuccess {
-			uptime += scan.Timestamp.Sub(recentTime)
+			uptime += time.Duration(timePassed * decay)


This code here should do pretty much the same as the one in updateEntry. At a first glance it seems like uptime and downtime themselves are not decayed.

Now that I think about it we can probably also change uptime downtime historicUptime and historicDowntime to be of type blockheight instead of Duration if we decide to do everything using blocks.

When we calculate the ratio later we would just cast them to float64.

We shouldn't turn historicUptime and historicDowntime into type BlockHeight.
Since, according to David we should time (and not block height) should be used for things (such as hosts) that lives outside the protocol

ChrisSchinnerl · 2018-03-28T15:15:06Z

modules/renter/hostdb/hostweight.go

+
+		blockDiff := scan.BlockHeight - recentBlockHeight
+		timePassed := float64(time.Duration(blockDiff) * 10 * time.Minute)
+		halftime := float64(1 * time.Hour * 24 * 30)


halftime should be in modules/renter/hostdb/consts.go instead of being declared inline in 2 files.

mikkeljuhl · 2018-04-03T18:30:48Z

@ChrisSchinnerl I've rebased and addressed your comments, please review 👍
@DavidVorick

DavidVorick · 2018-04-25T18:53:38Z

modules/renter/hostdb/consts.go

@@ -34,6 +34,9 @@ const (
 	// scans start getting compressed.
 	minScans = 12

+	// halftimeUpDownTime is the halftime used to decay the host up and downtime
+	halftimeUpDownTime = 30 * 24 * time.Hour


I think the word that you want here is halflife. I'd probably call this uptimeHalflife

DavidVorick · 2018-04-25T18:59:28Z

modules/renter/hostdb/hostweight.go

@@ -366,6 +362,22 @@ func (hdb *HostDB) uptimeAdjustments(entry modules.HostDBEntry) float64 {
 	return math.Pow(uptimeRatio, exp)
 }

+// decayHostUpOrDowntime decays a host's historic uptime or historic downtime.
+// It also adds the new block height to the historic uptime or historic downtime.
+func decayUptimeOrDowntime(entry *modules.HostDBEntry, scan modules.HostDBScan, recentScan modules.HostDBScan) {


functions that don't have any receiver should be moved to the top of the file, and then sorted alphabetically among eachother.

also, I would just name this function decayUptime, since we consider the cluster of uptime fields collectively to be the uptime fields.

as pointed out above, this function needs to be moved to scan.go

DavidVorick · 2018-04-25T19:04:50Z

cmd/siac/hostdbcmd.go

@@ -2,6 +2,9 @@ package main

 import (
 	"fmt"
+	"github.com/NebulousLabs/Sia/modules"
+	"github.com/NebulousLabs/Sia/node/api"
+	"github.com/spf13/cobra"


were these added by mistake? There are no other changes to this file.

DavidVorick · 2018-04-25T19:04:54Z

cmd/siac/hostcmd.go

@@ -13,6 +13,7 @@ import (
 	"github.com/NebulousLabs/Sia/node/api/client"
 	"github.com/NebulousLabs/Sia/types"

+	"github.com/NebulousLabs/Sia/node/api"


were these added by mistake? There are no other changes to this file.

DavidVorick · 2018-04-25T20:05:51Z

modules/renter/hostdb/hostweight.go

-		recentTime = scan.Timestamp
-		recentSuccess = scan.Success
+
+		decayUptimeOrDowntime(&entry, scan, recentScan)


This is the wrong place to apply decay - you are applying decay every time that the weight is being calculated. But we can calculate the weight for a lot of different reasons. We should only be applying decay after we scan the host. That means the decay algorithm should also be moved to scan.go.

DavidVorick · 2018-04-25T20:15:28Z

modules/renter/hostdb/hostweight.go

+	} else {
+		entry.HistoricDowntime *= decay
+		entry.HistoricDowntime += timePassed * decay
+	}


you need to decay both the historic uptime and the historic downtime any time that you apply a decay. The decayed time passed should be added only to the relevant field. The result should look something like this:

entry.HistoricUptime *= decay entry.HistoricDowntime *= decay if recentScan.Success { entry.HistoricUptime += timePassed*decay } else { entry.HistoricDowntime += timePassed*decay }

DavidVorick · 2018-04-25T20:18:12Z

modules/renter/hostdb/scan.go

-		} else {
-			newEntry.HistoricDowntime += timePassed
-		}
+		decayUptimeOrDowntime(&newEntry, newEntry.ScanHistory[1], newEntry.ScanHistory[0])


Upon further reflection (sorry for waffling around), the name should really be updateUptime, because you aren't just applying a decay you are also adding new information.

mikkeljuhl · 2018-04-27T06:49:28Z

@DavidVorick thanks for reviewing, your comments have been addressed.

… weight

tbenz9 · 2018-05-01T18:17:06Z

@mikkeljuhl Looks like a few of the regression tests are failing. Do the tests pass when run locally?

mikkeljuhl force-pushed the feature/uptime branch from b96b0fc to 6a3d643 Compare March 28, 2018 14:41

ChrisSchinnerl reviewed Mar 28, 2018

View reviewed changes

ChrisSchinnerl requested a review from DavidVorick March 28, 2018 15:18

mikkeljuhl force-pushed the feature/uptime branch 2 times, most recently from 36759d6 to d21257b Compare April 3, 2018 18:25

DavidVorick reviewed Apr 25, 2018

View reviewed changes

mikkeljuhl force-pushed the feature/uptime branch from 19b6f01 to 1c6c8d5 Compare April 27, 2018 06:40

Use BlockHeight to calculate uptime/downtime instead of timestamps.

f2a7e4d

mikkeljuhl force-pushed the feature/uptime branch from 1c6c8d5 to c3baff7 Compare April 27, 2018 06:47

Move updateUptime to scan.go; don't apply decay when calculating host…

23024af

… weight

mikkeljuhl force-pushed the feature/uptime branch from c3baff7 to 23024af Compare April 27, 2018 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use BlockHeight to calculate uptime/downtime instead of timestamps. #2908

Use BlockHeight to calculate uptime/downtime instead of timestamps. #2908

mikkeljuhl commented Mar 28, 2018 •

edited

ChrisSchinnerl left a comment

ChrisSchinnerl Mar 28, 2018

DavidVorick Apr 25, 2018

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

ChrisSchinnerl Mar 28, 2018

mikkeljuhl Apr 3, 2018

ChrisSchinnerl Mar 28, 2018

mikkeljuhl commented Apr 3, 2018 •

edited

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

DavidVorick Apr 25, 2018

mikkeljuhl commented Apr 27, 2018

tbenz9 commented May 1, 2018

Use BlockHeight to calculate uptime/downtime instead of timestamps. #2908

Are you sure you want to change the base?

Use BlockHeight to calculate uptime/downtime instead of timestamps. #2908

Conversation

mikkeljuhl commented Mar 28, 2018 • edited

ChrisSchinnerl 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

mikkeljuhl commented Apr 3, 2018 • edited

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

mikkeljuhl commented Apr 27, 2018

tbenz9 commented May 1, 2018

mikkeljuhl commented Mar 28, 2018 •

edited

mikkeljuhl commented Apr 3, 2018 •

edited