Remove txn from pool when contract formation or renewal fails

.changeset/clean_up_orphaned_transaction_hat_wasnt_broadcast_during_failed_formationrenewrefresh.md

@@ -0,0 +1,5 @@
+---
+default: patch
+---
+
+# Clean up orphaned transaction that wasn't broadcast during failed formation/renew/refresh

chain/manager.go

@@ -1443,6 +1443,51 @@ func (m *Manager) AddV2PoolTransactions(basis types.ChainIndex, txns []types.V2T
 	return false, nil
 }
 
+// RemoveV2PoolTransactions removes the given transactions from the transaction
+// pool, along with any pooled transactions that depend on them. It is intended
+// for abandoning a locally-added transaction set that will not be broadcast.
+// Transactions that are not in the pool are ignored.
+func (m *Manager) RemoveV2PoolTransactions(ids []types.TransactionID) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	remove := make(map[types.TransactionID]bool, len(ids))
+	for _, id := range ids {
+		remove[id] = true
+	}
+
+	filtered := m.txpool.v2txns[:0]
+	var removed bool
+	for _, txn := range m.txpool.v2txns {
+		if remove[txn.ID()] {
+			removed = true
+			continue
+		}
+		filtered = append(filtered, txn)
+	}
+	if !removed {
+		return
+	}
+	m.txpool.v2txns = filtered
+
+	// force a full revalidation
+	m.txpool.ms = nil
+	m.txpool.medianFee = nil
+	m.txpool.weight = 0
+	m.revalidatePool()
+
+	// release lock while notifying listeners
+	fns := make([]func(), 0, len(m.onPool))
+	for _, fn := range m.onPool {
+		fns = append(fns, fn)
+	}
+	m.mu.Unlock()
+	for _, fn := range fns {
+		fn()
+	}
+	m.mu.Lock()
+}
+
 // NewManager returns a Manager initialized with the provided Store and State.
 func NewManager(store Store, cs consensus.State, opts ...ManagerOption) *Manager {
 	m := &Manager{

chain/pool_test.go

@@ -107,3 +107,134 @@ func TestAddV2PoolTransactionsRecover(t *testing.T) {
 		t.Fatalf("expected invalid transaction, got %v", err)
 	}
 }
+
+func TestRemoveV2PoolTransactions(t *testing.T) {
+	// setup returns a fresh manager along with a spendable siacoin element and
+	// the key/policy needed to spend it.
+	setup := func(t *testing.T) (*chain.Manager, types.ChainIndex, types.SiacoinElement, types.PrivateKey, types.SpendPolicy) {
+		n, genesisBlock := testutil.V2Network()
+		sk := types.GeneratePrivateKey()
+		sp := types.PolicyPublicKey(sk.PublicKey())
+		addr := sp.Address()
+
+		store, genesisState, err := chain.NewDBStore(chain.NewMemDB(), n, genesisBlock, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		cm := chain.NewManager(store, genesisState)
+		es := testutil.NewElementStateStore(t, cm)
+
+		testutil.MineBlocks(t, cm, addr, 20+int(n.MaturityDelay))
+		es.Wait(t)
+
+		cs := cm.TipState()
+		basis, sces := es.SiacoinElements()
+		for _, sce := range sces {
+			if sce.SiacoinOutput.Address == addr && sce.MaturityHeight <= cs.Index.Height {
+				return cm, basis, sce, sk, sp
+			}
+		}
+		t.Fatal("no spendable element found")
+		return nil, types.ChainIndex{}, types.SiacoinElement{}, nil, types.SpendPolicy{}
+	}
+
+	// spend builds and signs a transaction spending parent and sending the
+	// remaining value (minus a fee) to dest.
+	spend := func(cm *chain.Manager, sk types.PrivateKey, sp types.SpendPolicy, parent types.SiacoinElement, dest types.Address) types.V2Transaction {
+		cs := cm.TipState()
+		txn := types.V2Transaction{
+			SiacoinInputs: []types.V2SiacoinInput{
+				{Parent: parent, SatisfiedPolicy: types.SatisfiedPolicy{Policy: sp}},
+			},
+			MinerFee: types.Siacoins(1),
+			SiacoinOutputs: []types.SiacoinOutput{
+				{Address: dest, Value: parent.SiacoinOutput.Value.Sub(types.Siacoins(1))},
+			},
+		}
+		txn.SiacoinInputs[0].SatisfiedPolicy.Signatures = []types.Signature{sk.SignHash(cs.InputSigHash(txn))}
+		return txn
+	}
+
+	t.Run("removes dependent transactions", func(t *testing.T) {
+		cm, basis, parent, sk, sp := setup(t)
+		parentTxn := spend(cm, sk, sp, parent, sp.Address())
+		childTxn := spend(cm, sk, sp, parentTxn.EphemeralSiacoinOutput(0), types.VoidAddress)
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{parentTxn, childTxn}); err != nil {
+			t.Fatal(err)
+		} else if n := len(cm.V2PoolTransactions()); n != 2 {
+			t.Fatalf("expected 2 pool transactions, got %d", n)
+		}
+
+		// removing the parent must also drop the child that depends on it
+		cm.RemoveV2PoolTransactions([]types.TransactionID{parentTxn.ID()})
+		if n := len(cm.V2PoolTransactions()); n != 0 {
+			t.Fatalf("expected empty pool after removing parent, got %d", n)
+		}
+	})
+
+	t.Run("keeps independent transactions", func(t *testing.T) {
+		cm, basis, parent, sk, sp := setup(t)
+		parentTxn := spend(cm, sk, sp, parent, sp.Address())
+		childTxn := spend(cm, sk, sp, parentTxn.EphemeralSiacoinOutput(0), types.VoidAddress)
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{parentTxn, childTxn}); err != nil {
+			t.Fatal(err)
+		}
+
+		// removing only the child must leave the parent in the pool
+		cm.RemoveV2PoolTransactions([]types.TransactionID{childTxn.ID()})
+		if _, ok := cm.V2PoolTransaction(parentTxn.ID()); !ok {
+			t.Fatal("expected parent to remain in pool")
+		} else if _, ok := cm.V2PoolTransaction(childTxn.ID()); ok {
+			t.Fatal("expected child to be removed from pool")
+		}
+	})
+
+	t.Run("no-op when transaction not in pool", func(t *testing.T) {
+		cm, basis, parent, sk, sp := setup(t)
+		txn := spend(cm, sk, sp, parent, types.VoidAddress)
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{txn}); err != nil {
+			t.Fatal(err)
+		}
+
+		var notified int
+		cancel := cm.OnPoolChange(func() { notified++ })
+		defer cancel()
+
+		// removing an unknown transaction must not change the pool or notify
+		cm.RemoveV2PoolTransactions([]types.TransactionID{{1, 2, 3}})
+		if n := len(cm.V2PoolTransactions()); n != 1 {
+			t.Fatalf("expected pool to be unchanged, got %d", n)
+		} else if notified != 0 {
+			t.Fatalf("expected no notification for no-op removal, got %d", notified)
+		}
+
+		// removing a known transaction must notify listeners
+		cm.RemoveV2PoolTransactions([]types.TransactionID{txn.ID()})
+		if notified != 1 {
+			t.Fatalf("expected 1 notification, got %d", notified)
+		} else if n := len(cm.V2PoolTransactions()); n != 0 {
+			t.Fatalf("expected empty pool, got %d", n)
+		}
+	})
+
+	t.Run("frees inputs for reuse", func(t *testing.T) {
+		cm, basis, parent, sk, sp := setup(t)
+		first := spend(cm, sk, sp, parent, sp.Address())
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{first}); err != nil {
+			t.Fatal(err)
+		}
+
+		// a different transaction reusing the same input conflicts while the
+		// first transaction is still in the pool
+		second := spend(cm, sk, sp, parent, types.VoidAddress)
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{second}); err == nil {
+			t.Fatal("expected double-spend conflict while first transaction is in the pool")
+		}
+
+		// removing the first transaction frees the input so it can be reused
+		cm.RemoveV2PoolTransactions([]types.TransactionID{first.ID()})
+		if _, err := cm.AddV2PoolTransactions(basis, []types.V2Transaction{second}); err != nil {
+			t.Fatalf("expected to reuse freed input after removal, got %v", err)
+		}
+	})
+}

rhp/v4/rpc_test.go

@@ -14,6 +14,7 @@ import (
 	"slices"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -1369,6 +1370,138 @@ func TestRPCRenew(t *testing.T) {
 	})
 }
 
+// failingContractor wraps an EphemeralContractor and can be configured to fail
+// AddV2Contract and RenewV2Contract, simulating a host that fails to persist a
+// contract after the transaction set has already been added to the pool but
+// before it is broadcast.
+type failingContractor struct {
+	*testutil.EphemeralContractor
+	failAdd   atomic.Bool
+	failRenew atomic.Bool
+}
+
+func (fc *failingContractor) AddV2Contract(ts rhp4.TransactionSet, usage proto4.Usage) error {
+	if fc.failAdd.Load() {
+		return errors.New("injected formation failure")
+	}
+	return fc.EphemeralContractor.AddV2Contract(ts, usage)
+}
+
+func (fc *failingContractor) RenewV2Contract(ts rhp4.TransactionSet, usage proto4.Usage) error {
+	if fc.failRenew.Load() {
+		return errors.New("injected renewal failure")
+	}
+	return fc.EphemeralContractor.RenewV2Contract(ts, usage)
+}
+
+// TestRPCRenewRecoverAfterPoolAdd ensures that when the host fails to persist a
+// renewal after the renewal set has already been added to the transaction pool,
+// the transaction is removed from the pool so that the contract is not left
+// resolved and a subsequent renewal can succeed.
+func TestRPCRenewRecoverAfterPoolAdd(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), // 0.2 SC
+			StoragePrice:  types.NewCurrency64(100),   // 100 H / byte / block
+			IngressPrice:  types.NewCurrency64(100),   // 100 H / byte
+			EgressPrice:   types.NewCurrency64(100),   // 100 H / byte
+			Collateral:    types.NewCurrency64(200),
+		},
+	})
+	ss := testutil.NewEphemeralSectorStore()
+	c := &failingContractor{EphemeralContractor: 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}
+
+	// form a contract and confirm it
+	formResult, err := rhp4.RPCFormContract(context.Background(), transport, cm, fundAndSign, cm.TipState(), 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)
+	}
+	if _, err := cm.AddV2PoolTransactions(formResult.FormationSet.Basis, formResult.FormationSet.Transactions); err != nil {
+		t.Fatal(err)
+	}
+	mineAndSync(t, cm, types.VoidAddress, 10, w, c)
+
+	revision := formResult.Contract
+	renewParams := proto4.RPCRenewContractParams{
+		ContractID:  revision.ID,
+		Allowance:   types.Siacoins(150),
+		Collateral:  types.Siacoins(300),
+		ProofHeight: revision.Revision.ProofHeight + 10,
+	}
+
+	// the pool should be empty now that the formation is confirmed
+	if n := len(cm.V2PoolTransactions()); n != 0 {
+		t.Fatalf("expected empty pool before renewal, got %d", n)
+	}
+
+	// configure the host to fail persisting the renewal; this happens after the
+	// renewal set is added to the pool but before it is broadcast
+	c.failRenew.Store(true)
+	if _, err := rhp4.RPCRenewContract(context.Background(), transport, cm, fundAndSign, cm.TipState(), settings.Prices, settings.WalletAddress, revision.Revision, renewParams); err == nil {
+		t.Fatal("expected renewal to fail")
+	}
+
+	// the failed renewal must have been removed from the pool, leaving the
+	// parent contract unresolved and revisable
+	if n := len(cm.V2PoolTransactions()); n != 0 {
+		t.Fatalf("expected failed renewal to be removed from the pool, got %d", n)
+	}
+	rs, err := rhp4.RPCLatestRevision(context.Background(), transport, revision.ID)
+	if err != nil {
+		t.Fatal(err)
+	} else if rs.Renewed {
+		t.Fatal("expected contract to not be renewed after failed renewal")
+	} else if !rs.Revisable {
+		t.Fatal("expected contract to still be revisable after failed renewal")
+	}
+
+	// retrying the renewal must now succeed
+	c.failRenew.Store(false)
+	renewResult, err := rhp4.RPCRenewContract(context.Background(), transport, cm, fundAndSign, cm.TipState(), settings.Prices, settings.WalletAddress, revision.Revision, renewParams)
+	if err != nil {
+		t.Fatalf("expected renewal to succeed after recovery, got %v", err)
+	}
+	if known, err := cm.AddV2PoolTransactions(renewResult.RenewalSet.Basis, renewResult.RenewalSet.Transactions); err != nil {
+		t.Fatal(err)
+	} else if !known {
+		t.Fatal("expected renewal set to be known")
+	}
+	if rs, err := rhp4.RPCLatestRevision(context.Background(), transport, revision.ID); err != nil {
+		t.Fatal(err)
+	} else if !rs.Renewed {
+		t.Fatal("expected contract to be renewed after successful retry")
+	}
+}
+
 func TestRPCTimeout(t *testing.T) {
 	n, genesis := testutil.V2Network()
 	cm, w := startTestNode(t, n, genesis)