From b8c94c83dec017764a414100f78080ba5e4f982a Mon Sep 17 00:00:00 2001 From: lukechampine Date: Tue, 25 Oct 2016 09:23:04 -0400 Subject: [PATCH 1/3] remove unused renewHeight field --- modules/renter/contractor/contractor.go | 1 - modules/renter/contractor/doc.go | 12 +++++------- modules/renter/contractor/persist.go | 3 --- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/modules/renter/contractor/contractor.go b/modules/renter/contractor/contractor.go index b7d6674e34..fd6b64ba05 100644 --- a/modules/renter/contractor/contractor.go +++ b/modules/renter/contractor/contractor.go @@ -46,7 +46,6 @@ type Contractor struct { downloaders map[types.FileContractID]*hostDownloader editors map[types.FileContractID]*hostEditor lastChange modules.ConsensusChangeID - renewHeight types.BlockHeight // height at which to renew contracts renewing map[types.FileContractID]bool // prevent revising during renewal revising map[types.FileContractID]bool // prevent overlapping revisions diff --git a/modules/renter/contractor/doc.go b/modules/renter/contractor/doc.go index 1b8a11562a..0f699355f2 100644 --- a/modules/renter/contractor/doc.go +++ b/modules/renter/contractor/doc.go @@ -22,13 +22,11 @@ same storage capacity, and they should all end at the same height. Hosts are selected from the HostDB; there is no support for manually specifying hosts. Contracts are automatically renewed by the contractor at a safe threshold -before they are set to expire. The contractor maintains a renewHeight variable -that indicates when its current set of contracts will expire. When contracts -are renewed, they are renewed with the current allowance, which may differ -from the allowance that was used to form the initial contracts. In general, -this means that allowance modifications only take effect upon the next -"contract cycle" (the exception being "sufficiently greater" modifications, as -defined above). +before they are set to expire. When contracts are renewed, they are renewed +with the current allowance, which may differ from the allowance that was used +to form the initial contracts. In general, this means that allowance +modifications only take effect upon the next "contract cycle" (the exception +being "sufficiently greater" modifications, as defined above). As an example, imagine that the user first sets an allowance that will cover 10 contracts of 10 sectors each for 100 blocks. The contractor will diff --git a/modules/renter/contractor/persist.go b/modules/renter/contractor/persist.go index bb00b3980e..1e686b22ab 100644 --- a/modules/renter/contractor/persist.go +++ b/modules/renter/contractor/persist.go @@ -13,7 +13,6 @@ type contractorPersist struct { CachedRevisions []cachedRevision Contracts []modules.RenterContract LastChange modules.ConsensusChangeID - RenewHeight types.BlockHeight FinancialMetrics modules.RenterFinancialMetrics } @@ -23,7 +22,6 @@ func (c *Contractor) persistData() contractorPersist { Allowance: c.allowance, BlockHeight: c.blockHeight, LastChange: c.lastChange, - RenewHeight: c.renewHeight, FinancialMetrics: c.financialMetrics, } for _, rev := range c.cachedRevisions { @@ -51,7 +49,6 @@ func (c *Contractor) load() error { c.contracts[contract.ID] = contract } c.lastChange = data.LastChange - c.renewHeight = data.RenewHeight c.financialMetrics = data.FinancialMetrics return nil } From 3dbdad4351838ade8734ab8cc0b34d2d6097b513 Mon Sep 17 00:00:00 2001 From: lukechampine Date: Tue, 25 Oct 2016 09:51:06 -0400 Subject: [PATCH 2/3] map old ids to their renewal This prevents downloads from failing after a contract has been renewed. A more robust strategy will use the host's public key to identify contracts rather than the contract ID. --- modules/renter/contractor/contractor.go | 10 +++++++ modules/renter/contractor/contractor_test.go | 28 ++++++++++++++++++++ modules/renter/contractor/downloader.go | 1 + modules/renter/contractor/editor.go | 1 + modules/renter/contractor/persist.go | 16 ++++++++--- modules/renter/contractor/renew.go | 1 + 6 files changed, 54 insertions(+), 3 deletions(-) diff --git a/modules/renter/contractor/contractor.go b/modules/renter/contractor/contractor.go index fd6b64ba05..cf60d0e70f 100644 --- a/modules/renter/contractor/contractor.go +++ b/modules/renter/contractor/contractor.go @@ -46,6 +46,7 @@ type Contractor struct { downloaders map[types.FileContractID]*hostDownloader editors map[types.FileContractID]*hostEditor lastChange modules.ConsensusChangeID + renewedIDs map[types.FileContractID]types.FileContractID renewing map[types.FileContractID]bool // prevent revising during renewal revising map[types.FileContractID]bool // prevent overlapping revisions @@ -94,6 +95,14 @@ func (c *Contractor) Contracts() (cs []modules.RenterContract) { return } +// resolveID returns the ID of the most recent renewal of id. +func (c *Contractor) resolveID(id types.FileContractID) types.FileContractID { + if newID, ok := c.renewedIDs[id]; ok && newID != id { + return c.resolveID(newID) + } + return id +} + // New returns a new Contractor. func New(cs consensusSet, wallet walletShim, tpool transactionPool, hdb hostDB, persistDir string) (*Contractor, error) { // Check for nil inputs. @@ -137,6 +146,7 @@ func newContractor(cs consensusSet, w wallet, tp transactionPool, hdb hostDB, p contracts: make(map[types.FileContractID]modules.RenterContract), downloaders: make(map[types.FileContractID]*hostDownloader), editors: make(map[types.FileContractID]*hostEditor), + renewedIDs: make(map[types.FileContractID]types.FileContractID), renewing: make(map[types.FileContractID]bool), revising: make(map[types.FileContractID]bool), } diff --git a/modules/renter/contractor/contractor_test.go b/modules/renter/contractor/contractor_test.go index 77a4c702c2..98077d2baa 100644 --- a/modules/renter/contractor/contractor_test.go +++ b/modules/renter/contractor/contractor_test.go @@ -135,6 +135,34 @@ func TestContracts(t *testing.T) { } } +// TestResolveID tests the resolveID method. +func TestResolveID(t *testing.T) { + c := &Contractor{ + renewedIDs: map[types.FileContractID]types.FileContractID{ + {1}: {2}, + {2}: {3}, + {3}: {4}, + {5}: {6}, + }, + } + tests := []struct { + id types.FileContractID + resolved types.FileContractID + }{ + {types.FileContractID{0}, types.FileContractID{0}}, + {types.FileContractID{1}, types.FileContractID{4}}, + {types.FileContractID{2}, types.FileContractID{4}}, + {types.FileContractID{3}, types.FileContractID{4}}, + {types.FileContractID{4}, types.FileContractID{4}}, + {types.FileContractID{5}, types.FileContractID{6}}, + } + for _, test := range tests { + if r := c.resolveID(test.id); r != test.resolved { + t.Errorf("expected %v -> %v, got %v", test.id, test.resolved, r) + } + } +} + // TestAllowance tests the Allowance method. func TestAllowance(t *testing.T) { c := &Contractor{ diff --git a/modules/renter/contractor/downloader.go b/modules/renter/contractor/downloader.go index 29eaa50b6c..1357b8a66e 100644 --- a/modules/renter/contractor/downloader.go +++ b/modules/renter/contractor/downloader.go @@ -122,6 +122,7 @@ func (hd *hostDownloader) Close() error { // from a host. func (c *Contractor) Downloader(id types.FileContractID) (_ Downloader, err error) { c.mu.RLock() + id = c.resolveID(id) cachedDownloader, haveDownloader := c.downloaders[id] height := c.blockHeight contract, haveContract := c.contracts[id] diff --git a/modules/renter/contractor/editor.go b/modules/renter/contractor/editor.go index 25fc0a7612..574719a6c5 100644 --- a/modules/renter/contractor/editor.go +++ b/modules/renter/contractor/editor.go @@ -179,6 +179,7 @@ func (he *hostEditor) Modify(oldRoot, newRoot crypto.Hash, offset uint64, newDat // delete sectors on a host. func (c *Contractor) Editor(id types.FileContractID) (_ Editor, err error) { c.mu.RLock() + id = c.resolveID(id) cachedEditor, haveEditor := c.editors[id] height := c.blockHeight contract, haveContract := c.contracts[id] diff --git a/modules/renter/contractor/persist.go b/modules/renter/contractor/persist.go index 1e686b22ab..3e1c0dca21 100644 --- a/modules/renter/contractor/persist.go +++ b/modules/renter/contractor/persist.go @@ -12,8 +12,9 @@ type contractorPersist struct { BlockHeight types.BlockHeight CachedRevisions []cachedRevision Contracts []modules.RenterContract - LastChange modules.ConsensusChangeID FinancialMetrics modules.RenterFinancialMetrics + LastChange modules.ConsensusChangeID + RenewedIDs map[string]string } // persistData returns the data in the Contractor that will be saved to disk. @@ -21,8 +22,8 @@ func (c *Contractor) persistData() contractorPersist { data := contractorPersist{ Allowance: c.allowance, BlockHeight: c.blockHeight, - LastChange: c.lastChange, FinancialMetrics: c.financialMetrics, + LastChange: c.lastChange, } for _, rev := range c.cachedRevisions { data.CachedRevisions = append(data.CachedRevisions, rev) @@ -30,6 +31,9 @@ func (c *Contractor) persistData() contractorPersist { for _, contract := range c.contracts { data.Contracts = append(data.Contracts, contract) } + for oldID, newID := range c.renewedIDs { + data.RenewedIDs[oldID.String()] = newID.String() + } return data } @@ -48,8 +52,14 @@ func (c *Contractor) load() error { for _, contract := range data.Contracts { c.contracts[contract.ID] = contract } - c.lastChange = data.LastChange c.financialMetrics = data.FinancialMetrics + c.lastChange = data.LastChange + for oldString, newString := range data.RenewedIDs { + var oldHash, newHash crypto.Hash + oldHash.LoadString(oldString) + newHash.LoadString(newString) + c.renewedIDs[types.FileContractID(oldHash)] = types.FileContractID(newHash) + } return nil } diff --git a/modules/renter/contractor/renew.go b/modules/renter/contractor/renew.go index c191567e51..0b454019b8 100644 --- a/modules/renter/contractor/renew.go +++ b/modules/renter/contractor/renew.go @@ -145,6 +145,7 @@ func (c *Contractor) managedRenewContracts() error { for id, contract := range newContracts { delete(c.contracts, id) c.contracts[contract.ID] = contract + c.renewedIDs[id] = contract.ID } err = c.saveSync() c.mu.Unlock() From 3fc2c9f5e98426c9177a36921f6dbbeacc494244 Mon Sep 17 00:00:00 2001 From: lukechampine Date: Tue, 25 Oct 2016 10:07:37 -0400 Subject: [PATCH 3/3] fix nil map in persistData --- modules/renter/contractor/persist.go | 1 + modules/renter/contractor/persist_test.go | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/modules/renter/contractor/persist.go b/modules/renter/contractor/persist.go index 3e1c0dca21..ae6ddd74f6 100644 --- a/modules/renter/contractor/persist.go +++ b/modules/renter/contractor/persist.go @@ -24,6 +24,7 @@ func (c *Contractor) persistData() contractorPersist { BlockHeight: c.blockHeight, FinancialMetrics: c.financialMetrics, LastChange: c.lastChange, + RenewedIDs: make(map[string]string), } for _, rev := range c.cachedRevisions { data.CachedRevisions = append(data.CachedRevisions, rev) diff --git a/modules/renter/contractor/persist_test.go b/modules/renter/contractor/persist_test.go index 9cab33b092..15ba43e15e 100644 --- a/modules/renter/contractor/persist_test.go +++ b/modules/renter/contractor/persist_test.go @@ -20,7 +20,8 @@ func (m memPersist) load(data *contractorPersist) error { *data = contractor func TestSaveLoad(t *testing.T) { // create contractor with mocked persist dependency c := &Contractor{ - contracts: make(map[types.FileContractID]modules.RenterContract), + contracts: make(map[types.FileContractID]modules.RenterContract), + renewedIDs: make(map[types.FileContractID]types.FileContractID), } c.persist = new(memPersist) @@ -30,6 +31,12 @@ func TestSaveLoad(t *testing.T) { {1}: {NetAddress: "bar"}, {2}: {NetAddress: "baz"}, } + c.renewedIDs = map[types.FileContractID]types.FileContractID{ + {0}: {1}, + {1}: {2}, + {2}: {3}, + } + // save and reload err := c.save() if err != nil { @@ -46,6 +53,12 @@ func TestSaveLoad(t *testing.T) { if !ok0 || !ok1 || !ok2 { t.Fatal("contracts were not restored properly:", c.contracts) } + _, ok0 = c.renewedIDs[types.FileContractID{0}] + _, ok1 = c.renewedIDs[types.FileContractID{1}] + _, ok2 = c.renewedIDs[types.FileContractID{2}] + if !ok0 || !ok1 || !ok2 { + t.Fatal("renewed IDs were not restored properly:", c.renewedIDs) + } // use stdPersist instead of mock c.persist = newPersist(build.TempDir("contractor", "TestSaveLoad")) @@ -67,4 +80,10 @@ func TestSaveLoad(t *testing.T) { if !ok0 || !ok1 || !ok2 { t.Fatal("contracts were not restored properly:", c.contracts) } + _, ok0 = c.renewedIDs[types.FileContractID{0}] + _, ok1 = c.renewedIDs[types.FileContractID{1}] + _, ok2 = c.renewedIDs[types.FileContractID{2}] + if !ok0 || !ok1 || !ok2 { + t.Fatal("renewed IDs were not restored properly:", c.renewedIDs) + } }