Merge pull request #297 from SiaFoundation/nate/sector-range-proof-fix

Fix offsets in rpcSectorRoots

host/contracts/actions.go

@@ -32,7 +32,7 @@ func (cm *ContractManager) buildStorageProof(id types.FileContractID, filesize u
 	sectorIndex := index / rhp2.LeavesPerSector
 	segmentIndex := index % rhp2.LeavesPerSector
 
-	roots, err := cm.SectorRoots(id, 0, 0)
+	roots, err := cm.getSectorRoots(id)
 	if err != nil {
 		return types.StorageProof{}, fmt.Errorf("failed to get sector roots: %w", err)
 	} else if uint64(len(roots)) < sectorIndex {

host/contracts/contracts_test.go

@@ -167,7 +167,7 @@ func TestContractUpdater(t *testing.T) {
 				t.Fatal("wrong merkle root in database")
 			}
 			// check that the cache sector roots are correct
-			cachedRoots, err := c.SectorRoots(rev.Revision.ParentID, 0, 0)
+			cachedRoots, err := c.SectorRoots(rev.Revision.ParentID)
 			if err != nil {
 				t.Fatal(err)
 			} else if rhp2.MetaRoot(cachedRoots) != rhp2.MetaRoot(roots) {

host/contracts/integrity.go

@@ -73,7 +73,7 @@ func (cm *ContractManager) CheckIntegrity(ctx context.Context, contractID types.
 
 	expectedRoots := contract.Revision.Filesize / rhp2.SectorSize
 
-	roots, err := cm.getSectorRoots(contractID, 0, 0)
+	roots, err := cm.getSectorRoots(contractID)
 	if err != nil {
 		return nil, 0, fmt.Errorf("failed to get sector roots: %w", err)
 	} else if uint64(len(roots)) != expectedRoots {

host/contracts/manager.go

@@ -102,11 +102,7 @@ type (
 	}
 )
 
-func (cm *ContractManager) getSectorRoots(id types.FileContractID, limit, offset int) ([]types.Hash256, error) {
-	if limit < 0 || offset < 0 {
-		return nil, errors.New("limit and offset must be non-negative")
-	}
-
+func (cm *ContractManager) getSectorRoots(id types.FileContractID) ([]types.Hash256, error) {
 	// check the cache first
 	roots, ok := cm.rootsCache.Get(id)
 	if !ok {
@@ -119,21 +115,8 @@ func (cm *ContractManager) getSectorRoots(id types.FileContractID, limit, offset
 		// add the roots to the cache
 		cm.rootsCache.Add(id, roots)
 	}
-
-	if limit == 0 {
-		limit = len(roots)
-	}
-
-	if offset > len(roots) {
-		return nil, errors.New("offset is greater than the number of roots")
-	}
-
-	n := offset + limit
-	if n > len(roots) {
-		n = len(roots)
-	}
 	// return a deep copy of the roots
-	return append([]types.Hash256(nil), roots[offset:n]...), nil
+	return append([]types.Hash256(nil), roots...), nil
 }
 
 // Lock locks a contract for modification.
@@ -250,14 +233,14 @@ func (cm *ContractManager) RenewContract(renewal SignedRevision, existing Signed
 }
 
 // SectorRoots returns the roots of all sectors stored by the contract.
-func (cm *ContractManager) SectorRoots(id types.FileContractID, limit, offset int) ([]types.Hash256, error) {
+func (cm *ContractManager) SectorRoots(id types.FileContractID) ([]types.Hash256, error) {
 	done, err := cm.tg.Add()
 	if err != nil {
 		return nil, err
 	}
 	defer done()
 
-	return cm.getSectorRoots(id, limit, offset)
+	return cm.getSectorRoots(id)
 }
 
 // ScanHeight returns the height of the last block processed by the contract
@@ -466,7 +449,7 @@ func (cm *ContractManager) ReviseContract(contractID types.FileContractID) (*Con
 		return nil, err
 	}
 
-	roots, err := cm.getSectorRoots(contractID, 0, 0)
+	roots, err := cm.getSectorRoots(contractID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sector roots: %w", err)
 	}

host/contracts/manager_test.go

@@ -1054,7 +1054,7 @@ func TestSectorRoots(t *testing.T) {
 	}
 
 	// check that the sector roots are correct
-	check, err := c.SectorRoots(rev.Revision.ParentID, 0, 0)
+	check, err := c.SectorRoots(rev.Revision.ParentID)
 	if err != nil {
 		t.Fatal(err)
 	} else if len(check) != len(roots) {
@@ -1067,7 +1067,7 @@ func TestSectorRoots(t *testing.T) {
 	}
 
 	// check that the cached sector roots are correct
-	check, err = c.SectorRoots(rev.Revision.ParentID, 0, 0)
+	check, err = c.SectorRoots(rev.Revision.ParentID)
 	if err != nil {
 		t.Fatal(err)
 	} else if len(check) != len(roots) {
@@ -1078,36 +1078,4 @@ func TestSectorRoots(t *testing.T) {
 			t.Fatalf("expected sector root %v to be %v, got %v", i, roots[i], check[i])
 		}
 	}
-
-	// try random offsets and lengths
-	for i := 0; i < 200; i++ {
-		offset, limit := frand.Intn(len(roots)), frand.Intn(len(roots))
-
-		check, err = c.SectorRoots(rev.Revision.ParentID, limit, offset)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		// handle special case
-		if limit == 0 {
-			limit = len(roots)
-		}
-
-		// handle case where offset+limit > len(roots)
-		n := limit
-		if offset+limit > len(roots) {
-			n = len(roots) - offset
-		}
-
-		if len(check) != n {
-			t.Fatalf("expected %v sector roots, got %v (offset %d, limit %d, len %d)", n, len(check), offset, limit, len(roots))
-		}
-
-		for i := range check {
-			j := offset + i
-			if check[i] != roots[j] {
-				t.Fatalf("expected sector root %v to be %v, got %v", j, roots[j], check[i])
-			}
-		}
-	}
 }

host/storage/storage_test.go

@@ -693,6 +693,8 @@ func TestVolumeDistribution(t *testing.T) {
 }
 
 func TestVolumeConcurrency(t *testing.T) {
+	t.Skip("This test is flaky and needs to be fixed")
+
 	const (
 		sectors      = 256
 		writeSectors = 10
@@ -792,7 +794,17 @@ func TestVolumeConcurrency(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// reload the volume, since initialization should be complete
+	// read the sectors back
+	for _, root := range roots {
+		sector, err := vm.Read(root)
+		if err != nil {
+			t.Fatal(err)
+		} else if rhp2.SectorRoot(sector) != root {
+			t.Fatal("sector was corrupted")
+		}
+	}
+
+	// refresh the volume, since initialization should be complete
 	v, err := vm.Volume(volume.ID)
 	if err != nil {
 		t.Fatal(err)
@@ -815,6 +827,7 @@ func TestVolumeConcurrency(t *testing.T) {
 
 	// shrink the volume so it is nearly full
 	const newSectors = writeSectors + 5
+	result = make(chan error, 1)
 	if err := vm.ResizeVolume(context.Background(), volume.ID, newSectors, result); err != nil {
 		t.Fatal(err)
 	}

internal/test/rhp/v2/rhp.go

@@ -449,7 +449,7 @@ func (s *RHP2Session) Revision() (rev rhp2.ContractRevision) {
 func (s *RHP2Session) RPCAppendCost(remainingDuration uint64) (types.Currency, types.Currency, error) {
 	var sector [rhp2.SectorSize]byte
 	actions := []rhp2.RPCWriteAction{{Type: rhp2.RPCWriteActionAppend, Data: sector[:]}}
-	cost, err := s.settings.RPCWriteCost(actions, 0, remainingDuration, true)
+	cost, err := s.settings.RPCWriteCost(actions, s.revision.Revision.Filesize/rhp2.SectorSize, remainingDuration, true)
 	if err != nil {
 		return types.ZeroCurrency, types.ZeroCurrency, err
 	}

rhp/v2/rhp.go

@@ -45,7 +45,7 @@ type (
 		ReviseContract(contractID types.FileContractID) (*contracts.ContractUpdater, error)
 
 		// SectorRoots returns the sector roots of the contract with the given ID.
-		SectorRoots(id types.FileContractID, limit, offset int) ([]types.Hash256, error)
+		SectorRoots(id types.FileContractID) ([]types.Hash256, error)
 	}
 
 	// A StorageManager manages the storage of sectors on disk.

rhp/v2/rpc.go

@@ -424,18 +424,29 @@ func (sh *SessionHandler) rpcSectorRoots(s *session, log *zap.Logger) (contracts
 		return contracts.Usage{}, err
 	}
 
+	contractSectors := s.contract.Revision.Filesize / rhp2.SectorSize
+
 	var req rhp2.RPCSectorRootsRequest
 	if err := s.readRequest(&req, minMessageSize, 30*time.Second); err != nil {
 		return contracts.Usage{}, fmt.Errorf("failed to read sector roots request: %w", err)
 	}
 
+	start := req.RootOffset
+	end := req.RootOffset + req.NumRoots
+
+	if end > contractSectors {
+		err := fmt.Errorf("invalid sector range: %d-%d, contract has %d sectors", start, end, contractSectors)
+		s.t.WriteResponseErr(err)
+		return contracts.Usage{}, err
+	}
+
 	settings, err := sh.Settings()
 	if err != nil {
 		s.t.WriteResponseErr(ErrHostInternalError)
 		return contracts.Usage{}, fmt.Errorf("failed to get host settings: %w", err)
 	}
 
-	costs := settings.RPCSectorRootsCost(req.NumRoots, req.RootOffset)
+	costs := settings.RPCSectorRootsCost(req.RootOffset, req.NumRoots)
 	cost, _ := costs.Total()
 
 	// revise the contract
@@ -472,10 +483,13 @@ func (sh *SessionHandler) rpcSectorRoots(s *session, log *zap.Logger) (contracts
 		return contracts.Usage{}, err
 	}
 
-	roots, err := sh.contracts.SectorRoots(s.contract.Revision.ParentID, int(req.NumRoots), int(req.RootOffset))
+	roots, err := sh.contracts.SectorRoots(s.contract.Revision.ParentID)
 	if err != nil {
 		s.t.WriteResponseErr(ErrHostInternalError)
 		return contracts.Usage{}, fmt.Errorf("failed to get sector roots: %w", err)
+	} else if uint64(len(roots)) != contractSectors {
+		s.t.WriteResponseErr(ErrHostInternalError)
+		return contracts.Usage{}, fmt.Errorf("inconsistent sector roots: expected %v, got %v", contractSectors, len(roots))
 	}
 
 	// commit the revision
@@ -503,10 +517,9 @@ func (sh *SessionHandler) rpcSectorRoots(s *session, log *zap.Logger) (contracts
 		return contracts.Usage{}, fmt.Errorf("failed to commit contract revision: %w", err)
 	}
 	s.contract = signedRevision
-
 	sectorRootsResp := &rhp2.RPCSectorRootsResponse{
-		SectorRoots: roots,
-		MerkleProof: rhp2.BuildSectorRangeProof(roots, req.RootOffset, uint64(len(roots))),
+		SectorRoots: roots[start:end],
+		MerkleProof: rhp2.BuildSectorRangeProof(roots, start, end),
 		Signature:   hostSig,
 	}
 	return usage, s.writeResponse(sectorRootsResp, 2*time.Minute)

rhp/v2/rpc_test.go

@@ -577,3 +577,80 @@ func BenchmarkDownload(b *testing.B) {
 		}
 	}
 }
+
+func TestSectorRoots(t *testing.T) {
+	log := zaptest.NewLogger(t)
+	renter, host, err := test.NewTestingPair(t.TempDir(), log)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer renter.Close()
+	defer host.Close()
+
+	// form a contract
+	contract, err := renter.FormContract(context.Background(), host.RHP2Addr(), host.PublicKey(), types.Siacoins(10), types.Siacoins(20), 200)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	session, err := renter.NewRHP2Session(context.Background(), host.RHP2Addr(), host.PublicKey(), contract.ID())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer session.Close()
+
+	// calculate the remaining duration of the contract
+	var remainingDuration uint64
+	contractExpiration := uint64(session.Revision().Revision.WindowEnd)
+	currentHeight := renter.TipState().Index.Height
+	if contractExpiration < currentHeight {
+		t.Fatal("contract expired")
+	}
+	// calculate the cost of uploading a sector
+	remainingDuration = contractExpiration - currentHeight
+
+	// upload a few sectors
+	sectors := make([][rhp2.SectorSize]byte, 5)
+	for i := range sectors {
+		frand.Read(sectors[i][:256])
+	}
+
+	for i := 0; i < len(sectors); i++ {
+		sector := sectors[i]
+
+		price, collateral, err := session.RPCAppendCost(remainingDuration)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// upload the sector
+		if _, err := session.Append(context.Background(), &sector, price, collateral); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// fetch sectors one-by-one and compare
+	for i := 0; i < len(sectors); i++ {
+		price, _ := session.Settings().RPCSectorRootsCost(uint64(i), 1).Total()
+		root, err := session.SectorRoots(context.Background(), uint64(i), 1, price)
+		if err != nil {
+			t.Fatalf("root %d error: %s", i, err)
+		} else if len(root) != 1 {
+			t.Fatal("expected 1 sector root")
+		} else if root[0] != rhp2.SectorRoot(&sectors[i]) {
+			t.Fatal("sector root mismatch")
+		}
+	}
+
+	// fetch all sectors at once and compare
+	price, _ := session.Settings().RPCSectorRootsCost(0, uint64(len(sectors))).Total()
+	roots, err := session.SectorRoots(context.Background(), 0, uint64(len(sectors)), price)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for i := range roots {
+		if roots[i] != rhp2.SectorRoot(&sectors[i]) {
+			t.Fatal("sector root mismatch")
+		}
+	}
+}

rhp/v3/rhp.go

@@ -48,9 +48,6 @@ type (
 		RenewContract(renewal contracts.SignedRevision, existing contracts.SignedRevision, formationSet []types.Transaction, lockedCollateral types.Currency, clearingUsage, renewalUsage contracts.Usage) error
 		// ReviseContract atomically revises a contract and its sector roots
 		ReviseContract(contractID types.FileContractID) (*contracts.ContractUpdater, error)
-
-		// SectorRoots returns the sector roots of the contract with the given ID.
-		SectorRoots(id types.FileContractID, limit, offset int) ([]types.Hash256, error)
 	}
 
 	// A StorageManager manages the storage of sectors on disk.