migrate `hostdb` to the from `weightedlist` to the new `hosttree` package #1525

avahowell · 2016-12-20T00:19:16Z

This PR removes weightedlist from the hostdb and replaces it with the new hosttree package. Hosts are still stored in the activeHosts map by the NetAddress, since changing fully to public keys requires changing the HostDB interface, which is a much larger change. A subsequent PR will be opened, likely after the renter pools branch is merged, which removes activeHosts and changes the HostDB to use public keys.

avahowell · 2016-12-20T03:07:57Z

Some contractor testing had to change because of these changes, since the hostTree stores hosts by publicKey it is no longer sufficient to simply re-announce the same host on a different netaddress. I've changed newTestingTrio to take a number of hosts to create, nhosts, and return a slice of hosts (with unique keypairs, obviously.)

DavidVorick · 2017-01-04T01:29:02Z

modules/renter.go

@@ -141,6 +141,8 @@ type HostDBEntry struct {
 	// ScanHistory is the set of scans performed on the host. It should always
 	// be ordered according to the scan's Timestamp, oldest to newest.
 	ScanHistory HostDBScans


I know that this is not from this PR, but I think here is bad form, should just be []hdbs

The hdbss type is purely for sorting.

yeah, my bad. Typically the pattern is to name the sorting type ByX. So we'd write:

sort.Sort(modules.ByTimestamp(entry.ScanHistory))

The name becomes pretty ugly though, since ByTimestamp is ambiguous -- it ought to be HostDBScansByTimestamp or something. Alternatively you could define the byTimestamp type locally, but then you'd need to redefine it in multiple modules.

btw, in Go 1.8 we can do:

sort.Slice(entry.ScanHistory, func(i, j int) bool { return entry.ScanHistory[i].Timestamp.Before(entry.ScanHistory[j].Timestamp) })

yeah that's what we should probably end up doing.

DavidVorick · 2017-01-04T01:30:33Z

modules/renter/contractor/host_integration_test.go

-func newTestingTrio(name string) (modules.Host, *Contractor, modules.TestMiner, error) {
+// newTestingTrio a Contractor, TestMiner, and a slice of hosts that can be
+// used for testing host/renter interactions.
+func newTestingTrio(name string, nhosts int) ([]modules.Host, *Contractor, modules.TestMiner, error) {


@lukechampine what are your thoughts on this change?

I don't really like it; it's not a trio if there's multiple hosts. AFAICT, the only place we create multiple hosts is TestIntegrationSetAllowance, where we create 2 instead of 1. So I would prefer to revert the change, and instead either create the extra host manually or add a new helper function for creating multiple hosts.

I agree that this change is ugly, I'll revert it and create an extra host in TestIntegrationSetAllowance.

DavidVorick · 2017-01-09T23:45:00Z

modules/renter/hostdb/hostentry_test.go

-	if len(hdb.activeHosts) != 2 {
-		t.Error("host was not added:", hdb.activeHosts)
-	}
+	hdb.hostTree.Insert(h2)


^_^ very nice

DavidVorick · 2017-01-09T23:47:13Z

modules/renter/hostdb/hostweight.go

-	siafundFee := adjustedContractPrice.Add(adjustedUploadPrice).Add(adjustedDownloadPrice).Add(entry.Collateral).MulTax()
-	totalPrice := entry.StoragePrice.Add(adjustedContractPrice).Add(adjustedUploadPrice).Add(adjustedDownloadPrice).Add(siafundFee)
+func (hdb *HostDB) calculateHostWeight() hosttree.WeightFunc {
+	return func(entry modules.HostDBEntry) types.Currency {


I don't like the tabbing here, would prefer to see the weight func defined separately, then return weightFunc or whatever. I don't think it makes sense to be inline here, it's a massive amount of code.

DavidVorick · 2017-01-09T23:52:15Z

modules/renter/hostdb/hostweight.go

+			}
+		} else {
+			// Shouldn't happen, but the usecase is covered anyway.
+			weight = weight.Div64(1000) // Because something weird is happening, don't trust this host very much.


Should probably be 10e3, not 1e3. Should definitely use e notation since it's got >= three zeros

DavidVorick · 2017-01-09T23:52:34Z

modules/renter/hostdb/hostweight.go

+			weight = weight.Div64(10) // 5,700x total penalty
+		}
+		if entry.RemainingStorage < requiredStorage {
+			weight = weight.Div64(100) // 570,000 total penalty


missing an x lol

DavidVorick · 2017-01-10T00:00:31Z

modules/renter/hostdb/hostdb.go

 		allHosts:    make(map[modules.NetAddress]*hostEntry),
 		scanPool:    make(chan *hostEntry, scanPoolSize),
 	}

+	hdb.hostTree = hosttree.New(hdb.calculateHostWeight())


shouldn't you be able to just specify hdb.calculateHostWeight, instead of having a function that returns a function? You can just define the function and then specify it here.

The reason for this is that the host weight function needs access to the hostdb's state.

@DavidVorick is right -- you can just pass hdb.calculateHostWeight if you modify it to satisfy the weightFunc type. It should be:

func (hdb *HostDB) calculateHostWeight(entry modules.HostDBEntry) types.Currency { // use hdb.blockheight internally }

ah yeah, that's a lot cleaner.

DavidVorick · 2017-01-10T00:02:39Z

modules/renter/hostdb/persist.go

-	for _, node := range hdb.activeHosts {
-		data.ActiveHosts = append(data.ActiveHosts, *node.hostEntry)
+	for _, entry := range hdb.activeHosts {
+		data.ActiveHosts = append(data.ActiveHosts, *entry)


just want to confirm that this saves everything important.

Can you verify that there is a test for saving and loading hosts, including things like the scan history?

yes, I've confirmed that TestSaveLoad fails after modifying these lines.

DavidVorick · 2017-01-10T00:11:30Z

modules/renter/hostdb/scan.go

@@ -107,10 +107,10 @@ func (hdb *HostDB) decrementReliability(addr modules.NetAddress, penalty types.C

 	// If the entry is in the active database, remove it from the active
 	// database.
-	node, exists := hdb.activeHosts[addr]
+	existingEntry, exists := hdb.activeHosts[addr]


so the entries still have the reliability cruft, instead of using the scan data?

yeah, ideally activeHosts would not exist but that requires a much larger change.

TestIntegrationSetAllowance

avahowell added the in progress label Dec 20, 2016

DavidVorick reviewed Jan 4, 2017

View reviewed changes

DavidVorick reviewed Jan 9, 2017

View reviewed changes

DavidVorick reviewed Jan 10, 2017

View reviewed changes

avahowell force-pushed the hosttree-implementation branch 2 times, most recently from f4415c3 to f3ab380 Compare January 10, 2017 04:52

avahowell added 13 commits January 9, 2017 23:57

migrate the hostdb to hosttree

936cde7

add RandomHosts docstrings, debug logging

6446ff0

add HostDBEntry.FirstSeen

3bfbee6

use HostDBEntry.FirstSeen instead of hostEntry.FirstSeen

e6e83df

fix and reactivate TestActiveHosts

0842f48

remove unused hostEntry.FirstSeen field

f6a5137

log.Severe hosttree selection errors, remove deprecated tree testing

170a894

add TestRandomHosts

68bcc42

change contractor tests to create and announce a slice of unique hosts

a1a34f7

revert changes to newTestingTrio, create extra host in

122d8bf

TestIntegrationSetAllowance

change calculateHostWeight to be a hostdb method, not return a func

633a4b1

handle hosttree.Insert error in scan.go

b474122

make branch in scan.go clearer

3298e66

avahowell force-pushed the hosttree-implementation branch from f3ab380 to 3298e66 Compare January 10, 2017 06:47

DavidVorick merged commit 9423a61 into master Jan 10, 2017

DavidVorick removed the in progress label Jan 10, 2017

lukechampine deleted the hosttree-implementation branch January 10, 2017 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate `hostdb` to the from `weightedlist` to the new `hosttree` package #1525

migrate `hostdb` to the from `weightedlist` to the new `hosttree` package #1525

avahowell commented Dec 20, 2016

avahowell commented Dec 20, 2016

DavidVorick Jan 4, 2017

lukechampine Jan 4, 2017 •

edited

lukechampine Jan 4, 2017

DavidVorick Jan 9, 2017

DavidVorick Jan 4, 2017

lukechampine Jan 4, 2017

avahowell Jan 4, 2017

DavidVorick Jan 9, 2017

DavidVorick Jan 9, 2017

DavidVorick Jan 9, 2017 •

edited

DavidVorick Jan 9, 2017

DavidVorick Jan 10, 2017

avahowell Jan 10, 2017

lukechampine Jan 10, 2017

lukechampine Jan 10, 2017

avahowell Jan 10, 2017

DavidVorick Jan 10, 2017

avahowell Jan 10, 2017

DavidVorick Jan 10, 2017

avahowell Jan 10, 2017

migrate hostdb to the from weightedlist to the new hosttree package #1525

migrate hostdb to the from weightedlist to the new hosttree package #1525

Conversation

avahowell commented Dec 20, 2016

avahowell commented Dec 20, 2016

Choose a reason for hiding this comment

lukechampine Jan 4, 2017 • 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

DavidVorick Jan 9, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migrate `hostdb` to the from `weightedlist` to the new `hosttree` package #1525

migrate `hostdb` to the from `weightedlist` to the new `hosttree` package #1525

lukechampine Jan 4, 2017 •

edited

DavidVorick Jan 9, 2017 •

edited