Fix RPC free sectors edge case

.changeset/fix_an_edge_case_in_rpcfreesectors_where_the_swap_and_truncate_deletion_logic_could_preserve_a_sector_root_that_was_supposed_to_be_deleted.md

@@ -0,0 +1,5 @@
+---
+default: patch
+---
+
+# Fixed an edge case in `RPCFreeSectors` where the deletion logic would keep a sector root that was supposed to be deleted.

rhp/v4/rpc.go

@@ -2,11 +2,13 @@ package rhp
 
 import (
 	"bufio"
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
 	"io"
 	"net"
+	"slices"
 	"time"
 
 	"go.sia.tech/core/consensus"
@@ -550,6 +552,17 @@ func RPCVerifySector(ctx context.Context, t TransportClient, prices rhp4.HostPri
 
 // RPCFreeSectors removes sectors from a contract.
 func RPCFreeSectors(ctx context.Context, t TransportClient, signer ContractSigner, cs consensus.State, prices rhp4.HostPrices, contract ContractRevision, indices []uint64) (RPCFreeSectorsResult, error) {
+	// sort indices descending and remove duplicates to avoid swapping a
+	// root with one that should be kept or is pending deletion.
+	//
+	// note: we should probably add this normalization to the server as well,
+	// but that would break backwards compatibility with existing hosts.
+	indices = slices.Clone(indices) // copy to avoid mutating caller's slice
+	slices.SortFunc(indices, func(a, b uint64) int {
+		return cmp.Compare(b, a) // descending
+	})
+	indices = slices.Compact(indices)
+
 	req := rhp4.RPCFreeSectorsRequest{
 		ContractID: contract.ID,
 		Prices:     prices,

rhp/v4/rpc_test.go

@@ -2,9 +2,11 @@ package rhp_test
 
 import (
 	"bytes"
+	"cmp"
 	"context"
 	"crypto/tls"
 	"fmt"
+	"maps"
 	"math"
 	"net"
 	"reflect"
@@ -263,6 +265,33 @@ func assertValidRevision(t *testing.T, cm rhp4.ChainManager, c rhp4.Contractor,
 	}
 }
 
+// assertSectorsFreed checks if the indices in deletedIndices
+// correspond to roots in the roots slice that have been freed.
+//
+// It returns the new slice of roots with the sectors removed.
+func assertSectorsFreed(tb testing.TB, revision types.V2FileContract, roots []types.Hash256, deletedIndices []uint64) []types.Hash256 {
+	tb.Helper()
+
+	deletedIndices = slices.Clone(deletedIndices)
+	slices.SortFunc(deletedIndices, func(a, b uint64) int {
+		return cmp.Compare(b, a) // desc
+	})
+	deletedIndices = slices.Compact(deletedIndices)
+
+	roots = slices.Clone(roots)
+	for i, n := range deletedIndices {
+		roots[n] = roots[len(roots)-i-1]
+	}
+	roots = roots[:len(roots)-len(deletedIndices)]
+
+	if expectedRoot := proto4.MetaRoot(roots); expectedRoot != revision.FileMerkleRoot {
+		tb.Fatalf("unexpected new merkle root: expected %v, got %v", revision.FileMerkleRoot, expectedRoot)
+	} else if revision.Filesize != uint64(len(roots))*proto4.SectorSize {
+		tb.Fatalf("unexpected new filesize: expected %v, got %v", uint64(len(roots))*proto4.SectorSize, revision.Filesize)
+	}
+	return roots
+}
+
 func TestFormContract(t *testing.T) {
 	n, genesis := testutil.V2Network()
 	hostKey, renterKey := types.GeneratePrivateKey(), types.GeneratePrivateKey()
@@ -2180,11 +2209,6 @@ func TestRPCFreeSectors(t *testing.T) {
 	for i, n := range frand.Perm(len(roots))[:len(roots)/2] {
 		indices[i] = uint64(n)
 	}
-	newRoots := append([]types.Hash256(nil), roots...)
-	for i, n := range indices {
-		newRoots[n] = newRoots[len(newRoots)-i-1]
-	}
-	newRoots = newRoots[:len(newRoots)-len(indices)]
 
 	// assert manipulating the signature returns [proto4.ErrInvalidSignature]
 	corrupted := revision
@@ -2198,8 +2222,157 @@ func TestRPCFreeSectors(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	expectedRoots := assertSectorsFreed(t, removeResult.Revision, roots, indices)
 	revision.Revision = removeResult.Revision
-	assertRevision(t, revision, newRoots)
+	assertRevision(t, revision, expectedRoots)
+}
+
+func TestFreeSectorsSwapConsistency(t *testing.T) {
+	n, genesis := testutil.V2Network()
+	hostKey, renterKey := types.GeneratePrivateKey(), types.GeneratePrivateKey()
+
+	cm, w := startTestNode(t, n, genesis)
+
+	// fund the wallet
+	mineAndSync(t, cm, w.Address(), int(n.MaturityDelay+20), w)
+
+	sr := testutil.NewEphemeralSettingsReporter()
+	sr.Update(proto4.HostSettings{
+		Release:             "test",
+		AcceptingContracts:  true,
+		WalletAddress:       w.Address(),
+		MaxCollateral:       types.Siacoins(10000),
+		MaxContractDuration: 1000,
+		RemainingStorage:    100 * proto4.SectorSize,
+		TotalStorage:        100 * proto4.SectorSize,
+		Prices: proto4.HostPrices{
+			ContractPrice: types.Siacoins(1).Div64(5),
+			StoragePrice:  types.NewCurrency64(100),
+			IngressPrice:  types.NewCurrency64(100),
+			EgressPrice:   types.NewCurrency64(100),
+			Collateral:    types.NewCurrency64(200),
+		},
+	})
+	ss := testutil.NewEphemeralSectorStore()
+	c := testutil.NewEphemeralContractor(cm)
+
+	transport := testRenterHostPairSiaMux(t, hostKey, cm, w, c, sr, ss, zap.NewNop())
+
+	settings, err := rhp4.RPCSettings(context.Background(), transport)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fundAndSign := &fundAndSign{w, renterKey}
+	cs := cm.TipState()
+
+	tests := []struct {
+		name    string
+		n       int
+		indices []uint64
+	}{
+		// index 0 replaced by last element which is also deleted
+		{name: "swap with tail", n: 5, indices: []uint64{0, 4}},
+		// same indices reversed; highest processed first avoids the bug
+		{name: "tail then head", n: 5, indices: []uint64{4, 0}},
+		// boundary between kept and replacement zones
+		{name: "boundary overlap", n: 6, indices: []uint64{1, 4}},
+		// last two elements are replacement sources for the first
+		{name: "multiple overlaps", n: 6, indices: []uint64{0, 4, 5}},
+		// kept root at index 2 written to truncated position 3
+		{name: "kept root lost", n: 4, indices: []uint64{1, 3}},
+		// smallest possible triggering case
+		{name: "minimal", n: 3, indices: []uint64{0, 2}},
+		// mid-slice deletion, no index 0 involved
+		{name: "mid swap with tail", n: 5, indices: []uint64{2, 4}},
+		// two low indices plus one in the tail
+		{name: "two low one high", n: 5, indices: []uint64{0, 1, 4}},
+		// majority deletion with tail overlap
+		{name: "majority delete", n: 6, indices: []uint64{0, 1, 2, 5}},
+		// contiguous tail indices overlap with replacement zone
+		{name: "contiguous tail", n: 5, indices: []uint64{0, 3, 4}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			formResult, err := rhp4.RPCFormContract(context.Background(), transport, cm, fundAndSign, cs, settings.Prices, hostKey.PublicKey(), settings.WalletAddress, proto4.RPCFormContractParams{
+				RenterPublicKey: renterKey.PublicKey(),
+				RenterAddress:   w.Address(),
+				Allowance:       types.Siacoins(100),
+				Collateral:      types.Siacoins(200),
+				ProofHeight:     cm.Tip().Height + 50,
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			revision := formResult.Contract
+
+			mineAndSync(t, cm, types.VoidAddress, 1, w, c)
+			cs = cm.TipState()
+
+			account := proto4.Account(renterKey.PublicKey())
+			fundResult, err := rhp4.RPCFundAccounts(context.Background(), transport, cs, renterKey, revision, []proto4.AccountDeposit{
+				{Account: account, Amount: types.Siacoins(25)},
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			revision.Revision = fundResult.Revision
+			token := proto4.NewAccountToken(renterKey, hostKey.PublicKey())
+
+			// upload sectors
+			roots := make([]types.Hash256, tt.n)
+			for i := range roots {
+				writeResult, err := rhp4.RPCWriteSector(context.Background(), transport, settings.Prices, token, bytes.NewReader(frand.Bytes(1024)), 1024)
+				if err != nil {
+					t.Fatal(err)
+				}
+				roots[i] = writeResult.Root
+			}
+
+			appendResult, err := rhp4.RPCAppendSectors(context.Background(), transport, fundAndSign, cs, settings.Prices, revision, roots)
+			if err != nil {
+				t.Fatal(err)
+			}
+			revision.Revision = appendResult.Revision
+
+			// free sectors using the edge-case indices
+			removeResult, err := rhp4.RPCFreeSectors(context.Background(), transport, fundAndSign, cs, settings.Prices, revision, tt.indices)
+			if err != nil {
+				t.Fatal(err)
+			}
+			assertSectorsFreed(t, removeResult.Revision, roots, tt.indices)
+			revision.Revision = removeResult.Revision
+
+			// build the set of roots that should survive
+			deleted := make(map[types.Hash256]bool)
+			for _, idx := range tt.indices {
+				deleted[roots[idx]] = true
+			}
+			kept := make(map[types.Hash256]bool)
+			for _, r := range roots {
+				if !deleted[r] {
+					kept[r] = true
+				}
+			}
+
+			// fetch the actual roots from the host
+			rootsResult, err := rhp4.RPCSectorRoots(context.Background(), transport, cs, settings.Prices, fundAndSign, revision, 0, revision.Revision.Filesize/proto4.SectorSize)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			for _, r := range rootsResult.Roots {
+				if deleted[r] {
+					t.Fatalf("deleted root %v still present on host", r)
+				}
+				delete(kept, r)
+			}
+			if len(kept) != 0 {
+				t.Fatalf("kept roots missing from host: %v", slices.Collect(maps.Keys(kept)))
+			}
+		})
+	}
 }
 
 func TestRPCSectorRoots(t *testing.T) {
@@ -2433,8 +2606,11 @@ func TestMaxSectorBatchSize(t *testing.T) {
 	assertRevision(t, rootsResp.Revision, appendRoots)
 	revision.Revision = rootsResp.Revision
 
-	// try to remove too many indices
+	// try to remove too many indices. They must be unique.
 	indices := make([]uint64, len(appendRoots)*10)
+	for i := range indices {
+		indices[i] = uint64(i)
+	}
 	_, err = rhp4.RPCFreeSectors(context.Background(), transport, renterKey, cs, settings.Prices, revision, indices)
 	if code := proto4.ErrorCode(err); code != proto4.ErrorCodeDecoding {
 		t.Fatalf("expected decoding error, got %q", err)