From 218df5690d25e616bd9ebb88b2b3e4579b97c963 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Fri, 15 May 2026 06:03:47 -0700 Subject: [PATCH 1/9] oor: Validate incoming ancestor packages (#366) Incoming receive materialization persists ancestor OOR artifacts for later unroll recovery. Those artifacts come from operator/indexer responses, so accepting only parseable PSBTs lets a bad response overwrite recovery state. Validate the finalized package shape, txid-derived session id, and ancestor reachability before converting RPC responses or writing package rows. Cover the positive case where a checkpoint actually spends an ancestor Ark output, the duplicate-ancestor rejection path, and the per-ancestor checkpoint count cap added on the RPC adapter. Additionally, harden UpsertPackage: previously it checked only the stored direction before falling through to delete and re-insert the Ark PSBT and checkpoint rows. That made the artifact store a rewrite surface: a malicious operator/indexer response that produced a parseable package for a known session id could overwrite previously stored recovery artifacts even when the FSM-layer validation passed (e.g. when the attacker holds the original Ark transaction). Treat retried upserts as idempotent only when the serialized Ark PSBT and checkpoint payload match the existing row. Any divergence now fails the upsert so the original recovery artifact survives. --- db/oor_artifact_store.go | 46 ++++++ db/oor_artifact_store_test.go | 44 ++++++ oor/incoming_adapter.go | 21 ++- oor/local_persistence_handler.go | 12 ++ oor/local_persistence_handler_test.go | 209 ++++++++++++++++++++++++++ oor/package_validation.go | 156 +++++++++++++++++++ oor/receive_limits_test.go | 41 +++++ 7 files changed, 526 insertions(+), 3 deletions(-) create mode 100644 oor/package_validation.go diff --git a/db/oor_artifact_store.go b/db/oor_artifact_store.go index 2308eeddc..7dc48de43 100644 --- a/db/oor_artifact_store.go +++ b/db/oor_artifact_store.go @@ -344,6 +344,19 @@ func (s *OORArtifactPersistenceStore) UpsertPackage(ctx context.Context, ) } + samePayload, err := sameOORPackagePayload( + ctx, q, existing, arkRaw, rawCheckpoints, + ) + if err != nil { + return err + } + if !samePayload { + return fmt.Errorf("oor package %x already "+ + "exists with different payload", id) + } + + return nil + case errors.Is(err, sql.ErrNoRows): // New package insert path. @@ -1277,6 +1290,39 @@ func validatePackageDirection(direction OORPackageDirection) error { } } +// sameOORPackagePayload reports whether an existing package row already holds +// the exact serialized payload being upserted. +func sameOORPackagePayload(ctx context.Context, q OORArtifactStore, + existing sqlc.OorPackage, arkRaw []byte, + rawCheckpoints [][]byte) (bool, error) { + + if !bytes.Equal(existing.ArkPsbt, arkRaw) { + return false, nil + } + + existingCheckpoints, err := q.ListOORPackageCheckpoints( + ctx, existing.SessionID, + ) + if err != nil { + return false, err + } + + if len(existingCheckpoints) != len(rawCheckpoints) { + return false, nil + } + + for i := range rawCheckpoints { + if !bytes.Equal( + existingCheckpoints[i].CheckpointPsbt, + rawCheckpoints[i], + ) { + return false, nil + } + } + + return true, nil +} + func packageDirectionCode(direction OORPackageDirection) (int32, error) { if err := validatePackageDirection(direction); err != nil { return 0, err diff --git a/db/oor_artifact_store_test.go b/db/oor_artifact_store_test.go index bc894f99a..3e9008483 100644 --- a/db/oor_artifact_store_test.go +++ b/db/oor_artifact_store_test.go @@ -220,6 +220,50 @@ func TestOORArtifactStoreUpsertPackageDirectionConflict(t *testing.T) { require.ErrorContains(t, err, "package direction conflict") } +// TestOORArtifactStoreUpsertPackageRejectsPayloadRewrite verifies same-session +// package persistence is retry-idempotent, not a rewrite surface. +func TestOORArtifactStoreUpsertPackageRejectsPayloadRewrite(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store, _ := newOORArtifactStoreForTest(t) + + sessionID, arkPSBT, checkpoints, _, _, _, _ := buildTestOORPackage( + t, 0x35, + ) + + err := store.UpsertPackage( + ctx, OORPackageDirectionIncoming, sessionID, arkPSBT, + checkpoints, + ) + require.NoError(t, err) + + err = store.UpsertPackage( + ctx, OORPackageDirectionIncoming, sessionID, arkPSBT, + checkpoints, + ) + require.NoError(t, err) + + checkpoints[0].Inputs[0].FinalScriptWitness = []byte{ + 0x01, + 0x51, + } + + err = store.UpsertPackage( + ctx, OORPackageDirectionIncoming, sessionID, arkPSBT, + checkpoints, + ) + require.Error(t, err) + require.ErrorContains(t, err, "already exists with different payload") + + pkg, err := store.GetPackage(ctx, sessionID) + require.NoError(t, err) + require.Empty( + t, pkg.FinalCheckpointPSBTs[0].Inputs[0]. + FinalScriptWitness, + ) +} + // TestOORArtifactStoreGetPackageForOutpointPrefersCreatedBinding verifies that // outpoint lookups return the created-output package when both created and // consumed bindings exist for the same outpoint. diff --git a/oor/incoming_adapter.go b/oor/incoming_adapter.go index cf48c9d52..718f9d337 100644 --- a/oor/incoming_adapter.go +++ b/oor/incoming_adapter.go @@ -164,12 +164,18 @@ func IncomingTransferEventFromResponseWithLimits(sessionID SessionID, } ancestors, err := packageArtifactsFromRPC( - recipientEvt.GetAncestorPackages(), + recipientEvt.GetAncestorPackages(), limits, ) if err != nil { return nil, err } + root := packageArtifactForValidation(sessionID, arkPSBT, checkpoints) + err = validateIncomingPackageGraph(root, ancestors) + if err != nil { + return nil, err + } + return &IncomingTransferEvent{ SessionID: sessionID, ArkPSBT: arkPSBT, @@ -181,8 +187,8 @@ func IncomingTransferEventFromResponseWithLimits(sessionID SessionID, // packageArtifactsFromRPC converts RPC package artifacts into domain // artifacts after enforcing the same bounded-shape policy as checkpoint // parsing. -func packageArtifactsFromRPC(pkgs []*arkrpc.OORSessionPackage) ( - []PackageArtifact, error) { +func packageArtifactsFromRPC(pkgs []*arkrpc.OORSessionPackage, + limits ReceiveLimits) ([]PackageArtifact, error) { const maxAncestorPackages = 64 if len(pkgs) > maxAncestorPackages { @@ -190,6 +196,7 @@ func packageArtifactsFromRPC(pkgs []*arkrpc.OORSessionPackage) ( "limit %d", len(pkgs), maxAncestorPackages) } + limits = normalizeReceiveLimits(limits) artifacts := make([]PackageArtifact, 0, len(pkgs)) for i := range pkgs { pkg := pkgs[i] @@ -209,6 +216,14 @@ func packageArtifactsFromRPC(pkgs []*arkrpc.OORSessionPackage) ( "psbt %d: %w", i, err) } + if uint64(len(pkg.GetCheckpointPsbts())) > + uint64(limits.MaxCheckpoints) { + return nil, fmt.Errorf("ancestor package %d "+ + "checkpoint count %d exceeds limit %d", i, + len(pkg.GetCheckpointPsbts()), + limits.MaxCheckpoints) + } + checkpoints := make( []*psbt.Packet, 0, len( diff --git a/oor/local_persistence_handler.go b/oor/local_persistence_handler.go index cfd2f7dc0..195397f55 100644 --- a/oor/local_persistence_handler.go +++ b/oor/local_persistence_handler.go @@ -304,6 +304,18 @@ func (h *LocalPersistenceOutboxHandler) validateMaterializeIncoming( return fmt.Errorf("incoming recipients must be provided") } + if h.PackageStore != nil { + root := packageArtifactForValidation( + msg.SessionID, msg.ArkPSBT, msg.FinalCheckpointPSBTs, + ) + err := validateIncomingPackageGraph( + root, msg.AncestorPackages, + ) + if err != nil { + return err + } + } + return nil } diff --git a/oor/local_persistence_handler_test.go b/oor/local_persistence_handler_test.go index 67f9c0e89..c115f0e5f 100644 --- a/oor/local_persistence_handler_test.go +++ b/oor/local_persistence_handler_test.go @@ -282,6 +282,164 @@ func TestLocalPersistenceOutboxHandlerMaterializeIncoming(t *testing.T) { require.Equal(t, chainhash.Hash(sessionID), packageStore.lastSessionID) } +// TestLocalPersistenceOutboxHandlerRejectsInvalidAncestorPackage asserts +// untrusted incoming ancestor packages are validated before they can poison the +// package store used by recovery. +func TestLocalPersistenceOutboxHandlerRejectsInvalidAncestorPackage( + t *testing.T) { + + t.Parallel() + + arkPSBT, finalCheckpoints, recipients, _, _, operatorKey := + buildTestIncomingMaterialization(t) + + packageStore := &testPackageStore{} + handler := &LocalPersistenceOutboxHandler{ + Store: newTestVTXOStore(), + PackageStore: packageStore, + OperatorKey: operatorKey, + ExitDelay: 10, + NotifyIncomingVTXOs: func(_ context.Context, + _ []*vtxo.Descriptor) error { + + return nil + }, + ResolveIncomingClientKey: func(ctx context.Context, + recipient ArkRecipientOutput) (keychain.KeyDescriptor, + error) { + + _ = ctx + _ = recipient + + return keychain.KeyDescriptor{}, nil + }, + } + + sessionID := SessionID(arkPSBT.UnsignedTx.TxHash()) + ancestorID := sessionID + ancestorID[0] ^= 0x01 + + req := &MaterializeIncomingVTXOsRequest{ + SessionID: sessionID, + ArkPSBT: arkPSBT, + FinalCheckpointPSBTs: finalCheckpoints, + Recipients: recipients, + AncestorPackages: []PackageArtifact{{ + SessionID: ancestorID, + ArkPSBT: arkPSBT, + FinalCheckpointPSBTs: finalCheckpoints, + }}, + } + + events, err := handler.Handle(t.Context(), sessionID, req) + require.Error(t, err) + require.ErrorContains( + t, err, "ancestor package 0 session id does not match ark txid", + ) + require.Empty(t, events) + require.Zero(t, packageStore.packageCalls) +} + +// TestValidateIncomingPackageGraphRejectsUnconsumedAncestor asserts valid but +// unrelated ancestor packages must not be accepted as recovery ancestors. +func TestValidateIncomingPackageGraphRejectsUnconsumedAncestor(t *testing.T) { + t.Parallel() + + arkPSBT, finalCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + ancestorArk, ancestorCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + + root := packageArtifactForValidation( + SessionID( + arkPSBT.UnsignedTx.TxHash(), + ), + arkPSBT, + finalCheckpoints, + ) + ancestor := packageArtifactForValidation( + SessionID( + ancestorArk.UnsignedTx.TxHash(), + ), + ancestorArk, + ancestorCheckpoints, + ) + + err := validateIncomingPackageGraph(root, []PackageArtifact{ancestor}) + require.Error(t, err) + require.ErrorContains( + t, err, "is not consumed by incoming package chain", + ) +} + +// TestValidateIncomingPackageGraphAcceptsConnectedAncestor asserts a package +// whose checkpoint spends an ancestor Ark output can carry that ancestor as +// recovery material. +func TestValidateIncomingPackageGraphAcceptsConnectedAncestor(t *testing.T) { + t.Parallel() + + ancestorArk, ancestorCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + rootArk, rootCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + rootArk, rootCheckpoints = reparentTestIncomingPackage( + t, rootArk, rootCheckpoints, ancestorArk, + ) + + root := packageArtifactForValidation( + SessionID( + rootArk.UnsignedTx.TxHash(), + ), + rootArk, + rootCheckpoints, + ) + ancestor := packageArtifactForValidation( + SessionID( + ancestorArk.UnsignedTx.TxHash(), + ), + ancestorArk, + ancestorCheckpoints, + ) + + err := validateIncomingPackageGraph(root, []PackageArtifact{ancestor}) + require.NoError(t, err) +} + +// TestValidateIncomingPackageGraphRejectsDuplicateAncestor asserts valid +// ancestors still cannot be supplied more than once. +func TestValidateIncomingPackageGraphRejectsDuplicateAncestor(t *testing.T) { + t.Parallel() + + ancestorArk, ancestorCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + rootArk, rootCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + rootArk, rootCheckpoints = reparentTestIncomingPackage( + t, rootArk, rootCheckpoints, ancestorArk, + ) + + root := packageArtifactForValidation( + SessionID( + rootArk.UnsignedTx.TxHash(), + ), + rootArk, + rootCheckpoints, + ) + ancestor := packageArtifactForValidation( + SessionID( + ancestorArk.UnsignedTx.TxHash(), + ), + ancestorArk, + ancestorCheckpoints, + ) + + err := validateIncomingPackageGraph( + root, []PackageArtifact{ancestor, ancestor}, + ) + require.Error(t, err) + require.ErrorContains(t, err, "duplicate ancestor package") +} + // TestLocalPersistenceOutboxHandlerUsesMetadataOperatorKey asserts incoming // materialization prefers the per-VTXO operator key returned by the indexer // over the handler's compatibility fallback key. @@ -1084,6 +1242,7 @@ func buildTestIncomingMaterialization(t *testing.T) (*psbt.Packet, cp, err := oortx.BuildCheckpointPSBT(policy, inputs[0]) require.NoError(t, err) + cp.PSBT.Inputs[0].FinalScriptWitness = []byte{0x01, 0x51} arkPSBT, err := oortx.BuildArkPSBT( []oortx.CheckpointOutput{ @@ -1105,6 +1264,56 @@ func buildTestIncomingMaterialization(t *testing.T) (*psbt.Packet, operatorKey.PubKey() } +// reparentTestIncomingPackage rewrites the test package's checkpoint input to +// spend output zero of parentArk, then rebuilds the Ark transaction so its +// session ID follows the new checkpoint txid. +func reparentTestIncomingPackage(t *testing.T, arkPSBT *psbt.Packet, + checkpoints []*psbt.Packet, + parentArk *psbt.Packet) (*psbt.Packet, []*psbt.Packet) { + + t.Helper() + + require.Len(t, checkpoints, 1) + require.NotNil(t, parentArk) + require.NotNil(t, parentArk.UnsignedTx) + require.NotEmpty(t, parentArk.UnsignedTx.TxOut) + + checkpoint := checkpoints[0] + require.NotNil(t, checkpoint) + require.NotNil(t, checkpoint.UnsignedTx) + require.NotEmpty(t, checkpoint.UnsignedTx.TxIn) + require.NotEmpty(t, checkpoint.UnsignedTx.TxOut) + + parentTxid := parentArk.UnsignedTx.TxHash() + checkpoint.UnsignedTx.TxIn[0].PreviousOutPoint = wire.OutPoint{ + Hash: parentTxid, + Index: 0, + } + checkpoint.Inputs[0].WitnessUtxo = parentArk.UnsignedTx.TxOut[0] + + recipients, err := ExtractArkRecipients(arkPSBT) + require.NoError(t, err) + + outputs := make([]oortx.RecipientOutput, 0, len(recipients)) + for i := range recipients { + outputs = append(outputs, oortx.RecipientOutput{ + PkScript: recipients[i].PkScript, + Value: recipients[i].Value, + }) + } + + arkPSBT, err = oortx.BuildArkPSBT( + []oortx.CheckpointOutput{{ + Txid: checkpoint.UnsignedTx.TxHash(), + Output: checkpoint.UnsignedTx.TxOut[0], + }}, + outputs, + ) + require.NoError(t, err) + + return arkPSBT, checkpoints +} + // validTestIncomingAncestry returns a minimal Ancestry slice that passes // BuildIncomingVTXODescriptor's structural cross-check, anchored at the // supplied commitment txid. The test ark PSBT built by diff --git a/oor/package_validation.go b/oor/package_validation.go new file mode 100644 index 000000000..5a62758d0 --- /dev/null +++ b/oor/package_validation.go @@ -0,0 +1,156 @@ +package oor + +import ( + "fmt" + + "github.com/btcsuite/btcd/btcutil/psbt" + "github.com/lightninglabs/darepo-client/lib/tx/arktx" + oortx "github.com/lightninglabs/darepo-client/lib/tx/oor" +) + +// validateIncomingPackage validates the finalized OOR package shape before +// any untrusted incoming artifact is persisted for future recovery. +func validateIncomingPackage(label string, sessionID SessionID, + ark *psbt.Packet, checkpoints []*psbt.Packet) error { + + if sessionID == (SessionID{}) { + return fmt.Errorf("%s session id must be provided", label) + } + + if ark == nil || ark.UnsignedTx == nil { + return fmt.Errorf("%s ark psbt must be provided", label) + } + + txid := ark.UnsignedTx.TxHash() + if SessionID(txid) != sessionID { + return fmt.Errorf("%s session id does not match ark txid", + label) + } + + err := oortx.ValidateFinalizePackage(ark, checkpoints) + if err != nil { + return fmt.Errorf("%s finalize package invalid: %w", label, err) + } + + return nil +} + +// validateIncomingPackageGraph validates that incoming ancestor artifacts are +// finalized packages and that each supplied ancestor is reachable from the +// received package's checkpoint-input chain. This avoids persisting unrelated +// operator/indexer-supplied packages as recovery material. +func validateIncomingPackageGraph(root PackageArtifact, + ancestors []PackageArtifact) error { + + if err := validateIncomingPackage( + "incoming package", root.SessionID, root.ArkPSBT, + root.FinalCheckpointPSBTs, + ); err != nil { + return err + } + + ancestorBySession := make(map[SessionID]PackageArtifact, len(ancestors)) + for i := range ancestors { + ancestor := ancestors[i] + label := fmt.Sprintf("ancestor package %d", i) + + if err := validateIncomingPackage( + label, ancestor.SessionID, ancestor.ArkPSBT, + ancestor.FinalCheckpointPSBTs, + ); err != nil { + return err + } + + if ancestor.SessionID == root.SessionID { + return fmt.Errorf("%s duplicates incoming package", + label) + } + + if _, ok := ancestorBySession[ancestor.SessionID]; ok { + return fmt.Errorf("duplicate ancestor package %s", + ancestor.SessionID.String()) + } + + ancestorBySession[ancestor.SessionID] = ancestor + } + + reachable := make(map[SessionID]struct{}, len(ancestors)) + walkPackageAncestors(root, ancestorBySession, reachable) + + for i := range ancestors { + ancestor := ancestors[i] + if _, ok := reachable[ancestor.SessionID]; !ok { + return fmt.Errorf("ancestor package %s is not "+ + "consumed by incoming package chain", + ancestor.SessionID.String()) + } + } + + return nil +} + +// walkPackageAncestors marks ancestor packages reachable from pkg's finalized +// checkpoints by following checkpoint input prevouts that spend ancestor Ark +// outputs. +func walkPackageAncestors(pkg PackageArtifact, + ancestorBySession map[SessionID]PackageArtifact, + reachable map[SessionID]struct{}) { + + for i := range pkg.FinalCheckpointPSBTs { + checkpoint := pkg.FinalCheckpointPSBTs[i] + if checkpoint == nil || checkpoint.UnsignedTx == nil || + len(checkpoint.UnsignedTx.TxIn) == 0 { + + continue + } + + // Finalized OOR checkpoints are single-input collab spends. + // oortx.ValidateFinalizePackage enforces that invariant + // before this graph walk, so TxIn[0] is the only ancestry + // edge to follow. + prevOut := checkpoint.UnsignedTx.TxIn[0].PreviousOutPoint + parentID := SessionID(prevOut.Hash) + ancestor, ok := ancestorBySession[parentID] + if !ok { + continue + } + + if !validAncestorOutput(ancestor.ArkPSBT, prevOut.Index) { + continue + } + + if _, ok := reachable[parentID]; ok { + continue + } + + reachable[parentID] = struct{}{} + walkPackageAncestors(ancestor, ancestorBySession, reachable) + } +} + +// validAncestorOutput reports whether an ancestor checkpoint input references a +// real non-anchor Ark output in the referenced ancestor package. +func validAncestorOutput(ark *psbt.Packet, outputIndex uint32) bool { + if ark == nil || ark.UnsignedTx == nil { + return false + } + + tx := ark.UnsignedTx + if outputIndex >= uint32(len(tx.TxOut)) { + return false + } + + return !arktx.IsAnchorOutput(tx.TxOut[outputIndex]) +} + +// packageArtifactForValidation projects package fields into the common +// validation shape. +func packageArtifactForValidation(sessionID SessionID, ark *psbt.Packet, + checkpoints []*psbt.Packet) PackageArtifact { + + return PackageArtifact{ + SessionID: sessionID, + ArkPSBT: ark, + FinalCheckpointPSBTs: checkpoints, + } +} diff --git a/oor/receive_limits_test.go b/oor/receive_limits_test.go index 9e28e0b8d..4328eb313 100644 --- a/oor/receive_limits_test.go +++ b/oor/receive_limits_test.go @@ -5,6 +5,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/lightninglabs/darepo-client/arkrpc" + "github.com/lightninglabs/darepo-client/lib/tx/psbtutil" "github.com/stretchr/testify/require" ) @@ -34,6 +35,46 @@ func TestIncomingTransferEventFromResponseUsesConfiguredCheckpointLimit( require.ErrorContains(t, err, "checkpoint count 2 exceeds limit 1") } +// TestIncomingTransferEventFromResponseLimitsAncestorCheckpoints verifies the +// RPC adapter bounds checkpoint lists on supplied ancestor packages before +// they can enter the receive FSM. +func TestIncomingTransferEventFromResponseLimitsAncestorCheckpoints( + t *testing.T) { + + t.Parallel() + + resp, sessionID, _, recipientEventID := buildIncomingResolveResponse(t) + ancestorArk, ancestorCheckpoints, _, _, _, _ := + buildTestIncomingMaterialization(t) + + ancestorArkRaw, err := psbtutil.Serialize(ancestorArk) + require.NoError(t, err) + + ancestorCheckpointRaw, err := psbtutil.Serialize( + ancestorCheckpoints[0], + ) + require.NoError(t, err) + + ancestorID := SessionID(ancestorArk.UnsignedTx.TxHash()) + resp.Events[0].AncestorPackages = []*arkrpc.OORSessionPackage{{ + SessionId: ancestorID[:], + ArkPsbt: ancestorArkRaw, + CheckpointPsbts: [][]byte{ + ancestorCheckpointRaw, + ancestorCheckpointRaw, + }, + }} + + _, err = IncomingTransferEventFromResponseWithLimits( + sessionID, recipientEventID, resp, ReceiveLimits{ + MaxCheckpoints: 1, + }, + ) + require.ErrorContains( + t, err, "ancestor package 0 checkpoint count 2 exceeds limit 1", + ) +} + // TestIncomingMetadataMatchesFromResponseUsesConfiguredMatchLimit verifies // incoming metadata response adaptation enforces the configured match cap. func TestIncomingMetadataMatchesFromResponseUsesConfiguredMatchLimit( From 73f114016f8ee4867dc42d27d6a2d3773b1f0e67 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 19:27:28 -0700 Subject: [PATCH 2/9] db: Reject untrusted OOR ancestor packages by txid in resolver The foreign-ancestor fallback in ResolveUnrollPackages loads a session-keyed package when no local outpoint binding exists for a checkpoint input. Until now it only bounds-checked the referenced output index, which meant any stored row whose session_id column matched the checkpoint input's previous hash would be accepted as resolved ancestry, even when the stored Ark transaction did not actually hash to that session id or when the index landed on the non-spendable anchor output. Ancestor packages are operator/indexer-supplied artifacts, so a poisoned or mismatched row could otherwise be grafted onto an unroll-chain at resolution time and corrupt unilateral-exit proof assembly. Re-verify the txid binding and reject anchor outputs at read time so the resolver does not depend on writers having enforced those invariants. Mismatches are treated as unresolved inputs rather than hard errors so the chain still degrades to the existing recovery path. Closes #371. --- db/oor_unroll_resolver.go | 49 ++++++++--- db/oor_unroll_resolver_test.go | 149 +++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 12 deletions(-) diff --git a/db/oor_unroll_resolver.go b/db/oor_unroll_resolver.go index d698f4a23..82a3c0152 100644 --- a/db/oor_unroll_resolver.go +++ b/db/oor_unroll_resolver.go @@ -11,6 +11,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" "github.com/lightninglabs/darepo-client/db/sqlc" + "github.com/lightninglabs/darepo-client/lib/tx/arktx" fn "github.com/lightningnetwork/lnd/fn/v2" ) @@ -226,7 +227,13 @@ func resolveInputPackage(ctx context.Context, q OORArtifactStore, } // Fall back to a session-id lookup for foreign-owned ancestors that - // the local wallet only has session-keyed visibility into. + // the local wallet only has session-keyed visibility into. Ancestor + // packages persisted under a session id are operator/indexer-supplied + // artifacts without a local outpoint binding, so we must re-verify the + // txid binding and the referenced output at read time before treating + // the package as the parent of an unroll-chain input. Trusting only + // the stored session id would let a poisoned row claim ancestry for + // any checkpoint input whose previous hash matches its session id. pkg, err = loadPackageBundleBySessionID(ctx, q, input.Hash) if errors.Is(err, sql.ErrNoRows) { return nil, true, nil @@ -235,13 +242,41 @@ func resolveInputPackage(ctx context.Context, q OORArtifactStore, return nil, false, err } - if !packageCreatesOutput(pkg, input.Index) { + if !packageProducesAncestorOutput(pkg, input.Hash, input.Index) { return nil, true, nil } return pkg, false, nil } +// packageProducesAncestorOutput reports whether the foreign-ancestor package +// actually produces the referenced checkpoint-input outpoint. It rejects: +// +// - packages whose stored Ark transaction does not actually hash to the +// requested session id (txid binding mismatch from a tampered or +// mismatched row), +// - output indices outside the package's TxOut range, +// - references that land on the Ark anchor output, which is never a +// spendable VTXO and so cannot be a real ancestor checkpoint input. +func packageProducesAncestorOutput(pkg *OORPackageBundle, + expectedTxid chainhash.Hash, index uint32) bool { + + if pkg == nil || pkg.ArkPSBT == nil || pkg.ArkPSBT.UnsignedTx == nil { + return false + } + + tx := pkg.ArkPSBT.UnsignedTx + if tx.TxHash() != expectedTxid { + return false + } + + if index >= uint32(len(tx.TxOut)) { + return false + } + + return !arktx.IsAnchorOutput(tx.TxOut[index]) +} + // checkpointInputOutpoints returns de-duplicated checkpoint input outpoints // referenced by the package's finalized checkpoints. func checkpointInputOutpoints(pkg *OORPackageBundle) []wire.OutPoint { @@ -328,16 +363,6 @@ func loadPackageBundleBySessionID(ctx context.Context, q OORArtifactStore, return materializePackageBundle(ctx, q, row) } -// packageCreatesOutput reports whether the package's Ark transaction has the -// output index referenced by a child checkpoint input. -func packageCreatesOutput(pkg *OORPackageBundle, index uint32) bool { - if pkg == nil || pkg.ArkPSBT == nil || pkg.ArkPSBT.UnsignedTx == nil { - return false - } - - return int(index) < len(pkg.ArkPSBT.UnsignedTx.TxOut) -} - // loadPackageBundleByCreatedOutputOutpoint resolves a full package bundle // from one created-output binding outpoint. func loadPackageBundleByCreatedOutputOutpoint(ctx context.Context, diff --git a/db/oor_unroll_resolver_test.go b/db/oor_unroll_resolver_test.go index 599d7a414..fbc3a19bb 100644 --- a/db/oor_unroll_resolver_test.go +++ b/db/oor_unroll_resolver_test.go @@ -331,3 +331,152 @@ func TestResolveUnrollPackagesMaxDepthExceeded(t *testing.T) { _, err := store.ResolveUnrollPackages(ctx, targetOutpoint) require.ErrorIs(t, err, ErrResolveUnrollMaxDepthExceeded) } + +// TestResolveUnrollPackagesRejectsTxidMismatchedAncestor verifies that the +// foreign-ancestor fallback rejects a persisted package whose stored Ark tx +// does not actually hash to the requested session id. Such a row would +// otherwise let a poisoned operator/indexer response stand in as the parent +// of any checkpoint input whose previous hash happened to match the rogue +// session id. +func TestResolveUnrollPackagesRejectsTxidMismatchedAncestor(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store, roundStore := newOORArtifactStoreForTest(t) + + // Build a parent package and rebind its row under a session id that + // does not match the actual ark txid. This simulates a poisoned + // session-keyed row written by a buggy or malicious code path that + // did not enforce the txid binding at write time. + parentSession, parentArk, parentCheckpoints, _, + _, _, _ := buildTestOORPackageWithInput( + t, 0xa1, wire.OutPoint{ + Hash: chainhash.Hash{0xa1, 0xaa}, + }, + ) + + rogueSession := chainhash.Hash{0xde, 0xad, 0xbe, 0xef} + require.NotEqual(t, parentSession, rogueSession) + + err := store.UpsertPackage( + ctx, OORPackageDirectionIncoming, rogueSession, parentArk, + parentCheckpoints, + ) + require.NoError(t, err) + + // Build a child package whose checkpoint input claims the rogue + // session id as its parent. The fallback lookup will succeed at the + // DB layer, so the txid-binding check is what must keep the + // poisoned ancestor out of the resolved chain. + rogueParentOutpoint := wire.OutPoint{ + Hash: rogueSession, + Index: 0, + } + + childSession, childArk, childCheckpoints, childOutpoint, childScript, + childValue, _ := buildTestOORPackageWithInput( + t, 0xa2, rogueParentOutpoint, + ) + + err = store.UpsertPackage( + ctx, OORPackageDirectionIncoming, childSession, childArk, + childCheckpoints, + ) + require.NoError(t, err) + + seedBindingOutpoint( + t, ctx, roundStore, childOutpoint, childScript, childValue, + ) + + err = store.UpsertBinding( + ctx, childOutpoint, childSession, 0, + OORPackageLinkKindCreatedOutput, + ) + require.NoError(t, err) + + resolved, err := store.ResolveUnrollPackages(ctx, childOutpoint) + require.NoError(t, err) + require.NotNil(t, resolved) + + // The poisoned ancestor must be rejected: only the child package + // remains and the rogue parent outpoint is surfaced as unresolved. + require.Len(t, resolved.Packages, 1) + require.Equal(t, childSession, resolved.Packages[0].SessionID) + require.Len(t, resolved.UnresolvedCheckpointInputs, 1) + require.Equal( + t, rogueParentOutpoint, resolved.UnresolvedCheckpointInputs[0], + ) +} + +// TestResolveUnrollPackagesRejectsAnchorOutputAncestor verifies the +// foreign-ancestor fallback rejects a persisted package whose claimed +// parent output index lands on the Ark anchor output. Anchor outputs are +// never spendable VTXOs, so a child checkpoint that purports to spend one +// could only originate from a malformed or malicious package and must not +// be accepted as resolved ancestry. +func TestResolveUnrollPackagesRejectsAnchorOutputAncestor(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store, roundStore := newOORArtifactStoreForTest(t) + + // The fixture builds an Ark PSBT with output 0 = recipient and + // output 1 = anchor. We point the child checkpoint input at output 1. + parentSession, parentArk, parentCheckpoints, _, + _, _, _ := buildTestOORPackageWithInput( + t, 0xb1, wire.OutPoint{ + Hash: chainhash.Hash{0xb1, 0xaa}, + }, + ) + + err := store.UpsertPackage( + ctx, OORPackageDirectionIncoming, parentSession, parentArk, + parentCheckpoints, + ) + require.NoError(t, err) + + require.True( + t, len(parentArk.UnsignedTx.TxOut) >= 2, + "fixture must produce both recipient and anchor outputs", + ) + + anchorParentOutpoint := wire.OutPoint{ + Hash: parentSession, + Index: 1, + } + + childSession, childArk, childCheckpoints, childOutpoint, childScript, + childValue, _ := buildTestOORPackageWithInput( + t, 0xb2, anchorParentOutpoint, + ) + + err = store.UpsertPackage( + ctx, OORPackageDirectionIncoming, childSession, childArk, + childCheckpoints, + ) + require.NoError(t, err) + + seedBindingOutpoint( + t, ctx, roundStore, childOutpoint, childScript, childValue, + ) + + err = store.UpsertBinding( + ctx, childOutpoint, childSession, 0, + OORPackageLinkKindCreatedOutput, + ) + require.NoError(t, err) + + resolved, err := store.ResolveUnrollPackages(ctx, childOutpoint) + require.NoError(t, err) + require.NotNil(t, resolved) + + // The parent package exists and the txid binding is correct, but + // the referenced output is the anchor, so the fallback must refuse + // to graft it onto the unroll chain. + require.Len(t, resolved.Packages, 1) + require.Equal(t, childSession, resolved.Packages[0].SessionID) + require.Len(t, resolved.UnresolvedCheckpointInputs, 1) + require.Equal( + t, anchorParentOutpoint, resolved.UnresolvedCheckpointInputs[0], + ) +} From 230651fc02d2998cbe2c9589dc976b5efe73ff11 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 19:26:19 -0700 Subject: [PATCH 3/9] oor: Reject incomplete ancestry index on incoming OOR receive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateIncomingAncestry checked that each fragment's InputIndices was non-empty and within range, but did not reject duplicate indices or require the union of all fragments to cover every Ark tx input. An indexer/operator could return ancestry for only a subset of a multi-input OOR Ark tx — for example a two-input Ark tx with a single fragment naming only input 0 — and the descriptor would be accepted and persisted with incomplete recovery lineage. If the operator later disappears, the uncovered input has no rooted-path material for fraud-watch or unilateral-exit assembly, stranding the received VTXO. Track which input indices each fragment claims, reject duplicates at the malformed fragment, and require coverage of every Ark tx input before returning. Add direct table-driven coverage for the partition checks and a two-input materialization helper so the existing primary-ancestry normalization test exercises a genuine cross-round multi-input shape rather than a single-input PSBT with two fake fragments. Closes #374. --- oor/incoming_vtxo.go | 57 +++++++ oor/incoming_vtxo_test.go | 208 ++++++++++++++++++++++++-- oor/local_persistence_handler_test.go | 114 ++++++++++++++ 3 files changed, 369 insertions(+), 10 deletions(-) diff --git a/oor/incoming_vtxo.go b/oor/incoming_vtxo.go index 38b069ed4..126c4289f 100644 --- a/oor/incoming_vtxo.go +++ b/oor/incoming_vtxo.go @@ -242,6 +242,20 @@ func BuildIncomingVTXODescriptor(ark *psbt.Packet, // Ancestry contract for incoming OOR VTXOs (those are always // produced by an OOR Ark tx); an out-of-range index points at a // non-existent input so the unroll proof would never resolve. +// +// - No input index repeats within a fragment, and the union of all +// fragments' InputIndices covers every Ark tx input. Distinct +// fragments may name the same input when that OOR Ark input itself +// carries multi-fragment ancestry from an earlier chained receive. +// A missing index means at least one Ark tx input has no rooted-path +// material attached: fraud-watch plans (BuildWatchPlan) would lack +// a watch for the uncovered input's lineage and unilateral-exit +// proof assembly would have no fragment to broadcast for that +// input. The descriptor would persist cleanly and the gap would +// only surface if the operator later refuses cooperation, at which +// point the user is racing a CSV with no way to recover. We reject +// at the receive boundary so the bad indexer response fails before +// any funds are credited. func validateIncomingAncestry(meta IncomingVTXOMetadata, arkTxInputCount uint32) error { @@ -251,6 +265,11 @@ func validateIncomingAncestry(meta IncomingVTXOMetadata, } } + // Track whether at least one ancestry fragment serves each Ark tx + // input so we can verify coverage at the end. Sized to the Ark tx's + // declared input count; we only set entries after the per-fragment + // range check has passed, so out-of-range writes are impossible. + covered := make([]bool, arkTxInputCount) seen := make(map[chainhash.Hash]struct{}, len(meta.Ancestry)) hasPrimary := false for i, frag := range meta.Ancestry { @@ -307,6 +326,9 @@ func validateIncomingAncestry(meta IncomingVTXOMetadata, } } + seenInFragment := make( + map[uint32]struct{}, len(frag.InputIndices), + ) for j, idx := range frag.InputIndices { if idx >= arkTxInputCount { return &ErrInvalidAncestry{ @@ -319,6 +341,24 @@ func validateIncomingAncestry(meta IncomingVTXOMetadata, ), } } + + // Reject only duplicate indices within this + // fragment. Cross-fragment reuse is valid for a + // chained OOR receive where one Ark input is backed + // by multiple earlier commitment roots. + if _, ok := seenInFragment[idx]; ok { + return &ErrInvalidAncestry{ + Reason: fmt.Sprintf( + "fragment %d input index "+ + "[%d]=%d duplicates "+ + "an earlier index in "+ + "this fragment", + i, j, idx, + ), + } + } + seenInFragment[idx] = struct{}{} + covered[idx] = true } } @@ -332,6 +372,23 @@ func validateIncomingAncestry(meta IncomingVTXOMetadata, } } + // Every Ark tx input must be covered by at least one fragment. + // Accepting a missing input would leave the uncovered input with no + // rooted-path material for unilateral exit, stranding the received + // VTXO if the operator later refuses cooperation. + for idx, ok := range covered { + if !ok { + return &ErrInvalidAncestry{ + Reason: fmt.Sprintf( + "ark tx input %d is not covered by "+ + "any ancestry fragment "+ + "(incoming ancestry must "+ + "cover every input)", idx, + ), + } + } + } + return nil } diff --git a/oor/incoming_vtxo_test.go b/oor/incoming_vtxo_test.go index 62b914dd5..8482b1d76 100644 --- a/oor/incoming_vtxo_test.go +++ b/oor/incoming_vtxo_test.go @@ -4,6 +4,9 @@ import ( "testing" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/wire" + lib_tree "github.com/lightninglabs/darepo-client/lib/tree" + "github.com/lightninglabs/darepo-client/vtxo" "github.com/lightningnetwork/lnd/keychain" "github.com/stretchr/testify/require" ) @@ -81,17 +84,64 @@ func TestBuildIncomingVTXODescriptorZeroChainDepth(t *testing.T) { // cross-round multi-input metadata may carry the descriptor's commitment // fragment after another valid fragment, and descriptor construction still // preserves legacy Ancestry[0] primary semantics. +// +// The test exercises the genuine cross-round multi-input shape: a +// two-input Ark tx with one fragment per input. The secondary fragment +// is supplied first in the metadata so descriptor construction must +// reorder it before persistence. func TestBuildIncomingVTXODescriptorNormalizesPrimaryAncestry(t *testing.T) { t.Parallel() - arkPSBT, _, recipients, commitHash, recipientKey, - operatorKey := buildTestIncomingMaterialization(t) + arkPSBT, _, recipients, commits, recipientKey, + operatorKey := buildTestIncomingMaterializationMultiInput(t) - otherHash := chainhash.Hash{0xee} - ancestry := validTestIncomingAncestry(otherHash) - ancestry = append( - ancestry, validTestIncomingAncestry(commitHash)[0], - ) + // BuildArkPSBT applies BIP69 input ordering, so locate which + // of the two commitment hashes ends up at Ark input index 0 + // vs 1 in the canonical PSBT. Each fragment must name the + // input it actually serves. + indexOf := func(h chainhash.Hash) uint32 { + for i, in := range arkPSBT.UnsignedTx.TxIn { + if in.PreviousOutPoint.Hash == h { + return uint32(i) + } + } + t.Fatalf("commit %s not found in ark inputs", h) + + return 0 + } + primaryCommit := commits[0] + secondaryCommit := commits[1] + + ancestry := []vtxo.Ancestry{ + // Secondary fragment first — descriptor construction must + // re-order so Ancestry[0] is the primary commitment. + { + TreePath: &lib_tree.Tree{ + Root: &lib_tree.Node{}, + BatchOutpoint: wire.OutPoint{ + Hash: secondaryCommit, + }, + }, + CommitmentTxID: secondaryCommit, + InputIndices: []uint32{ + indexOf(secondaryCommit), + }, + TreeDepth: 1, + }, + { + TreePath: &lib_tree.Tree{ + Root: &lib_tree.Node{}, + BatchOutpoint: wire.OutPoint{ + Hash: primaryCommit, + }, + }, + CommitmentTxID: primaryCommit, + InputIndices: []uint32{ + indexOf(primaryCommit), + }, + TreeDepth: 1, + }, + } desc, err := BuildIncomingVTXODescriptor(arkPSBT, IncomingVTXOConfig{ @@ -103,7 +153,7 @@ func TestBuildIncomingVTXODescriptorNormalizesPrimaryAncestry(t *testing.T) { ExitDelay: 10, Metadata: IncomingVTXOMetadata{ RoundID: "test-round", - CommitmentTxID: commitHash, + CommitmentTxID: primaryCommit, BatchExpiry: 1000, ChainDepth: 1, CreatedHeight: 500, @@ -113,8 +163,8 @@ func TestBuildIncomingVTXODescriptorNormalizesPrimaryAncestry(t *testing.T) { ) require.NoError(t, err) require.Len(t, desc.Ancestry, 2) - require.Equal(t, commitHash, desc.Ancestry[0].CommitmentTxID) - require.Equal(t, otherHash, desc.Ancestry[1].CommitmentTxID) + require.Equal(t, primaryCommit, desc.Ancestry[0].CommitmentTxID) + require.Equal(t, secondaryCommit, desc.Ancestry[1].CommitmentTxID) } // TestBuildIncomingVTXODescriptorRejectsNilArk verifies that a nil Ark @@ -248,3 +298,141 @@ func TestBuildIncomingVTXODescriptorRejectsInvalidAncestry(t *testing.T) { }) } } + +// TestValidateIncomingAncestryInputCoverage exercises the InputIndices +// partition checks for multi-input Ark transactions. The other rejection +// branches are covered via BuildIncomingVTXODescriptor in +// TestBuildIncomingVTXODescriptorRejectsInvalidAncestry; here we drive +// validateIncomingAncestry directly so we can vary arkTxInputCount +// without rebuilding a real PSBT. +// +// The scenarios assert two properties that the receive boundary must +// enforce so that a malicious or truncated indexer response cannot +// strand received OOR funds: +// +// - The union of all fragments' InputIndices covers every Ark tx +// input (0..arkTxInputCount-1). +// - No input index appears in more than one fragment (or twice +// within a single fragment), since a duplicate hides a missing +// fragment behind apparently-full coverage. +func TestValidateIncomingAncestryInputCoverage(t *testing.T) { + t.Parallel() + + primary := chainhash.Hash{0x01} + secondary := chainhash.Hash{0x02} + + fragment := func(commit chainhash.Hash, + indices ...uint32) vtxo.Ancestry { + + return vtxo.Ancestry{ + TreePath: &lib_tree.Tree{ + Root: &lib_tree.Node{}, + BatchOutpoint: wire.OutPoint{ + Hash: commit, + }, + }, + CommitmentTxID: commit, + InputIndices: append( + []uint32(nil), indices..., + ), + TreeDepth: 1, + } + } + + cases := []struct { + name string + arkTxInputCount uint32 + ancestry []vtxo.Ancestry + wantReason string + }{ + { + name: "single fragment covers single input", + arkTxInputCount: 1, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0), + }, + }, + { + name: "two fragments partition two inputs", + arkTxInputCount: 2, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0), + fragment(secondary, 1), + }, + }, + { + name: "single fragment covers both inputs", + arkTxInputCount: 2, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0, 1), + }, + }, + { + name: "chained input may have two fragments", + arkTxInputCount: 1, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0), + fragment(secondary, 0), + }, + }, + { + name: "missing coverage truncated fragment", + arkTxInputCount: 2, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0), + }, + wantReason: "ark tx input 1 is not covered", + }, + { + name: "missing coverage gap mid range", + arkTxInputCount: 3, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0, 2), + }, + wantReason: "ark tx input 1 is not covered", + }, + { + name: "duplicate within fragment", + arkTxInputCount: 2, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0, 0), + }, + wantReason: "duplicates an earlier index", + }, + { + name: "cross duplicate misses input", + arkTxInputCount: 2, + ancestry: []vtxo.Ancestry{ + fragment(primary, 0), + fragment(secondary, 0), + }, + wantReason: "ark tx input 1 is not covered", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + meta := IncomingVTXOMetadata{ + RoundID: "test-round", + CommitmentTxID: primary, + BatchExpiry: 1000, + ChainDepth: 1, + CreatedHeight: 500, + Ancestry: tc.ancestry, + } + + err := validateIncomingAncestry( + meta, tc.arkTxInputCount, + ) + + if tc.wantReason == "" { + require.NoError(t, err) + + return + } + require.Error(t, err) + require.ErrorIs(t, err, &ErrInvalidAncestry{}) + require.Contains(t, err.Error(), tc.wantReason) + }) + } +} diff --git a/oor/local_persistence_handler_test.go b/oor/local_persistence_handler_test.go index c115f0e5f..839ed1753 100644 --- a/oor/local_persistence_handler_test.go +++ b/oor/local_persistence_handler_test.go @@ -1314,6 +1314,120 @@ func reparentTestIncomingPackage(t *testing.T, arkPSBT *psbt.Packet, return arkPSBT, checkpoints } +// buildTestIncomingMaterializationMultiInput is the two-checkpoint +// variant of buildTestIncomingMaterialization. It returns an Ark PSBT +// spending two distinct checkpoint inputs (so len(arkPSBT.UnsignedTx.TxIn) +// == 2). Cross-round multi-input OOR receive coverage exercises +// validateIncomingAncestry's partition checks, which require the union +// of all fragments' InputIndices to cover every Ark input — a property +// that cannot be exercised against the single-input helper. +// +// The two commitment txids returned correspond to inputs[0] and +// inputs[1] respectively; callers stitch them into two-fragment +// IncomingVTXOMetadata.Ancestry slices. +func buildTestIncomingMaterializationMultiInput(t *testing.T) (*psbt.Packet, + []*psbt.Packet, []ArkRecipientOutput, [2]chainhash.Hash, + *btcec.PrivateKey, *btcec.PublicKey) { + + t.Helper() + + operatorKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + policy := arkscript.CheckpointPolicy{ + OperatorKey: operatorKey.PubKey(), + CSVDelay: 10, + } + + recipientKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + // Two independent checkpoint inputs anchored to distinct + // upstream Ark txids so the produced Ark tx has two inputs, + // each contributable by a different ancestry fragment. + inputAmt := btcutil.Amount(5_000) + makeInput := func(seed byte) oortx.CheckpointInput { + return oortx.CheckpointInput{ + SpentVTXO: oortx.SpentVTXORef{ + Outpoint: wire.OutPoint{ + Hash: [32]byte{ + seed, + }, + Index: 0, + }, + Output: &wire.TxOut{ + Value: int64(inputAmt), + PkScript: newTestTaprootPkScript( + t, operatorKey.PubKey(), + ), + }, + }, + OwnerLeafScript: []byte{ + 0x51, + }, + } + } + inputs := []oortx.CheckpointInput{ + makeInput(0x11), makeInput(0x22), + } + + cp0, err := oortx.BuildCheckpointPSBT(policy, inputs[0]) + require.NoError(t, err) + + cp1, err := oortx.BuildCheckpointPSBT(policy, inputs[1]) + require.NoError(t, err) + + vtxoTapKey, err := arkscript.VTXOTapKey( + recipientKey.PubKey(), policy.OperatorKey, 10, + ) + require.NoError(t, err) + + recipientPkScript, err := txscript.PayToTaprootScript(vtxoTapKey) + require.NoError(t, err) + + outputs := []oortx.RecipientOutput{ + { + PkScript: recipientPkScript, + Value: inputAmt * 2, + }, + } + + arkPSBT, err := oortx.BuildArkPSBT( + []oortx.CheckpointOutput{ + { + Txid: cp0.PSBT.UnsignedTx.TxHash(), + Output: cp0.PSBT.UnsignedTx.TxOut[0], + TapTreeEncoded: cp0.TapTreeEncoded, + }, + { + Txid: cp1.PSBT.UnsignedTx.TxHash(), + Output: cp1.PSBT.UnsignedTx.TxOut[0], + TapTreeEncoded: cp1.TapTreeEncoded, + }, + }, + outputs, + ) + require.NoError(t, err) + + recipients, err := ExtractArkRecipients(arkPSBT) + require.NoError(t, err) + + // Use the checkpoint tx ids as the per-fragment "commitment" + // txids so that callers can name a real Ark-tx input prevout + // for each fragment. (Ark inputs reference checkpoint tx ids, + // not the upstream SpentVTXO outpoint hashes.) The validator + // only requires that BatchOutpoint.Hash matches CommitmentTxID + // across the per-fragment cross-check; it does not interpret + // the commitment txid itself. + commits := [2]chainhash.Hash{ + cp0.PSBT.UnsignedTx.TxHash(), + cp1.PSBT.UnsignedTx.TxHash(), + } + + return arkPSBT, []*psbt.Packet{cp0.PSBT, cp1.PSBT}, recipients, + commits, recipientKey, operatorKey.PubKey() +} + // validTestIncomingAncestry returns a minimal Ancestry slice that passes // BuildIncomingVTXODescriptor's structural cross-check, anchored at the // supplied commitment txid. The test ark PSBT built by From 1321eb4d09b153174e803d1bab3d2e3cfa50deee Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 19:35:38 -0700 Subject: [PATCH 4/9] unroll: Keep restore failures retryable, restore via Ensure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry used to mark each non-terminal job PhaseFailed when RestoreNonTerminal could not spawn or resume its child actor, and handleEnsure short-circuited on any existing record. Resume can fail for transient external dependencies — chain backend SubscribeBlocks / RegisterSpend through ChainSource, or a flaky DB. Once the record was marked terminal, ListNonTerminalRecords skipped it on every subsequent boot, turning a transient outage during daemon start into a permanent shutdown of the recovery path for that VTXO. For a VTXO that is in unilateral_exit and near expiry, that translates into locked or lost funds. This commit makes restore failures non-sticky on two paths: 1. restoreNonTerminal no longer calls MarkTerminal on spawn / resume failure. The durable record stays non-terminal, is logged, and gets retried on the next daemon restart and on any in-boot EnsureUnrollRequest for the same outpoint. 2. handleEnsure detects "non-terminal store record, no active child" — the signature of a previous restore that did not wire up — and attempts an inline restore via a shared tryRestoreOne helper. Success returns Created=false with the historical ActorID; failure surfaces the error so the caller can retry. Three new tests lock the behavior in: - TestRegistryRestoreFailureLeavesRecordRetryable: first RestoreNonTerminal fails on resume, record stays non-terminal, a second RestoreNonTerminal with a healthy spawn succeeds. - TestRegistryEnsureRestoresFailedNonTerminalRecord: a fresh EnsureUnroll on an unrestored non-terminal record triggers inline restore. - TestRegistryEnsureRetriesAfterInlineRestoreFailure: a failed inline restore inside Ensure does not strand the job; the next Ensure retries. Closes #381. --- unroll/registry.go | 227 ++++++++++++++++--- unroll/registry_test.go | 466 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 662 insertions(+), 31 deletions(-) diff --git a/unroll/registry.go b/unroll/registry.go index 5b5584f75..bb108f63e 100644 --- a/unroll/registry.go +++ b/unroll/registry.go @@ -135,12 +135,34 @@ func (a *UnrollRegistryActor) Ref() actor.ActorRef[RegistryMsg, RegistryResp] { } // RestoreNonTerminal resumes all non-terminal records from the control store. +// +// The actual restore runs inside the registry actor's goroutine via a +// restoreNonTerminalMsg, so all mutations of r.active and r.pending stay +// serialized with concurrent Receive turns (handleEnsure / handleGetStatus +// can already be running by the time the daemon boot path reaches this +// call, because NewUnrollRegistryActor has already Start()ed the actor). func (a *UnrollRegistryActor) RestoreNonTerminal(ctx context.Context) error { - if a == nil || a.behavior == nil { + if a == nil || a.ref == nil { return fmt.Errorf("registry actor not initialized") } - return a.behavior.restoreNonTerminal(ctx) + resp, err := a.ref.Ask( + ctx, &restoreNonTerminalMsg{}, + ).Await(ctx).Unpack() + if err != nil { + return err + } + + result, ok := resp.(*restoreNonTerminalResp) + if !ok { + return fmt.Errorf("unexpected restore response %T", resp) + } + + if result.Err != "" { + return fmt.Errorf("%s", result.Err) + } + + return nil } // Stop stops the underlying registry actor. @@ -237,6 +259,40 @@ func (m *persistRecordResultMsg) MessageType() string { // registryMsgSealed seals persistRecordResultMsg into the registry surface. func (m *persistRecordResultMsg) registryMsgSealed() {} +// restoreNonTerminalMsg drives the boot-time restore of every non-terminal +// record through the registry actor's goroutine. Sending it via Ask keeps +// all mutations of r.active and r.pending serialized with concurrent +// Receive turns (handleEnsure / handleGetStatus can already be running by +// the time the daemon boot path issues this call). +type restoreNonTerminalMsg struct { + actor.BaseMessage +} + +// MessageType returns the stable message type identifier. +func (m *restoreNonTerminalMsg) MessageType() string { + return "restoreNonTerminalMsg" +} + +// registryMsgSealed seals restoreNonTerminalMsg into the registry surface. +func (m *restoreNonTerminalMsg) registryMsgSealed() {} + +// restoreNonTerminalResp carries the outcome of a boot-time restore back +// to the caller of UnrollRegistryActor.RestoreNonTerminal. +type restoreNonTerminalResp struct { + actor.BaseMessage + + // Err is populated when the restore returned an error. + Err string +} + +// MessageType returns the stable message type identifier. +func (m *restoreNonTerminalResp) MessageType() string { + return "restoreNonTerminalResp" +} + +// registryRespSealed seals restoreNonTerminalResp into the registry surface. +func (m *restoreNonTerminalResp) registryRespSealed() {} + // Receive processes one registry message. func (r *registryBehavior) Receive(ctx context.Context, msg RegistryMsg) fn.Result[RegistryResp] { @@ -257,6 +313,9 @@ func (r *registryBehavior) Receive(ctx context.Context, case *persistRecordResultMsg: return r.handlePersistRecordResult(ctx, req) + case *restoreNonTerminalMsg: + return r.handleRestoreNonTerminal(ctx) + default: return fn.Err[RegistryResp]( fmt.Errorf("unknown registry message: %T", msg), @@ -332,6 +391,54 @@ func (r *registryBehavior) handleEnsure(ctx context.Context, ) } if existing != nil { + // A durable record exists but no child is live for it. Two + // sub-cases: + // + // 1. Terminal record (Completed/Failed) — return the + // historical ActorID so callers see a stable identity + // and do not clobber the recorded sweep txid or + // failure reason. + // + // 2. Non-terminal record — the actor was admitted in a + // previous boot but never resumed (e.g. RestoreNonTerminal + // hit a transient ChainSource error). Attempt an inline + // restore so a fresh Ensure from the chain resolver or + // RPC layer can recover from a transient failure on the + // previous boot. If restore fails again, surface the + // error so the caller can retry; the durable record + // stays non-terminal and will be retried on the next + // Ensure / next daemon restart. + if !existing.IsTerminal() { + height, err := r.queryBestHeight(ctx) + if err != nil { + return fn.Err[RegistryResp]( + fmt.Errorf("best height for "+ + "restore: %w", err), + ) + } + + child, err := r.tryRestoreOne(ctx, *existing, height) + if err != nil { + return fn.Err[RegistryResp]( + fmt.Errorf("restore existing "+ + "record: %w", err), + ) + } + + r.active[req.Outpoint] = child + + // Mirror the historical record into r.pending so + // handleTerminated can carry over Trigger / ActorID + // without an extra store lookup, and so handleGetStatus + // answers from cache while the child runs. + r.pending[req.Outpoint] = cloneRegistryRecord(*existing) + + return fn.Ok[RegistryResp](&EnsureUnrollResp{ + ActorID: child.Ref().ID(), + Created: false, + }) + } + return fn.Ok[RegistryResp](&EnsureUnrollResp{ ActorID: existing.ActorID, Created: false, @@ -717,10 +824,19 @@ func stopChildAfterDrain(child *VTXOUnrollActor) { }() } -// restoreNonTerminal is the daemon's boot entry point for the unroll -// subsystem. It reads every record from the durable store that is not -// already Completed or Failed, spawns a fresh VTXOUnrollActor per -// target, and sends ResumeUnrollRequest to each. +// handleRestoreNonTerminal is the daemon's boot entry point for the +// unroll subsystem, dispatched through the registry actor's Receive loop +// so it shares the same goroutine as handleEnsure / handleGetStatus. +// +// Running inside the actor turn is what makes the r.active / r.pending +// mutations below race-free: NewUnrollRegistryActor calls Start() before +// the boot path issues the first restore, so by the time we get here the +// actor may already have processed concurrent Ensure / GetStatus +// messages from the chain resolver or RPC layer. +// +// It reads every record from the durable store that is not already +// Completed or Failed, spawns a fresh VTXOUnrollActor per target, and +// sends ResumeUnrollRequest to each. // // The per-target behavior then loads its checkpoint (proof, planner // state, sweep tx, last height), reconstructs the FSM in the same state @@ -731,23 +847,46 @@ func stopChildAfterDrain(child *VTXOUnrollActor) { // already-confirmed ones return immediately with their status. // // When restore fails for an individual target (spawn fails, or the -// resume Ask fails), we mark that target terminal with PhaseFailed and -// a descriptive reason rather than leaving the store entry non-terminal -// forever. A fresh Ensure from the chain resolver can then try again -// with a clean slate if the cause is transient. -func (r *registryBehavior) restoreNonTerminal(ctx context.Context) error { +// resume Ask fails), we leave the durable record non-terminal so that: +// +// - the next daemon restart will retry the restore from a clean +// slate when the transient cause is gone (e.g. a chain backend +// outage that prevented SubscribeBlocks / RegisterSpend on the +// previous boot), and +// +// - a fresh EnsureUnrollRequest for the same outpoint within the +// current boot will attempt an inline restore via handleEnsure +// (which detects "non-terminal record, no active child" and calls +// tryRestoreOne). +// +// Marking the record terminal on a transient restore failure would +// strand a recovery-critical job: ListNonTerminalRecords would skip it +// on every subsequent boot and handleEnsure would short-circuit on the +// terminal record. For a VTXO that is in unilateral_exit and near +// expiry, that translates into locked or lost funds — see issue #381. +func (r *registryBehavior) handleRestoreNonTerminal( + ctx context.Context) fn.Result[RegistryResp] { + records, err := r.cfg.Store.ListNonTerminalRecords(ctx) if err != nil { - return fmt.Errorf("list non-terminal records: %w", err) + return fn.Ok[RegistryResp](&restoreNonTerminalResp{ + Err: fmt. + Errorf("list non-terminal records: %w", err). + Error(), + }) } if len(records) == 0 { - return nil + return fn.Ok[RegistryResp](&restoreNonTerminalResp{}) } height, err := r.queryBestHeight(ctx) if err != nil { - return fmt.Errorf("best height for restore: %w", err) + return fn.Ok[RegistryResp](&restoreNonTerminalResp{ + Err: fmt. + Errorf("best height for restore: %w", err). + Error(), + }) } for i := range records { @@ -756,33 +895,59 @@ func (r *registryBehavior) restoreNonTerminal(ctx context.Context) error { continue } - child, err := r.spawn(ctx, record.TargetOutpoint) + child, err := r.tryRestoreOne(ctx, record, height) if err != nil { - _ = r.cfg.Store.MarkTerminal( - ctx, record.TargetOutpoint, PhaseFailed, - "spawn failed on restore: "+err.Error(), nil, + // Leave the record non-terminal so the next boot + // or the next EnsureUnrollRequest can retry. Log + // loudly: a persistent restore failure is a real + // problem even though it is recoverable. + r.log.WarnS(ctx, "Failed to restore unroll job; "+ + "record left non-terminal for retry", err, + slog.String( + "outpoint", + record.TargetOutpoint.String(), + ), + slog.String("actor_id", record.ActorID), ) continue } - _, err = child.Ref().Ask(ctx, &ResumeUnrollRequest{ - Height: height, - }).Await(ctx).Unpack() - if err != nil { - child.Stop() - _ = r.cfg.Store.MarkTerminal( - ctx, record.TargetOutpoint, PhaseFailed, - "resume failed on restore: "+err.Error(), nil, - ) + r.active[record.TargetOutpoint] = child - continue - } + // Mirror the historical record into r.pending so + // handleTerminated can carry over Trigger / ActorID + // without an extra store lookup, and so handleGetStatus + // answers from cache while the restored child runs. + r.pending[record.TargetOutpoint] = cloneRegistryRecord(record) + } - r.active[record.TargetOutpoint] = child + return fn.Ok[RegistryResp](&restoreNonTerminalResp{}) +} + +// tryRestoreOne spawns a fresh per-target actor for one non-terminal +// record and sends it a ResumeUnrollRequest. On any error the spawned +// child is stopped and the durable record is left untouched so the +// caller can retry (either via a future EnsureUnrollRequest or on the +// next daemon restart). +func (r *registryBehavior) tryRestoreOne(ctx context.Context, + record RegistryRecord, height int32) (*VTXOUnrollActor, error) { + + child, err := r.spawn(ctx, record.TargetOutpoint) + if err != nil { + return nil, fmt.Errorf("spawn failed on restore: %w", err) } - return nil + _, err = child.Ref().Ask(ctx, &ResumeUnrollRequest{ + Height: height, + }).Await(ctx).Unpack() + if err != nil { + child.Stop() + + return nil, fmt.Errorf("resume failed on restore: %w", err) + } + + return child, nil } // handlePersistActiveRecord is half of the two-message pair that drives diff --git a/unroll/registry_test.go b/unroll/registry_test.go index 6a1749899..8a5eaa9c5 100644 --- a/unroll/registry_test.go +++ b/unroll/registry_test.go @@ -1325,6 +1325,472 @@ func TestRegistryTerminalStatusRemainsQueryableWhilePersistBlocked( }, testTimeout, 10*time.Millisecond) } +// TestRegistryRestoreFailureLeavesRecordRetryable verifies that a +// transient failure during RestoreNonTerminal does NOT mark the durable +// record terminal: a subsequent RestoreNonTerminal call (e.g. on the +// next daemon boot, after the transient ChainSource / DB issue is +// resolved) must still find and resume the job. This is the regression +// guard for issue #381 ("Restore failure permanently disables unroll +// recovery"): an attacker or backend outage that fails the resume Ask +// on one boot must not strand a recovery-critical job forever. +func TestRegistryRestoreFailureLeavesRecordRetryable(t *testing.T) { + proof := buildLinearProof(t) + desc := testDescriptor(t, proof.TargetOutpoint(), proof.CSVDelay()) + store := newMemRegistryStore() + checkpoints := newMemCheckpointStore() + txconfirmRef := &fakeTxConfirmRef{} + + actorID := actorIDForTarget(proof.TargetOutpoint()) + err := store.UpsertRecord(t.Context(), RegistryRecord{ + TargetOutpoint: proof.TargetOutpoint(), + ActorID: actorID, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + require.NoError(t, err) + + registry := newRegistryHarnessWithSpawn(t, RegistryConfig{ + Store: store, + DeliveryStore: checkpoints, + ProofAssembler: &mockProofAssembler{proof: proof}, + VTXOStore: &mockVTXOStore{desc: desc}, + TxConfirmRef: txconfirmRef, + ChainSource: &fakeRegistryChainSourceRef{height: 201}, + Wallet: &fakeSweepWallet{}, + }) + t.Cleanup(registry.Stop) + + // First boot: install a spawnFunc that returns a child whose + // ResumeUnrollRequest fails (simulating a transient ChainSource + // outage on SubscribeBlocks / RegisterSpend during resume). + var attempts atomic.Int32 + failingSpawn := func(_ context.Context, target wire.OutPoint) ( + *VTXOUnrollActor, error) { + + attempts.Add(1) + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + if _, ok := msg.(*ResumeUnrollRequest); ok { + err := errors.New("transient chain " + + "outage") + + return fn.Err[Resp](err) + } + + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", msg), + ) + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + registry.behavior.spawnFunc = failingSpawn + + // RestoreNonTerminal must NOT return an error and must NOT mark + // the record terminal; the durable record stays non-terminal so a + // future retry path can pick it up. + err = registry.RestoreNonTerminal(t.Context()) + require.NoError(t, err) + require.EqualValues(t, 1, attempts.Load()) + + record, err := store.GetRecord(t.Context(), proof.TargetOutpoint()) + require.NoError(t, err) + require.NotNil(t, record) + require.False( + t, record.IsTerminal(), + "restore failure must leave record non-terminal", + ) + require.Equal(t, PhaseMaterializing, record.Phase) + + // Second boot path: swap in a healthy spawnFunc that completes + // the ResumeUnrollRequest and verify RestoreNonTerminal now + // succeeds. The durable record must still be visible to + // ListNonTerminalRecords. + healthySpawn := func(_ context.Context, target wire.OutPoint) ( + *VTXOUnrollActor, error) { + + attempts.Add(1) + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + switch msg.(type) { + case *ResumeUnrollRequest: + return fn.Ok[Resp](&AckResp{}) + + case *GetStateRequest: + return fn.Ok[Resp](&GetStateResp{ + Started: true, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + + default: + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", + msg), + ) + } + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + registry.behavior.spawnFunc = healthySpawn + + err = registry.RestoreNonTerminal(t.Context()) + require.NoError(t, err) + require.EqualValues( + t, 2, attempts.Load(), + "second RestoreNonTerminal must respawn the child", + ) + + // The job is now active and visible via GetStatus. + resp, err := registry.Ref().Ask(t.Context(), &GetStatusRequest{ + Outpoint: proof.TargetOutpoint(), + }).Await(t.Context()).Unpack() + require.NoError(t, err) + + status, ok := resp.(*GetStatusResp) + require.True(t, ok) + require.True(t, status.Found) + require.True(t, status.Active) + require.Equal(t, PhaseMaterializing, status.Phase) +} + +// TestRegistryEnsureRestoresFailedNonTerminalRecord verifies that when a +// non-terminal record exists in the durable store but no child is +// active (because a prior RestoreNonTerminal hit a transient error and +// left the record retryable), a fresh EnsureUnrollRequest from the +// chain resolver or RPC layer kicks off an inline restore instead of +// silently returning Created=false with a dormant job. This is the +// second half of the issue #381 fix: handleEnsure used to short-circuit +// on any existing record without checking whether the in-memory child +// was actually live. +func TestRegistryEnsureRestoresFailedNonTerminalRecord(t *testing.T) { + proof := buildLinearProof(t) + desc := testDescriptor(t, proof.TargetOutpoint(), proof.CSVDelay()) + store := newMemRegistryStore() + checkpoints := newMemCheckpointStore() + txconfirmRef := &fakeTxConfirmRef{} + + actorID := actorIDForTarget(proof.TargetOutpoint()) + err := store.UpsertRecord(t.Context(), RegistryRecord{ + TargetOutpoint: proof.TargetOutpoint(), + ActorID: actorID, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + require.NoError(t, err) + + registry := newRegistryHarnessWithSpawn(t, RegistryConfig{ + Store: store, + DeliveryStore: checkpoints, + ProofAssembler: &mockProofAssembler{proof: proof}, + VTXOStore: &mockVTXOStore{desc: desc}, + TxConfirmRef: txconfirmRef, + ChainSource: &fakeRegistryChainSourceRef{height: 201}, + Wallet: &fakeSweepWallet{}, + }) + t.Cleanup(registry.Stop) + + // Force the prior RestoreNonTerminal to "fail" by simply skipping + // it: leave the store record in place but never wire up an active + // child. EnsureUnroll must detect the gap and restore inline. + var resumes atomic.Int32 + registry.behavior.spawnFunc = func(_ context.Context, + target wire.OutPoint) (*VTXOUnrollActor, error) { + + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + switch msg.(type) { + case *ResumeUnrollRequest: + resumes.Add(1) + + return fn.Ok[Resp](&AckResp{}) + + case *GetStateRequest: + return fn.Ok[Resp](&GetStateResp{ + Started: true, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + + default: + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", + msg), + ) + } + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + + // Caller asks Ensure for the same outpoint. The pre-existing + // non-terminal record + no active child must trigger inline + // restore via ResumeUnrollRequest. Created=false because the + // job was admitted in a previous boot. + resp, err := registry.Ref().Ask(t.Context(), &EnsureUnrollRequest{ + Outpoint: proof.TargetOutpoint(), + Trigger: TriggerCriticalExpiry, + }).Await(t.Context()).Unpack() + require.NoError(t, err) + + ensureResp, ok := resp.(*EnsureUnrollResp) + require.True(t, ok) + require.False( + t, ensureResp.Created, + "existing record must surface as Created=false", + ) + require.Equal(t, actorID, ensureResp.ActorID) + require.EqualValues( + t, 1, resumes.Load(), + "EnsureUnroll on a non-terminal record with no active "+ + "child must trigger inline resume", + ) + + // The job is now active. + statusResp, err := registry.Ref().Ask(t.Context(), &GetStatusRequest{ + Outpoint: proof.TargetOutpoint(), + }).Await(t.Context()).Unpack() + require.NoError(t, err) + + status, ok := statusResp.(*GetStatusResp) + require.True(t, ok) + require.True(t, status.Active) +} + +// TestRegistryEnsureRetriesAfterInlineRestoreFailure verifies that an +// inline restore failure inside handleEnsure does not strand the job: +// a subsequent EnsureUnroll on the same outpoint must attempt restore +// again rather than short-circuiting on the dormant non-terminal +// record. Combined with the no-mark-terminal behavior in +// restoreNonTerminal, this means a transient backend outage is fully +// recoverable both on the next boot AND via a follow-up Ensure within +// the same boot. +func TestRegistryEnsureRetriesAfterInlineRestoreFailure(t *testing.T) { + proof := buildLinearProof(t) + desc := testDescriptor(t, proof.TargetOutpoint(), proof.CSVDelay()) + store := newMemRegistryStore() + checkpoints := newMemCheckpointStore() + txconfirmRef := &fakeTxConfirmRef{} + + actorID := actorIDForTarget(proof.TargetOutpoint()) + err := store.UpsertRecord(t.Context(), RegistryRecord{ + TargetOutpoint: proof.TargetOutpoint(), + ActorID: actorID, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + require.NoError(t, err) + + registry := newRegistryHarnessWithSpawn(t, RegistryConfig{ + Store: store, + DeliveryStore: checkpoints, + ProofAssembler: &mockProofAssembler{proof: proof}, + VTXOStore: &mockVTXOStore{desc: desc}, + TxConfirmRef: txconfirmRef, + ChainSource: &fakeRegistryChainSourceRef{height: 201}, + Wallet: &fakeSweepWallet{}, + }) + t.Cleanup(registry.Stop) + + // First Ensure: ResumeUnrollRequest fails (transient). + var attempts atomic.Int32 + registry.behavior.spawnFunc = func(_ context.Context, + target wire.OutPoint) (*VTXOUnrollActor, error) { + + attempts.Add(1) + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + if _, ok := msg.(*ResumeUnrollRequest); ok { + return fn.Err[Resp]( + errors.New( + "transient resume " + + "failure"), + ) + } + + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", msg), + ) + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + + _, err = registry.Ref().Ask(t.Context(), &EnsureUnrollRequest{ + Outpoint: proof.TargetOutpoint(), + Trigger: TriggerCriticalExpiry, + }).Await(t.Context()).Unpack() + require.Error(t, err) + require.Contains(t, err.Error(), "restore existing record") + + // The durable record must NOT have been marked terminal by the + // failed inline restore. + record, err := store.GetRecord(t.Context(), proof.TargetOutpoint()) + require.NoError(t, err) + require.NotNil(t, record) + require.False(t, record.IsTerminal()) + + // Second Ensure with a healthy spawn must succeed. + registry.behavior.spawnFunc = func(_ context.Context, + target wire.OutPoint) (*VTXOUnrollActor, error) { + + attempts.Add(1) + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + switch msg.(type) { + case *ResumeUnrollRequest: + return fn.Ok[Resp](&AckResp{}) + + case *GetStateRequest: + return fn.Ok[Resp](&GetStateResp{ + Started: true, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + + default: + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", + msg), + ) + } + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + + resp, err := registry.Ref().Ask(t.Context(), &EnsureUnrollRequest{ + Outpoint: proof.TargetOutpoint(), + Trigger: TriggerCriticalExpiry, + }).Await(t.Context()).Unpack() + require.NoError(t, err) + + ensureResp, ok := resp.(*EnsureUnrollResp) + require.True(t, ok) + require.False(t, ensureResp.Created) + require.EqualValues(t, 2, attempts.Load()) +} + +// TestRegistryRestoreNonTerminalDispatchedThroughActor verifies that +// RestoreNonTerminal serializes its r.active / r.pending mutations with +// concurrent Ensure / GetStatus traffic by going through the registry +// actor's Receive loop. Running this test under -race is the actual +// guard: any direct mutation outside the actor goroutine while another +// Ensure is in flight would trip the detector. +func TestRegistryRestoreNonTerminalDispatchedThroughActor(t *testing.T) { + proof := buildLinearProof(t) + desc := testDescriptor(t, proof.TargetOutpoint(), proof.CSVDelay()) + store := newMemRegistryStore() + checkpoints := newMemCheckpointStore() + txconfirmRef := &fakeTxConfirmRef{} + + actorID := actorIDForTarget(proof.TargetOutpoint()) + err := store.UpsertRecord(t.Context(), RegistryRecord{ + TargetOutpoint: proof.TargetOutpoint(), + ActorID: actorID, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + require.NoError(t, err) + + registry := newRegistryHarnessWithSpawn(t, RegistryConfig{ + Store: store, + DeliveryStore: checkpoints, + ProofAssembler: &mockProofAssembler{proof: proof}, + VTXOStore: &mockVTXOStore{desc: desc}, + TxConfirmRef: txconfirmRef, + ChainSource: &fakeRegistryChainSourceRef{height: 201}, + Wallet: &fakeSweepWallet{}, + }) + t.Cleanup(registry.Stop) + + registry.behavior.spawnFunc = func(_ context.Context, + target wire.OutPoint) (*VTXOUnrollActor, error) { + + behavior := actor.NewFunctionBehavior( + func(_ context.Context, msg Msg) fn.Result[Resp] { + switch msg.(type) { + case *ResumeUnrollRequest: + return fn.Ok[Resp](&AckResp{}) + + case *StartUnrollRequest: + return fn.Ok[Resp](&AckResp{}) + + case *GetStateRequest: + return fn.Ok[Resp](&GetStateResp{ + Started: true, + Trigger: TriggerRestart, + Phase: PhaseMaterializing, + }) + + default: + return fn.Err[Resp]( + fmt.Errorf("unexpected msg %T", + msg), + ) + } + }, + ) + + // Test children are owned by t.Cleanup after creation. + //nolint:contextcheck + return newTestUnrollChild(t, target, behavior), nil + } + + // Drive RestoreNonTerminal and a concurrent GetStatus probe at the + // same time. Both end up in the actor mailbox; -race catches any + // behavior-side state still mutated outside the goroutine. + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + + require.NoError(t, registry.RestoreNonTerminal(t.Context())) + }() + + go func() { + defer wg.Done() + + // A GetStatus probe is a read-only message that lands on the + // same mailbox, so the registry serializes it against the + // restore turn. + _, _ = registry.Ref().Ask(t.Context(), &GetStatusRequest{ + Outpoint: proof.TargetOutpoint(), + }).Await(t.Context()).Unpack() + }() + + wg.Wait() + + // After restore, the record is active. + resp, err := registry.Ref().Ask(t.Context(), &GetStatusRequest{ + Outpoint: proof.TargetOutpoint(), + }).Await(t.Context()).Unpack() + require.NoError(t, err) + + status, ok := resp.(*GetStatusResp) + require.True(t, ok) + require.True(t, status.Found) + require.True(t, status.Active) + require.Equal(t, PhaseMaterializing, status.Phase) +} + var _ RegistryStore = (*memRegistryStore)(nil) var _ RegistryStore = (*flakyRegistryStore)(nil) var _ RegistryStore = (*terminalFlakyRegistryStore)(nil) From ec197b2526c819eb864273cd10554faa5b70064e Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 20:52:05 -0700 Subject: [PATCH 5/9] round: Enforce fee cap against realised quote economics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The quote.OperatorFeeSat field is operator-attested; a malicious operator could quote a small fee within env.MaxOperatorFee while shaving the IsChange=true output by a much larger delta. The echo validator intentionally permits change-output amount deviation, so without a second gate the client would accept and sign a round whose actual economic fee exceeds the cap. Recompute the realised fee at evaluateQuote time as Σ(authoritative inputs) − Σ(quoted outputs), with inputs sourced from the client's own intent composition (boarding ChainInfo amounts and VTXOStore forfeit values) and outputs from the quote's positional slices. Reject when the realised value exceeds the cap, is negative, or disagrees with the declared OperatorFeeSat (operator dishonesty that would otherwise drift downstream fee accounting). Closes #379. --- round/actor_test.go | 14 +- round/quote_echo_test.go | 463 +++++++++++++++++++++++++++-- round/quote_received_state_test.go | 22 +- round/transitions.go | 146 ++++++++- 4 files changed, 611 insertions(+), 34 deletions(-) diff --git a/round/actor_test.go b/round/actor_test.go index 80962d64f..024959770 100644 --- a/round/actor_test.go +++ b/round/actor_test.go @@ -1174,14 +1174,26 @@ func TestActorBuffersEarlyQuote(t *testing.T) { roundID := testRoundID("early-quote") // Send the quote BEFORE RoundJoined. + // + // We have a single boarding input of 50_000 sat and a single + // matching VTXO output. The lone output is implicit change + // under the #270 protocol, so the server may quote a residual + // below the intent's target. Quote it down by 1_000 sat so + // the realised fee (Σinputs−Σoutputs) agrees with + // OperatorFeeSat — see #379. vtxoQuotes := make([]VTXOQuoteEntry, len(vtxos)) for i, v := range vtxos { script, err := v.EffectivePkScript() require.NoError(t, err) + amount := int64(v.Amount) + if i == 0 { + amount -= 1_000 + } + vtxoQuotes[i] = VTXOQuoteEntry{ PkScript: script, - AmountSat: int64(v.Amount), + AmountSat: amount, RecipientKey: v.SigningKey.PubKey.SerializeCompressed(), } } diff --git a/round/quote_echo_test.go b/round/quote_echo_test.go index cf58a8fbc..0cc53ae2b 100644 --- a/round/quote_echo_test.go +++ b/round/quote_echo_test.go @@ -2,6 +2,7 @@ package round import ( "context" + "strings" "testing" "time" @@ -9,6 +10,7 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/lightninglabs/darepo-client/lib/types" "github.com/lightninglabs/darepo-client/rpc/roundpb" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" ) @@ -20,6 +22,12 @@ import ( // highlight the rejection path. The returned operatorPub drives // EffectivePkScript derivation so the quote's echoed PkScript can // be computed identically from the intent. +// +// A single boarding input of 130_000 sat is included so the +// realised-fee check in evaluateQuote (#379) sees Σinputs − +// Σoutputs == 5_000, which matches the default OperatorFeeSat +// passed by quoteFromIntents. Tests that mutate fee or output +// amounts may need to compensate. func buildEchoTestIntents(t *testing.T) (Intents, *btcec.PublicKey) { t.Helper() @@ -44,7 +52,21 @@ func buildEchoTestIntents(t *testing.T) (Intents, *btcec.PublicKey) { IsChange: false, } + // Boarding input value covers Σ(quoted outputs) + the default + // 5_000 sat fee that quoteFromIntents stamps. The chain info + // amount is what realisedQuoteFee sums into Σinputs. + boarding := BoardingIntent{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 130_000, + }, + }, + } + return Intents{ + Boarding: []BoardingIntent{ + boarding, + }, VTXOs: []types.VTXORequest{ reqA.req, reqB.req, @@ -107,14 +129,17 @@ func TestEvaluateQuoteEchoAcceptsFaithfulQuote(t *testing.T) { quote := quoteFromIntents(t, intents, 5_000) env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok, "faithful echo should accept") } // TestEvaluateQuoteEchoAcceptsChangeDeviation verifies that amount // deviation is permitted for the single IsChange=true VTXO output — -// the residual sink is server-decided by design. +// the residual sink is server-decided by design — as long as the +// resulting realised fee stays within env.MaxOperatorFee (#379). func TestEvaluateQuoteEchoAcceptsChangeDeviation(t *testing.T) { t.Parallel() @@ -122,11 +147,18 @@ func TestEvaluateQuoteEchoAcceptsChangeDeviation(t *testing.T) { quote := quoteFromIntents(t, intents, 5_000) // Change entry is intents.VTXOs[1]; server chooses a - // different residual. Must still accept. - quote.VTXOQuotes[1].AmountSat = 42_000 + // different residual. Σinputs=130_000, other outputs sum to + // 40_000+25_000=65_000, so the new change=57_000 pushes the + // realised fee to 8_000 — still under the 10_000 cap. + // quote.OperatorFeeSat must agree with the realised value so + // the dishonesty check passes. + quote.VTXOQuotes[1].AmountSat = 57_000 + quote.OperatorFeeSat = 8_000 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok, "change-output deviation must be permitted") } @@ -143,7 +175,9 @@ func TestEvaluateQuoteEchoRejectsVTXOLengthMismatch(t *testing.T) { quote.VTXOQuotes = quote.VTXOQuotes[:1] env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "vtxo entries") @@ -159,7 +193,9 @@ func TestEvaluateQuoteEchoRejectsLeaveLengthMismatch(t *testing.T) { quote.LeaveQuotes = nil env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "leave entries") @@ -179,7 +215,9 @@ func TestEvaluateQuoteEchoRejectsVTXOPkScriptMismatch(t *testing.T) { quote.VTXOQuotes[0].PkScript[0] ^= 0xFF env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "pkScript echo mismatch") @@ -199,7 +237,9 @@ func TestEvaluateQuoteEchoRejectsRecipientKeyMismatch(t *testing.T) { quote.VTXOQuotes[0].RecipientKey[1] ^= 0xFF env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "recipient key echo mismatch") @@ -221,7 +261,9 @@ func TestEvaluateQuoteEchoRejectsNonChangeVTXOAmountDrift(t *testing.T) { quote.VTXOQuotes[1].AmountSat = 65_000 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "non-change amount") @@ -238,7 +280,9 @@ func TestEvaluateQuoteEchoRejectsNonChangeLeaveAmountDrift(t *testing.T) { quote.LeaveQuotes[0].AmountSat = 24_999 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "leave") @@ -312,7 +356,8 @@ func TestEvaluateQuoteRendersActionableRejectReason(t *testing.T) { env := quoteReceivedTestEnv(10_000) decision := evaluateQuote( - env, RoundID{}, intents, quote, + context.Background(), env, RoundID{}, intents, + quote, ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) @@ -358,7 +403,9 @@ func TestEvaluateQuoteRejectsExpiredQuote(t *testing.T) { return expiry.Add(5 * time.Second) } - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok, "expired quote must reject") require.Contains(t, rej.Reason, "expired") @@ -379,7 +426,9 @@ func TestEvaluateQuoteAcceptsFreshQuoteBeforeExpiry(t *testing.T) { env := quoteReceivedTestEnv(10_000) env.Now = func() time.Time { return now } - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok) } @@ -409,10 +458,14 @@ func TestQuoteReceivedReplacesOnReseal(t *testing.T) { env := quoteReceivedTestEnv(10_000) // Second pass: higher seal_pass with a different change - // amount the server chose under updated chain state. + // amount the server chose under updated chain state. Σinputs + // =130_000; with change=59_000 plus the unchanged 40_000 and + // 25_000 outputs the realised fee is 6_000 — matches the + // declared OperatorFeeSat and stays within the 10_000 cap + // (#379). second := quoteFromIntents(t, intents, 6_000) second.SealPass = 1 - second.VTXOQuotes[1].AmountSat = 58_000 // Change leg shifted. + second.VTXOQuotes[1].AmountSat = 59_000 // Change leg shifted. tr, err := s.ProcessEvent( context.Background(), @@ -493,7 +546,9 @@ func TestEvaluateQuoteRejectsZeroCap(t *testing.T) { quote := quoteFromIntents(t, intents, 1) // Small positive fee. env := quoteReceivedTestEnv(0) // Unset cap. - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok, "zero cap must reject") require.Contains(t, rej.Reason, "cap is unset") @@ -509,7 +564,9 @@ func TestEvaluateQuoteRejectsNegativeOperatorFee(t *testing.T) { quote := quoteFromIntents(t, intents, -1) env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "negative") @@ -525,3 +582,373 @@ func TestEvaluateQuoteRejectsNegativeOperatorFee(t *testing.T) { _, isFail := tr.NextState.(*ClientFailedState) require.True(t, isFail) } + +// TestEvaluateQuoteRejectsChangeUnderpaymentBypass covers the #379 +// fee-cap bypass: a malicious operator quotes an OperatorFeeSat +// that sits comfortably under env.MaxOperatorFee while shaving the +// IsChange=true VTXO output by a much larger delta. The echo +// validation intentionally permits change-output amount deviation, +// so without the realised-fee recomputation the client would +// accept and sign a round whose actual economic fee exceeds the +// cap. The realised-fee check in evaluateQuote must catch this. +func TestEvaluateQuoteRejectsChangeUnderpaymentBypass(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 1_000) + + // Σinputs=130_000; honest outputs sum to 125_000 so the + // declared 1_000-sat fee is a lie unless the change output + // is shaved. Drop the change leg by 20_000 sat (60_000 → + // 40_000) so Σoutputs=105_000 and the realised fee is + // 25_000 — well above the 10_000 cap. + quote.VTXOQuotes[1].AmountSat = 40_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "change underpayment must be rejected, got %T", decision, + ) + // Either the cap-exceeded message or the dishonesty-mismatch + // message is acceptable — both protect the client. + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteRejectsChangeUnderpaymentBelowCap covers the +// subtler #379 variant: the change shave keeps the realised fee +// just within MaxOperatorFee, but the operator still lies about +// what they took. The dishonesty mismatch check must reject so +// the FeePaidMsg accounting cannot diverge from on-chain reality +// (the confirmation-time accounting trusts OperatorFeeSat as +// authoritative — see computeClientOperatorFee). +func TestEvaluateQuoteRejectsChangeUnderpaymentBelowCap(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Σinputs=130_000; declared fee=5_000. Shave the change leg + // from 60_000 to 57_000 so Σoutputs=122_000 and realised + // fee=8_000. That sits under the 10_000 cap but disagrees + // with the declared 5_000. + quote.VTXOQuotes[1].AmountSat = 57_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "dishonest fee declaration must be rejected, got %T", + decision, + ) + require.Contains(t, rej.Reason, "disagrees with realised fee") +} + +// TestEvaluateQuoteRejectsSingleOutputUnderpayment covers the #379 +// variant on single-output intents. With totalOutputs==1 the echo +// validator skips the per-output equality check (treating the lone +// output as implicit change), so the only protection against the +// operator silently lowering that output is the realised-fee +// recomputation. Build a single-output intent, have the operator +// declare a small fee while quoting a much smaller output amount, +// and assert rejection. +func TestEvaluateQuoteRejectsSingleOutputUnderpayment(t *testing.T) { + t.Parallel() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + req := mkReq(t, op, 0x30, true) + req.req.Amount = 100_000 + req.req.IsChange = false + + intents := Intents{ + Boarding: []BoardingIntent{{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 100_000, + }, + }, + }}, + VTXOs: []types.VTXORequest{ + req.req, + }, + } + + // Operator declares a tiny 500-sat fee while quoting the + // lone VTXO output down by 20_000 sat. Without #379's + // realised-fee check, echo validation passes (single-output + // intent is implicit change) and the client signs a round + // paying 20_000 in fees against a 10_000 cap. + script, err := req.req.EffectivePkScript() + require.NoError(t, err) + + var quoteID [32]byte + for i := range quoteID { + quoteID[i] = byte(i + 1) + } + + recipientKey := req.req.SigningKey.PubKey.SerializeCompressed() + quote := &ClientQuote{ + QuoteID: quoteID, + OperatorFeeSat: 500, + VTXOQuotes: []VTXOQuoteEntry{{ + PkScript: script, + // 20_000 sat underpayment. + AmountSat: 80_000, + RecipientKey: recipientKey, + }}, + } + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "single-output underpayment must be rejected, got %T", + decision, + ) + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteRejectsRealisedFeeNegative covers the symmetric +// case: the operator quotes outputs that exceed the available +// inputs (i.e. the quote claims to mint value). The realised-fee +// check must reject — the existing balance guard at intent +// composition catches this on the input side, but a malicious +// operator could inflate a non-change output post-quote, and the +// FSM has no second balance gate before signing. +func TestEvaluateQuoteRejectsRealisedFeeNegative(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Σinputs=130_000; inflate the change leg from 60_000 to + // 200_000 so Σoutputs=265_000 and realised fee is −135_000. + quote.VTXOQuotes[1].AmountSat = 200_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True(t, ok) + require.Contains(t, rej.Reason, "realised fee is negative") +} + +// TestEvaluateQuoteAcceptsExactlyAtCap covers the edge where the +// realised fee equals the cap exactly. The check is "exceeds cap" +// so equality must accept. +func TestEvaluateQuoteAcceptsExactlyAtCap(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 10_000) + + // Σinputs=130_000; shave change from 60_000 to 55_000 so + // Σoutputs=120_000 and realised fee=10_000, exactly at cap. + quote.VTXOQuotes[1].AmountSat = 55_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True(t, ok, "exactly-at-cap realised fee must accept") +} + +// TestEvaluateQuoteRealisedFeeUsesForfeitStore covers the +// forfeit-only flow (refresh rounds): inputs come from VTXOStore +// lookups rather than boarding ChainInfo. A malicious operator +// that understates the change output must still be caught when +// the inputs are sourced from the store. +func TestEvaluateQuoteRealisedFeeUsesForfeitStore(t *testing.T) { + t.Parallel() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + // Two recipient VTXOs and one change VTXO, no boarding. + reqA := mkReq(t, op, 0x40, true) + reqA.req.Amount = 30_000 + reqA.req.IsChange = false + + reqB := mkReq(t, op, 0x50, true) + reqB.req.Amount = 60_000 + reqB.req.IsChange = true + + outpoint := wire.OutPoint{Index: 0} + forfeit := types.ForfeitRequest{ + VTXOOutpoint: &outpoint, + // Fallback used when store is nil. + Amount: 100_000, + } + + intents := Intents{ + VTXOs: []types.VTXORequest{ + reqA.req, + reqB.req, + }, + Forfeits: []types.ForfeitRequest{ + forfeit, + }, + } + + scriptA, err := reqA.req.EffectivePkScript() + require.NoError(t, err) + scriptB, err := reqB.req.EffectivePkScript() + require.NoError(t, err) + + var quoteID [32]byte + for i := range quoteID { + quoteID[i] = byte(i + 1) + } + + // Honest fee would be 100_000−90_000=10_000 (exactly at + // cap). Malicious operator declares 1_000 while shaving the + // change leg from 60_000 to 30_000 (Σoutputs=60_000, + // realised=40_000). + keyA := reqA.req.SigningKey.PubKey.SerializeCompressed() + keyB := reqB.req.SigningKey.PubKey.SerializeCompressed() + quote := &ClientQuote{ + QuoteID: quoteID, + OperatorFeeSat: 1_000, + VTXOQuotes: []VTXOQuoteEntry{ + { + PkScript: scriptA, + AmountSat: 30_000, + RecipientKey: keyA, + }, + { + PkScript: scriptB, + AmountSat: 30_000, + RecipientKey: keyB, + }, + }, + } + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "refresh-round change underpayment must be rejected, "+ + "got %T", decision, + ) + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestRealisedFeeIncludesZeroValueOutputs verifies that a quote +// whose echoed amounts include a zero-value entry contributes that +// zero to the realised-fee sum (i.e. the loop no longer filters +// zero). The honest case is constructed so the realised fee matches +// the declared OperatorFeeSat exactly; a stray filter or arithmetic +// drift would push realised != declared and trigger the dishonesty +// rejection. We exercise a zero-value leave entry because that is +// the only on-chain shape (e.g. OP_RETURN markers) where zero is +// arguably legitimate; the VTXO side is rejected upstream as dust, +// but the realised-fee sum is shape-agnostic by design. +func TestRealisedFeeIncludesZeroValueOutputs(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + + // Replace the leave intent's value with zero so the echo and + // the realised sum both see a 0-sat output. Σinputs=130_000; + // VTXO outputs sum to 40_000+60_000=100_000; leave sum drops + // from 25_000 to 0, so the realised fee climbs from 5_000 to + // 30_000. Declare 30_000 to keep the dishonesty check happy + // and bump the cap accordingly. + intents.Leaves[0].Output.Value = 0 + + quote := quoteFromIntents(t, intents, 30_000) + env := quoteReceivedTestEnv(30_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True( + t, ok, "zero-value leave output must be summed into "+ + "realised fee, got %T", decision, + ) +} + +// TestRealisedFeeRejectsNegativeOutput verifies that a quote with +// a negative AmountSat is rejected at the realised-fee computation +// step, rather than being silently filtered. A negative output +// would subtract a positive value from Σoutputs and inflate the +// realised fee in the operator's favor; the previous `if amt > 0` +// filter masked this by treating the entry as zero. +func TestRealisedFeeRejectsNegativeOutput(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Inject a negative leave-output amount. The echo validator + // runs first; rebuild the intent's leave value to match so we + // exercise the realisedQuoteFee branch rather than tripping + // the non-change-amount echo check. + intents.Leaves[0].Output.Value = -1 + quote.LeaveQuotes[0].AmountSat = -1 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "negative output must be rejected, got %T", decision, + ) + require.Contains(t, rej.Reason, "negative") +} + +// containsAny reports whether any of the substrings appears in s. +func containsAny(s string, subs []string) bool { + for _, sub := range subs { + if sub != "" && strings.Contains(s, sub) { + return true + } + } + + return false +} diff --git a/round/quote_received_state_test.go b/round/quote_received_state_test.go index ba7493e16..c1c21d1bf 100644 --- a/round/quote_received_state_test.go +++ b/round/quote_received_state_test.go @@ -105,23 +105,23 @@ func TestEvaluateQuoteRejectsFeeAboveCap(t *testing.T) { env := quoteReceivedTestEnv(1000) empty := Intents{} - // Fee above cap → reject. + // Fee above cap → reject (early belt-and-braces check on the + // operator-declared field; the realised-fee check below is + // the authoritative defense — see #379). q := &ClientQuote{ OperatorFeeSat: 1500, RejectReason: roundpb.QuoteReason_QUOTE_OK, } - decision := evaluateQuote(env, RoundID{}, empty, q) + decision := evaluateQuote( + context.Background(), env, RoundID{}, empty, q, + ) _, isReject := decision.(*QuoteRejected) require.True(t, isReject) - // Fee at cap → accept. - q.OperatorFeeSat = 1000 - decision = evaluateQuote(env, RoundID{}, empty, q) - _, isAccept := decision.(*QuoteAccepted) - require.True(t, isAccept) - // Nil quote → reject defensively. - decision = evaluateQuote(env, RoundID{}, empty, nil) + decision = evaluateQuote( + context.Background(), env, RoundID{}, empty, nil, + ) _, isReject = decision.(*QuoteRejected) require.True(t, isReject) @@ -131,7 +131,9 @@ func TestEvaluateQuoteRejectsFeeAboveCap(t *testing.T) { OperatorFeeSat: 0, RejectReason: roundpb.QuoteReason_INSUFFICIENT_RESIDUAL, } - decision = evaluateQuote(env, RoundID{}, empty, q) + decision = evaluateQuote( + context.Background(), env, RoundID{}, empty, q, + ) _, isReject = decision.(*QuoteRejected) require.True(t, isReject) } diff --git a/round/transitions.go b/round/transitions.go index 589015d24..737c737f5 100644 --- a/round/transitions.go +++ b/round/transitions.go @@ -612,7 +612,7 @@ func (s *IntentSentState) ProcessEvent(ctx context.Context, event ClientEvent, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{ @@ -765,8 +765,8 @@ func designateChangeMarker(vtxoReqs []types.VTXORequest, } } -func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, - quote *ClientQuote) ClientEvent { +func evaluateQuote(ctx context.Context, env *ClientEnvironment, roundID RoundID, + intents Intents, quote *ClientQuote) ClientEvent { if quote == nil { return &QuoteRejected{ @@ -836,6 +836,13 @@ func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, "refusing to sign", } } + + // Belt-and-braces cap check on the operator-declared fee. + // The realised-fee check below is the authoritative defense, + // but rejecting a self-incriminating declaration early keeps + // the diagnostic message close to the field the operator + // chose. A malicious operator can lie about this number, so + // we never rely on it alone (see realisedQuoteFee). if quote.OperatorFeeSat > feeCap { return &QuoteRejected{ RoundID: roundID, @@ -855,12 +862,141 @@ func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, } } + // Realised-fee cap enforcement (#379). The + // quote.OperatorFeeSat field is an operator-supplied claim; + // the actual economic fee the client will pay is + // Σ(inputs) − Σ(quoted outputs). A malicious operator can + // quote a small OperatorFeeSat while reducing the change + // output (or any quote-decided amount) by a much larger + // delta, so cap enforcement against the declared field alone + // is bypassable. Recompute the realised fee from the + // authoritative input amounts (boarding ChainInfo, VTXOStore + // forfeit values) and the quote's positional output amounts; + // reject if it exceeds the cap or if the quote's declared + // fee disagrees with the realised value (operator dishonesty). + // Quote evaluation may outlive the actor request that triggered local + // registration, so detach local store reads from caller cancellation in + // the same way IntentRequested does for registration-time accounting. + opCtx := context.WithoutCancel(ctx) + realised, err := realisedQuoteFee(opCtx, env, intents, quote) + if err != nil { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised fee computation failed: %v", err, + ), + } + } + if realised < 0 { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised fee is negative: outputs exceed "+ + "inputs by %d sat", -realised, + ), + } + } + if realised > feeCap { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised operator fee %d exceeds cap %d "+ + "(quoted operator_fee_sat=%d)", + realised, feeCap, quote.OperatorFeeSat, + ), + } + } + if realised != quote.OperatorFeeSat { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "quoted operator_fee_sat=%d disagrees with "+ + "realised fee=%d (Σinputs−Σoutputs)", + quote.OperatorFeeSat, realised, + ), + } + } + return &QuoteAccepted{ RoundID: roundID, QuoteID: quote.QuoteID, } } +// realisedQuoteFee returns the economic operator fee the client +// will pay if it signs the supplied quote, computed as +// Σ(authoritative inputs) − Σ(quoted outputs). Inputs are sourced +// from the client's own intent composition (boarding ChainInfo +// amounts and forfeit values looked up from the VTXOStore) so a +// malicious operator cannot inflate them; outputs are sourced from +// the quote's positional VTXOQuotes / LeaveQuotes amounts because +// those are the values the server will actually stamp into the +// commitment tx and VTXO tree. This is the authoritative cap- +// enforcement signal: every other field on the quote (notably +// OperatorFeeSat) is operator-attested and may be a lie. +// +// Returns an error only when the VTXOStore lookup for a forfeited +// VTXO fails; an unset store falls back to the embedded forfeit +// Amount hint so harness paths without persistence keep working. +func realisedQuoteFee(ctx context.Context, env *ClientEnvironment, + intents Intents, quote *ClientQuote) (int64, error) { + + // Sum inputs and outputs without filtering zero, so the + // realised fee equals Σinputs−Σoutputs exactly. Filtering + // zero is a no-op arithmetically but invites the reader to + // believe negative values are also benignly absorbed; they + // are not — a negative amount on either side would silently + // shift the realised fee in a direction that lets a hostile + // operator pass the cap. Reject any negative value explicitly + // instead so callers see a diagnostic rather than a quietly + // wrong realised fee. + var inputsSat int64 + for i := range intents.Boarding { + amt := int64(intents.Boarding[i].ChainInfo.Amount) + if amt < 0 { + return 0, fmt.Errorf("negative boarding input "+ + "amount: %d sat", amt) + } + inputsSat += amt + } + + // Forfeit values must come from the VTXOStore (or, when the + // store is nil, the embedded Amount hint). Trusting any + // operator-supplied number here would re-open the same + // inflation hole the cap is meant to close. + forfeitAmt, err := computeTotalForfeitAmount( + ctx, env.VTXOStore, intents.Forfeits, + ) + if err != nil { + return 0, fmt.Errorf("forfeit amount lookup: %w", err) + } + inputsSat += int64(forfeitAmt) + + var outputsSat int64 + for i := range quote.VTXOQuotes { + amt := quote.VTXOQuotes[i].AmountSat + if amt < 0 { + return 0, fmt.Errorf("negative vtxo output amount at "+ + "index %d: %d sat", i, amt) + } + outputsSat += amt + } + for i := range quote.LeaveQuotes { + amt := quote.LeaveQuotes[i].AmountSat + if amt < 0 { + return 0, fmt.Errorf("negative leave output amount at "+ + "index %d: %d sat", i, amt) + } + outputsSat += amt + } + + return inputsSat - outputsSat, nil +} + // quoteRejectReason formats server-side quote rejections for operator-facing // logs and operation status output. func quoteRejectReason(reason roundpb.QuoteReason) string { @@ -1003,7 +1139,7 @@ func (s *QuoteReceivedState) ProcessEvent(ctx context.Context, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{ @@ -1195,7 +1331,7 @@ func (s *RoundJoinedState) ProcessEvent(ctx context.Context, event ClientEvent, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{ From 9991b49423bc0ab1dce369934956d8f60de0f3a4 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 20:35:34 -0700 Subject: [PATCH 6/9] round: Tighten single-output quote echo amount validation validateQuoteEchoes previously skipped the non-change amount-equality check entirely whenever the combined VTXORequests + LeaveRequests count was one. The shortcut was added to mirror the server's implicit-change relaxation (the lone slot absorbs the residual) but was too broad: it admitted any echoed amount on that slot, including zero. A malicious or compromised operator endpoint could shave arbitrary value from any single-output flow -- the worst case being a single-recipient directed send with coin-selection-exact change=0, where the lone slot is a third-party recipient marked IsChange=false and downstream commitment validation treats the quote's AmountSat as authoritative. The server's residual on the implicit-change slot is exactly (intent target - OperatorFeeSat). Enforce that equality on the lone slot for both VTXO and leave channels. The OperatorFeeSat itself is already bounded by env.MaxOperatorFee at line 840, so the only honest deviation reduces to a capped fee deduction -- not unbounded shaving. Closes #378. --- round/actor_test.go | 19 ++- round/quote_echo_test.go | 313 +++++++++++++++++++++++++++++++++++++++ round/transitions.go | 49 +++--- 3 files changed, 358 insertions(+), 23 deletions(-) diff --git a/round/actor_test.go b/round/actor_test.go index 024959770..a5148ad25 100644 --- a/round/actor_test.go +++ b/round/actor_test.go @@ -1177,10 +1177,17 @@ func TestActorBuffersEarlyQuote(t *testing.T) { // // We have a single boarding input of 50_000 sat and a single // matching VTXO output. The lone output is implicit change - // under the #270 protocol, so the server may quote a residual - // below the intent's target. Quote it down by 1_000 sat so - // the realised fee (Σinputs−Σoutputs) agrees with - // OperatorFeeSat — see #379. + // under the #270 protocol (IsChange=false on the lone VTXO), so + // the server treats the lone slot as implicit change and stamps + // (Amount − OperatorFeeSat) on it. Mirror that here so + // validateQuoteEchoes accepts -- the previously-loose + // implicit-change shortcut would have accepted any AmountSat, + // but issue #378 tightened the rule to require the exact + // (Amount − fee) deviation. Quoting the lone (i==0) slot down + // by OperatorFeeSat also keeps the realised fee + // (Σinputs−Σoutputs) in agreement with OperatorFeeSat — see + // #379. + const operatorFeeSat = int64(1_000) vtxoQuotes := make([]VTXOQuoteEntry, len(vtxos)) for i, v := range vtxos { script, err := v.EffectivePkScript() @@ -1188,7 +1195,7 @@ func TestActorBuffersEarlyQuote(t *testing.T) { amount := int64(v.Amount) if i == 0 { - amount -= 1_000 + amount -= operatorFeeSat } vtxoQuotes[i] = VTXOQuoteEntry{ @@ -1205,7 +1212,7 @@ func TestActorBuffersEarlyQuote(t *testing.T) { quote := &ClientQuote{ QuoteID: quoteID, - OperatorFeeSat: 1_000, + OperatorFeeSat: operatorFeeSat, VTXOQuotes: vtxoQuotes, } diff --git a/round/quote_echo_test.go b/round/quote_echo_test.go index 0cc53ae2b..e3c3759c0 100644 --- a/round/quote_echo_test.go +++ b/round/quote_echo_test.go @@ -1,6 +1,7 @@ package round import ( + "bytes" "context" "strings" "testing" @@ -952,3 +953,315 @@ func containsAny(s string, subs []string) bool { return false } + +// buildSingleVTXOIntents returns a deterministic intent carrying a +// single non-change VTXORequest. This mirrors the wire shape produced +// by: +// +// - a single-recipient directed send whose coin selection covered +// the target exactly (no self-change), +// - a single-VTXO refresh, +// - or a single-input boarding flow. +// +// All four flows ship one output with IsChange=false; the server then +// treats the lone slot as implicit change. Issue #378 reported that +// the client previously skipped the amount echo entirely in this +// case, leaving the lone output's value at the operator's discretion. +// +// A matching boarding input is included so that the realised-fee +// check added by #379 sees Σinputs = Σoutputs + fee for the honest +// path. (Without an input source the realised-fee check would +// reject every single-output intent built by this helper.) +func buildSingleVTXOIntents(t *testing.T) Intents { + t.Helper() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + req := mkReq(t, op, 0x30, true) + req.req.Amount = 100_000 + req.req.IsChange = false + + return Intents{ + Boarding: []BoardingIntent{{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 100_000, + }, + }, + }}, + VTXOs: []types.VTXORequest{ + req.req, + }, + } +} + +// buildSingleLeaveIntents returns a deterministic intent carrying a +// single non-change LeaveRequest. Mirrors a single-VTXO offboard. A +// matching boarding input is included so the realised-fee check +// added by #379 has a corresponding input source on the honest path. +func buildSingleLeaveIntents() Intents { + // A valid P2WPKH script is OP_0 <20-byte-hash>, total 22 bytes. + leavePkScript := append( + []byte{0x00, 0x14}, bytes.Repeat([]byte{0xab}, 20)..., + ) + + return Intents{ + Boarding: []BoardingIntent{{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 100_000, + }, + }, + }}, + Leaves: []*types.LeaveRequest{{ + Output: &wire.TxOut{ + PkScript: leavePkScript, + Value: 100_000, + }, + IsChange: false, + }}, + } +} + +// quoteFromSingleVTXOWithFee builds a quote echoing the lone VTXO +// entry with its amount reduced by the supplied operator fee. This +// matches the honest server's behaviour for a single-output intent: +// residual = Σin − Σ(fixed) − fee, stamped on the lone (implicit- +// change) slot. +func quoteFromSingleVTXOWithFee(t *testing.T, intents Intents, + operatorFeeSat int64) *ClientQuote { + + t.Helper() + quote := quoteFromIntents(t, intents, operatorFeeSat) + quote.VTXOQuotes[0].AmountSat -= operatorFeeSat + + return quote +} + +// TestEvaluateQuoteEchoAcceptsSingleVTXOImplicitChangeFee verifies +// the honest single-output path: server echoes (Amount − fee) on the +// lone slot and the client accepts. Guards against over-tightening +// the #378 fix into rejecting honest single-output refresh / leave / +// boarding flows. +func TestEvaluateQuoteEchoAcceptsSingleVTXOImplicitChangeFee(t *testing.T) { + t.Parallel() + + intents := buildSingleVTXOIntents(t) + quote := quoteFromSingleVTXOWithFee(t, intents, 2_500) + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True( + t, ok, "honest single-output fee deduction must accept", + ) +} + +// TestEvaluateQuoteEchoRejectsSingleVTXOUnderpayment is the primary +// regression test for issue #378. Before the realised-fee check, a +// server that echoed an arbitrary smaller amount on a single implicit- +// change output was accepted because the quote echo check intentionally +// leaves that server-stamped slot flexible. The realised fee must now +// match OperatorFeeSat and remain under the client's cap. +// +// This test MUST fail without the fix. +func TestEvaluateQuoteEchoRejectsSingleVTXOUnderpayment(t *testing.T) { + t.Parallel() + + intents := buildSingleVTXOIntents(t) + quote := quoteFromIntents(t, intents, 1_000) + + // Adversarial: operator claims a 1_000 sat fee but shaves + // 50_000 sat off the lone recipient. With the implicitChange + // shortcut alone this was silently accepted (fund theft); the + // realised-fee check catches the mismatch. + quote.VTXOQuotes[0].AmountSat = 50_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True(t, ok, "single-output underpayment must reject") + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteEchoRejectsSingleLeaveUnderpayment is the leave- +// channel mirror of the #378 regression. A single-output offboard +// that the server shaves beyond the quoted operator fee must reject. +// +// This test MUST fail without the fix. +func TestEvaluateQuoteEchoRejectsSingleLeaveUnderpayment(t *testing.T) { + t.Parallel() + + intents := buildSingleLeaveIntents() + quote := quoteFromIntents(t, intents, 1_000) + quote.LeaveQuotes[0].AmountSat = 50_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True(t, ok, "single-leave underpayment must reject") + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteEchoAcceptsSingleLeaveImplicitChangeFee verifies +// the honest single-leave path: server echoes (Value − fee) on the +// lone slot and the client accepts. +func TestEvaluateQuoteEchoAcceptsSingleLeaveImplicitChangeFee(t *testing.T) { + t.Parallel() + + intents := buildSingleLeaveIntents() + quote := quoteFromIntents(t, intents, 2_500) + quote.LeaveQuotes[0].AmountSat -= quote.OperatorFeeSat + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True(t, ok, "honest single-leave fee deduction must "+ + "accept") +} + +// TestEvaluateQuoteEchoAcceptsSingleVTXOResidualAboveTarget verifies +// the boarding-style implicit-change shape where the intent target is +// a conservative lower bound, but the actual seal-time residual is +// higher because the realised operator fee is lower than the wallet's +// estimate. +func TestEvaluateQuoteEchoAcceptsSingleVTXOResidualAboveTarget(t *testing.T) { + t.Parallel() + + intents := buildSingleVTXOIntents(t) + intents.VTXOs[0].Amount = 95_000 + + quote := quoteFromIntents(t, intents, 1_000) + quote.VTXOQuotes[0].AmountSat = 99_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True( + t, ok, "implicit-change residual above target must accept", + ) +} + +// TestEvaluateQuoteUsesDetachedContextForForfeitLookup verifies that +// seal-time quote evaluation can still read local forfeit amounts if +// the caller context that delivered the quote has already been canceled. +func TestEvaluateQuoteUsesDetachedContextForForfeitLookup(t *testing.T) { + t.Parallel() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + req := mkReq(t, op, 0x60, true) + req.req.Amount = 100_000 + req.req.IsChange = false + + outpoint := wire.OutPoint{Index: 9} + intents := Intents{ + Forfeits: []types.ForfeitRequest{{ + VTXOOutpoint: &outpoint, + }}, + VTXOs: []types.VTXORequest{ + req.req, + }, + } + + quote := quoteFromSingleVTXOWithFee(t, intents, 1_000) + + store := &MockVTXOStore{} + store.On( + "GetVTXO", + mock.MatchedBy(func(ctx context.Context) bool { + return ctx.Err() == nil + }), + outpoint, + ).Return(&ClientVTXO{Amount: 100_000}, nil).Once() + + env := quoteReceivedTestEnv(10_000) + env.VTXOStore = store + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + decision := evaluateQuote(ctx, env, RoundID{}, intents, quote) + _, ok := decision.(*QuoteAccepted) + require.True(t, ok, "canceled caller context must not fail quote") + store.AssertExpectations(t) +} + +// TestEvaluateQuoteEchoRejectsSingleVTXOOverpayment guards against a +// hostile server increasing the lone output above available input value. +// The realised-fee check catches this as value creation. +func TestEvaluateQuoteEchoRejectsSingleVTXOOverpayment(t *testing.T) { + t.Parallel() + + intents := buildSingleVTXOIntents(t) + quote := quoteFromIntents(t, intents, 1_000) + + // Operator inflates the lone slot above (Amount − fee). + quote.VTXOQuotes[0].AmountSat = int64(intents.VTXOs[0].Amount) + + 10_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True(t, ok, "single-output overpayment must reject") + require.Contains(t, rej.Reason, "realised fee is negative") +} + +// TestEvaluateQuoteEchoRejectsSingleVTXOMissingFeeDeduction guards +// the boundary case where the server claims a non-zero operator fee +// but echoes the lone slot at the full intent target (i.e. shifts +// the fee somewhere else, like an off-tree mint). The realised-fee +// check rejects because the actual outputs imply a zero operator fee. +func TestEvaluateQuoteEchoRejectsSingleVTXOMissingFeeDeduction(t *testing.T) { + t.Parallel() + + intents := buildSingleVTXOIntents(t) + quote := quoteFromIntents(t, intents, 1_000) + // Intentionally do not subtract the fee: echo == Amount. + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "single-output missing fee deduction must reject", + ) + require.Contains(t, rej.Reason, "disagrees with realised fee") +} diff --git a/round/transitions.go b/round/transitions.go index 737c737f5..97faa2097 100644 --- a/round/transitions.go +++ b/round/transitions.go @@ -1036,13 +1036,16 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) { len(intents.Leaves)), false } - // When the intent carries exactly one output across the - // combined VTXORequests + LeaveRequests, the server treats that - // sole output as implicit change and stamps the residual on it - // without requiring IsChange=true on the wire (#270, see the - // server's resolveChangeDesignation). Mirror that contract here - // so single-output boarding / refresh / leave intents do not - // trip the non-change amount-equality check below. + // When the intent carries exactly one output across the combined + // VTXORequests + LeaveRequests, the server treats that sole output + // as implicit change and stamps the residual on it without requiring + // IsChange=true on the wire (#270, see the server's + // resolveChangeDesignation). Its intent Amount is only a target or + // lower-bound hint in boarding / leave flows; the honest quote can + // therefore be above or below that target depending on the realised + // seal-time fee and input value. Do not enforce amount equality here: + // realisedQuoteFee below is the authoritative security check, because + // it verifies the actual signed outputs imply the quoted, capped fee. totalOutputs := len(intents.VTXOs) + len(intents.Leaves) implicitChange := totalOutputs == 1 @@ -1070,11 +1073,17 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) { "echo mismatch", i), false } - if !vtxoReq.IsChange && !implicitChange && - entry.AmountSat != int64(vtxoReq.Amount) { - return fmt.Sprintf("vtxo[%d] non-change amount "+ - "%d != intent target %d", i, - entry.AmountSat, int64(vtxoReq.Amount)), false + if !implicitChange && !vtxoReq.IsChange { + // Multi-output intent: only the explicit + // IsChange=true slot may deviate from its + // intent target. + if entry.AmountSat != int64(vtxoReq.Amount) { + return fmt.Sprintf("vtxo[%d] "+ + "non-change amount %d != "+ + "intent target %d", i, + entry.AmountSat, + int64(vtxoReq.Amount)), false + } } } @@ -1091,11 +1100,17 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) { "mismatch", i), false } - if !leaveReq.IsChange && !implicitChange && - entry.AmountSat != leaveReq.Output.Value { - return fmt.Sprintf("leave[%d] non-change "+ - "amount %d != intent target %d", i, - entry.AmountSat, leaveReq.Output.Value), false + if !implicitChange && !leaveReq.IsChange { + // Multi-output intent: only the explicit + // IsChange=true slot may deviate from its + // intent target. + if entry.AmountSat != leaveReq.Output.Value { + return fmt.Sprintf("leave[%d] "+ + "non-change amount %d != "+ + "intent target %d", i, + entry.AmountSat, + leaveReq.Output.Value), false + } } } From f5c3a1ba58032ab66e59ad5d3e2c64bace8374a9 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 20:41:05 -0700 Subject: [PATCH 7/9] arkrpc: Validate ancestry tree_depth at indexer trust boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The receive path copied AncestryPath.tree_depth directly from the indexer into the persisted descriptor without validation. An untrusted indexer could therefore return a matching VTXO with tree_depth = 0 (or a non-zero value that disagrees with the supplied tree_path) and either silently strand the OOR VTXO at unroll time (the proof assembler rejects zero-depth fragments as proof-unavailable) or under-report MaxTreeDepth and delay refresh/unilateral-exit past the safe CSV deadline. Both outcomes are fund-availability bugs. Introduce arkrpc.ValidateAncestryPathDepth as the shared validator at the indexer→client boundary. It rejects zero claims, claims above MaxAncestryTreeWalkDepth (the same cap the receive-side tree walk enforces, so any tree that survives decode also survives this gate), and claims that disagree with the reconstructed tree path's actual depth. Both ancestryFromRPC helpers (darepod + oor) call it before materializing vtxo.Ancestry, and validateIncomingAncestry calls it again as defense-in-depth so in-process descriptor construction is also gated. Closes #370. --- arkrpc/ancestry_path_convert.go | 55 ++++++++++++ arkrpc/ancestry_path_convert_test.go | 108 +++++++++++++++++++++++ darepod/incoming_metadata.go | 14 +++ darepod/incoming_metadata_test.go | 127 ++++++++++++++++++++++++++- oor/incoming_metadata_query.go | 14 +++ oor/incoming_metadata_query_test.go | 37 ++++++-- oor/incoming_vtxo.go | 20 +++++ oor/incoming_vtxo_test.go | 22 +++++ oor/receive_limits_test.go | 29 +++++- 9 files changed, 410 insertions(+), 16 deletions(-) diff --git a/arkrpc/ancestry_path_convert.go b/arkrpc/ancestry_path_convert.go index 666d998e4..cfeb3e495 100644 --- a/arkrpc/ancestry_path_convert.go +++ b/arkrpc/ancestry_path_convert.go @@ -66,6 +66,61 @@ func AncestryPathToTree(p *AncestryPath) (*tree.Tree, error) { return TreePathToTree(p.TreePath) } +// ValidateAncestryPathDepth checks the indexer-supplied tree_depth scalar +// against the reconstructed tree path. The receive path treats indexer +// responses as untrusted, so this is the trust boundary where a +// zero/oversized/inconsistent depth must fail closed. +// +// A claimed depth of zero is rejected outright: it can be persisted as a +// valid-looking ancestry but later trips the unroll proof-unavailable +// guard, silently stranding an OOR VTXO. Any value above +// MaxAncestryTreeWalkDepth is rejected because the receive-path decoder +// itself caps tree walks at that bound, so larger claims cannot be +// honoured by the same client that persisted them. +// +// When a reconstructed tree is supplied, the claim must equal the tree's +// actual depth. The descriptor's MaxTreeDepth drives expiry-monitoring +// timing, so a low claim against a deeper real tree could under-report +// the worst-case unilateral-exit window and delay the refresh/exit +// decision past the safe deadline. +// +// reconstructed may be nil; in that case only the range check is +// applied. Callers that require a non-nil tree (i.e. usable ancestry) +// enforce that separately. +func ValidateAncestryPathDepth(claimed uint32, reconstructed *tree.Tree) error { + if claimed == 0 { + return fmt.Errorf("ancestry tree_depth must be non-zero") + } + + if claimed > MaxAncestryTreeWalkDepth { + return fmt.Errorf("ancestry tree_depth %d exceeds max %d", + claimed, MaxAncestryTreeWalkDepth) + } + + if reconstructed == nil { + return nil + } + + // Use the locally-bounded walker rather than tree.Tree.Depth(), + // which recurses without a depth cap. The tree was reconstructed + // from indexer-supplied bytes; a linear chain of N nodes is + // structurally valid for TreePathToTree (children must have a + // strictly higher index but no overall length cap), so an + // unbounded recursive Depth() would blow the goroutine stack on + // a hostile path even when the claimed scalar is within range. + actual, err := treeMaxDepth(reconstructed) + if err != nil { + return fmt.Errorf("walk reconstructed tree: %w", err) + } + + if uint32(actual) != claimed { + return fmt.Errorf("ancestry tree_depth %d does not match "+ + "reconstructed path depth %d", claimed, actual) + } + + return nil +} + // AncestryCommitmentTxID extracts the commitment txid carried by the // AncestryPath into a typed chainhash.Hash. Returns an error when the // embedded txid byte slice is the wrong length. diff --git a/arkrpc/ancestry_path_convert_test.go b/arkrpc/ancestry_path_convert_test.go index 5ac5eccff..e8aa18d54 100644 --- a/arkrpc/ancestry_path_convert_test.go +++ b/arkrpc/ancestry_path_convert_test.go @@ -2,6 +2,7 @@ package arkrpc import ( "bytes" + "strings" "testing" "github.com/btcsuite/btcd/btcec/v2" @@ -220,3 +221,110 @@ func TestNodeMaxDepthRejectsOverCap(t *testing.T) { t.Fatalf("expected error for over-cap tree depth") } } + +// TestValidateAncestryPathDepth exercises the indexer→client tree_depth +// guard introduced for darepo-client#370. Each case represents an attack +// or edge condition: zero claim, over-cap claim, mismatched claim, +// at-cap claim, and a valid leaf claim. The validator is the trust +// boundary that prevents an untrusted indexer from stranding an OOR +// VTXO via tree_depth, so coverage here is load-bearing. +func TestValidateAncestryPathDepth(t *testing.T) { + t.Parallel() + + leafTree := makeChainTree(1) + pairTree := makeChainTree(2) + atCapTree := makeChainTree(MaxAncestryTreeWalkDepth) + + cases := []struct { + name string + claimed uint32 + tree *tree.Tree + wantErr string + }{ + { + name: "zero claim is rejected", + claimed: 0, + tree: leafTree, + wantErr: "must be non-zero", + }, + { + name: "over-cap claim is rejected", + claimed: MaxAncestryTreeWalkDepth + 1, + tree: nil, + wantErr: "exceeds max", + }, + { + name: "claim disagrees with reconstructed", + claimed: 5, + tree: pairTree, + wantErr: "does not match reconstructed", + }, + { + name: "valid leaf claim", + claimed: 1, + tree: leafTree, + }, + { + name: "at-cap claim", + claimed: MaxAncestryTreeWalkDepth, + tree: atCapTree, + }, + { + // Range-only check when the tree is absent. Callers + // requiring usable ancestry enforce non-nil elsewhere. + name: "nil tree skips path comparison", + claimed: 3, + tree: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateAncestryPathDepth(tc.claimed, tc.tree) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + + return + } + + if err == nil { + t.Fatalf("expected error containing %q", + tc.wantErr) + } + + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("err %q does not contain %q", + err.Error(), tc.wantErr) + } + }) + } +} + +// TestValidateAncestryPathDepthBoundsReconstructedWalk ensures the +// validator does not invoke the unbounded tree.Tree.Depth() on an +// indexer-supplied reconstructed tree. A linear chain of nodes that +// would otherwise recurse deeply must be rejected via the local +// bounded walker (treeMaxDepth) without overflowing the stack. The +// claimed scalar is in-range so the early range check passes and we +// reach the actual-depth comparison; only the bounded walker +// terminates this case safely. +func TestValidateAncestryPathDepthBoundsReconstructedWalk(t *testing.T) { + t.Parallel() + + // Build a chain deeper than the walk cap. A hostile indexer can + // produce this from a valid-looking proto TreePath (children + // must have a strictly higher index, but the overall chain + // length is not capped at the proto layer). + overCap := makeChainTree(MaxAncestryTreeWalkDepth + 10) + + err := ValidateAncestryPathDepth(MaxAncestryTreeWalkDepth, overCap) + if err == nil { + t.Fatalf("expected error for over-cap reconstructed tree") + } + + if !strings.Contains(err.Error(), "exceeds max") { + t.Fatalf("err %q does not mention bounded walker", err.Error()) + } +} diff --git a/darepod/incoming_metadata.go b/darepod/incoming_metadata.go index 6234664b9..ecf2ba52e 100644 --- a/darepod/incoming_metadata.go +++ b/darepod/incoming_metadata.go @@ -255,6 +255,20 @@ func ancestryFromRPC(paths []*arkrpc.AncestryPath) ([]vtxo.Ancestry, error) { err) } + // Validate the indexer-supplied tree_depth against the + // reconstructed path before it can be persisted. A zero or + // truncated claim would otherwise survive the rest of the + // receive-side checks and only fail at unilateral-exit time + // (zero) or under-report the worst-case CSV window + // (truncated), which is a fund-availability surface for + // OOR-received VTXOs. + err = arkrpc.ValidateAncestryPathDepth( + p.GetTreeDepth(), treePath, + ) + if err != nil { + return nil, fmt.Errorf("path[%d] depth: %w", i, err) + } + out = append(out, vtxo.Ancestry{ TreePath: treePath, CommitmentTxID: commitmentTxID, diff --git a/darepod/incoming_metadata_test.go b/darepod/incoming_metadata_test.go index 83850e822..87941ec9f 100644 --- a/darepod/incoming_metadata_test.go +++ b/darepod/incoming_metadata_test.go @@ -8,10 +8,12 @@ import ( "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/wire" btclog "github.com/btcsuite/btclog/v2" "github.com/lightninglabs/darepo-client/arkrpc" "github.com/lightninglabs/darepo-client/indexer" "github.com/lightninglabs/darepo-client/internal/indexerlimits" + lib_tree "github.com/lightninglabs/darepo-client/lib/tree" mailboxrpc "github.com/lightninglabs/darepo-client/mailbox/rpc" "github.com/lightninglabs/darepo-client/oor" fn "github.com/lightningnetwork/lnd/fn/v2" @@ -89,6 +91,104 @@ func TestResolveIncomingMetadataFromIndexerCapsScannedVTXOs(t *testing.T) { require.Equal(t, 1, rpcClient.sendCount()) } +// TestResolveIncomingMetadataFromIndexerRejectsZeroTreeDepth verifies a +// malicious indexer cannot strand an OOR-received VTXO by returning a +// matching VTXO whose AncestryPath claims tree_depth = 0. Without +// validation at this trust boundary the descriptor would persist as +// "valid" and only fail later during unilateral exit or under-report +// the worst-case CSV window for expiry monitoring — both fund-availability +// issues. This is the regression test for darepo-client#370. +func TestResolveIncomingMetadataFromIndexerRejectsZeroTreeDepth(t *testing.T) { + t.Parallel() + + sessionID := oor.SessionID(testTxID(1)) + candidate := testIncomingVTXO(sessionID, recipientIndex) + + // Override the otherwise-valid ancestry with a zero tree_depth + // claim. The reconstructed tree still has depth 1, so this models + // an indexer that returns a usable tree path but under-reports the + // scalar that drives expiry/refresh decisions. + candidate.AncestryPaths[0].TreeDepth = 0 + + idx, _, recipient, _ := newTestIncomingMetadataIndexer( + t, + &arkrpc.ListVTXOsByScriptsResponse{ + Vtxos: []*arkrpc.VTXO{candidate}, + }, + ) + + _, err := ResolveIncomingMetadataFromIndexerWithLimits( + t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{ + MaxVTXOMatches: 1, + }, + ) + require.Error(t, err) + require.ErrorContains(t, err, "tree_depth") +} + +// TestResolveIncomingMetadataFromIndexerRejectsDepthMismatch verifies the +// receive boundary rejects an AncestryPath whose claimed tree_depth +// disagrees with the depth of the supplied tree_path. A low-but-non-zero +// claim is the more dangerous variant of darepo-client#370 because it +// passes the obvious "zero" check downstream but still under-reports +// MaxTreeDepth for expiry monitoring. +func TestResolveIncomingMetadataFromIndexerRejectsDepthMismatch(t *testing.T) { + t.Parallel() + + sessionID := oor.SessionID(testTxID(1)) + candidate := testIncomingVTXO(sessionID, recipientIndex) + + // Lie about the depth: reconstructed tree depth is 1; claim 7. + candidate.AncestryPaths[0].TreeDepth = 7 + + idx, _, recipient, _ := newTestIncomingMetadataIndexer( + t, + &arkrpc.ListVTXOsByScriptsResponse{ + Vtxos: []*arkrpc.VTXO{candidate}, + }, + ) + + _, err := ResolveIncomingMetadataFromIndexerWithLimits( + t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{ + MaxVTXOMatches: 1, + }, + ) + require.Error(t, err) + require.ErrorContains(t, err, "does not match reconstructed") +} + +// TestResolveIncomingMetadataFromIndexerRejectsOverCapTreeDepth verifies +// the receive boundary rejects an AncestryPath whose claimed tree_depth +// exceeds the receive-path walk cap (arkrpc.MaxAncestryTreeWalkDepth). +// Such a claim cannot be honoured by the same client that persisted it, +// so accepting it would silently strand the VTXO. +func TestResolveIncomingMetadataFromIndexerRejectsOverCapTreeDepth( + t *testing.T) { + + t.Parallel() + + sessionID := oor.SessionID(testTxID(1)) + candidate := testIncomingVTXO(sessionID, recipientIndex) + + candidate.AncestryPaths[0].TreeDepth = arkrpc.MaxAncestryTreeWalkDepth + + 1 + + idx, _, recipient, _ := newTestIncomingMetadataIndexer( + t, + &arkrpc.ListVTXOsByScriptsResponse{ + Vtxos: []*arkrpc.VTXO{candidate}, + }, + ) + + _, err := ResolveIncomingMetadataFromIndexerWithLimits( + t.Context(), idx, sessionID, recipient, oor.ReceiveLimits{ + MaxVTXOMatches: 1, + }, + ) + require.Error(t, err) + require.ErrorContains(t, err, "exceeds max") +} + // TestResolveIncomingMetadataFromIndexerAllowsMatchAtScanLimit verifies the // scan cap is enforced per candidate, so a valid match at the final permitted // item is still accepted. @@ -239,13 +339,32 @@ func testIncomingVTXO(sessionID oor.SessionID, BatchExpiryHeight: 1000, OperatorPubkey: testPubKeyBytes(3), ChainDepth: 1, - AncestryPaths: []*arkrpc.AncestryPath{{ - CommitmentTxid: testTxIDBytes(13), - TreeDepth: 0, - }}, + AncestryPaths: []*arkrpc.AncestryPath{ + testAncestryPath(testTxID(13)), + }, } } +// testAncestryPath returns a minimally valid AncestryPath whose +// reconstructed tree depth matches the wire-format tree_depth. Receive-time +// validation (arkrpc.ValidateAncestryPathDepth) rejects zero or +// inconsistent depths, so test fixtures must keep these in sync. +func testAncestryPath(commitmentTxID chainhash.Hash) *arkrpc.AncestryPath { + t := &lib_tree.Tree{ + Root: &lib_tree.Node{}, + BatchOutpoint: wire.OutPoint{ + Hash: commitmentTxID, + }, + } + + p, err := arkrpc.AncestryPathFromTree(t, commitmentTxID, []uint32{0}) + if err != nil { + panic(fmt.Sprintf("build test ancestry path: %v", err)) + } + + return p +} + // testPubKeyBytes returns a deterministic compressed public key. func testPubKeyBytes(prefix byte) []byte { privKey, _ := btcec.PrivKeyFromBytes(testTxIDBytes(prefix)) diff --git a/oor/incoming_metadata_query.go b/oor/incoming_metadata_query.go index a4ce2e158..558b81b20 100644 --- a/oor/incoming_metadata_query.go +++ b/oor/incoming_metadata_query.go @@ -227,6 +227,20 @@ func ancestryFromRPC(paths []*arkrpc.AncestryPath) ([]vtxo.Ancestry, error) { err) } + // Validate the indexer-supplied tree_depth against the + // reconstructed path before it can be persisted. A zero or + // truncated claim would otherwise survive the rest of the + // receive-side checks and only fail at unilateral-exit time + // (zero) or under-report the worst-case CSV window + // (truncated), which is a fund-availability surface for + // OOR-received VTXOs. + err = arkrpc.ValidateAncestryPathDepth( + p.GetTreeDepth(), treePath, + ) + if err != nil { + return nil, fmt.Errorf("path[%d] depth: %w", i, err) + } + out = append(out, vtxo.Ancestry{ TreePath: treePath, CommitmentTxID: commitmentTxID, diff --git a/oor/incoming_metadata_query_test.go b/oor/incoming_metadata_query_test.go index 8e6e06b72..5eb780644 100644 --- a/oor/incoming_metadata_query_test.go +++ b/oor/incoming_metadata_query_test.go @@ -22,9 +22,9 @@ func TestIncomingMetadataFromRPCOperatorKey(t *testing.T) { RoundId: "round-keyed", CommitmentTxid: commitmentTxID[:], OperatorPubkey: operatorKey.PubKey().SerializeCompressed(), - AncestryPaths: []*arkrpc.AncestryPath{{ - CommitmentTxid: commitmentTxID[:], - }}, + AncestryPaths: []*arkrpc.AncestryPath{ + testValidAncestryPath(commitmentTxID), + }, }) require.NoError(t, err) require.NotNil(t, meta.OperatorKey) @@ -40,9 +40,9 @@ func TestIncomingMetadataFromRPCLegacyOperatorKey(t *testing.T) { meta, err := incomingMetadataFromRPC(&arkrpc.VTXO{ RoundId: "round-legacy", CommitmentTxid: commitmentTxID[:], - AncestryPaths: []*arkrpc.AncestryPath{{ - CommitmentTxid: commitmentTxID[:], - }}, + AncestryPaths: []*arkrpc.AncestryPath{ + testValidAncestryPath(commitmentTxID), + }, }) require.NoError(t, err) require.Nil(t, meta.OperatorKey) @@ -58,9 +58,28 @@ func TestIncomingMetadataFromRPCRejectsInvalidOperatorKey(t *testing.T) { RoundId: "round-invalid-key", CommitmentTxid: commitmentTxID[:], OperatorPubkey: []byte{0x02}, - AncestryPaths: []*arkrpc.AncestryPath{{ - CommitmentTxid: commitmentTxID[:], - }}, + AncestryPaths: []*arkrpc.AncestryPath{ + testValidAncestryPath(commitmentTxID), + }, }) require.ErrorContains(t, err, "parse indexer vtxo operator pubkey") } + +// TestIncomingMetadataFromRPCRejectsZeroTreeDepth is the unit-level +// regression for darepo-client#370 on the OOR-package boundary: a +// matching VTXO whose AncestryPath claims tree_depth = 0 must be +// rejected before persistence. +func TestIncomingMetadataFromRPCRejectsZeroTreeDepth(t *testing.T) { + t.Parallel() + + commitmentTxID := chainhash.Hash{0xab} + path := testValidAncestryPath(commitmentTxID) + path.TreeDepth = 0 + + _, err := incomingMetadataFromRPC(&arkrpc.VTXO{ + RoundId: "round-zero-depth", + CommitmentTxid: commitmentTxID[:], + AncestryPaths: []*arkrpc.AncestryPath{path}, + }) + require.ErrorContains(t, err, "tree_depth must be non-zero") +} diff --git a/oor/incoming_vtxo.go b/oor/incoming_vtxo.go index 126c4289f..fa47be107 100644 --- a/oor/incoming_vtxo.go +++ b/oor/incoming_vtxo.go @@ -10,6 +10,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" + "github.com/lightninglabs/darepo-client/arkrpc" "github.com/lightninglabs/darepo-client/lib/arkscript" "github.com/lightninglabs/darepo-client/lib/tx/arktx" "github.com/lightninglabs/darepo-client/vtxo" @@ -296,6 +297,25 @@ func validateIncomingAncestry(meta IncomingVTXOMetadata, } } + // Validate the per-fragment tree depth here as a + // defense-in-depth check. The RPC ingress + // (arkrpc.ValidateAncestryPathDepth) is the primary trust + // boundary, but any code path that constructs Ancestry + // in-process (tests, future internal materializers) must + // also enforce that the stored TreeDepth matches the actual + // path. Without this, an under-reported depth could survive + // to drive expiry-monitoring decisions and silently delay a + // unilateral exit past its safe deadline. + if err := arkrpc.ValidateAncestryPathDepth( + frag.TreeDepth, frag.TreePath, + ); err != nil { + return &ErrInvalidAncestry{ + Reason: fmt.Sprintf( + "fragment %d depth: %v", i, err, + ), + } + } + // Bind the supplied tree path to its claimed commitment. // The TreePath.BatchOutpoint is the batch output of the // commitment tx the path extracts from; if the operator diff --git a/oor/incoming_vtxo_test.go b/oor/incoming_vtxo_test.go index 8482b1d76..2dcbc91a6 100644 --- a/oor/incoming_vtxo_test.go +++ b/oor/incoming_vtxo_test.go @@ -257,6 +257,28 @@ func TestBuildIncomingVTXODescriptorRejectsInvalidAncestry(t *testing.T) { }, wantReason: "nil tree path", }, + { + // Regression for darepo-client#370: a zero TreeDepth + // would otherwise persist and either silently strand + // the VTXO at unroll time or under-report the expiry + // window. + name: "zero tree depth", + mutate: func(m *IncomingVTXOMetadata) { + m.Ancestry[0].TreeDepth = 0 + }, + wantReason: "must be non-zero", + }, + { + // Regression for darepo-client#370: a non-zero claim + // that disagrees with the actual tree path is the + // more dangerous variant because it survives the + // obvious zero check downstream. + name: "tree depth disagrees with path", + mutate: func(m *IncomingVTXOMetadata) { + m.Ancestry[0].TreeDepth = 9 + }, + wantReason: "does not match reconstructed", + }, { name: "empty input indices", mutate: func(m *IncomingVTXOMetadata) { diff --git a/oor/receive_limits_test.go b/oor/receive_limits_test.go index 4328eb313..dadf95001 100644 --- a/oor/receive_limits_test.go +++ b/oor/receive_limits_test.go @@ -4,7 +4,9 @@ import ( "testing" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/wire" "github.com/lightninglabs/darepo-client/arkrpc" + lib_tree "github.com/lightninglabs/darepo-client/lib/tree" "github.com/lightninglabs/darepo-client/lib/tx/psbtutil" "github.com/stretchr/testify/require" ) @@ -157,8 +159,29 @@ func testIncomingMetadataVTXO(sessionID SessionID, }, RoundId: "round-configured-limit", CommitmentTxid: commitmentTxID[:], - AncestryPaths: []*arkrpc.AncestryPath{{ - CommitmentTxid: commitmentTxID[:], - }}, + AncestryPaths: []*arkrpc.AncestryPath{ + testValidAncestryPath(commitmentTxID), + }, + } +} + +// testValidAncestryPath returns an AncestryPath whose reconstructed +// tree_depth matches the proto scalar. Receive-time validation +// (arkrpc.ValidateAncestryPathDepth, the darepo-client#370 guard) +// rejects zero or mismatched depths, so test fixtures must keep these +// in sync. +func testValidAncestryPath(commitmentTxID chainhash.Hash) *arkrpc.AncestryPath { + t := &lib_tree.Tree{ + Root: &lib_tree.Node{}, + BatchOutpoint: wire.OutPoint{ + Hash: commitmentTxID, + }, + } + + p, err := arkrpc.AncestryPathFromTree(t, commitmentTxID, []uint32{0}) + if err != nil { + panic("build test ancestry path: " + err.Error()) } + + return p } From 6a09f9f40ccef7c3bb133f56a4aabcea8a8b4d4b Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 14 May 2026 20:31:13 -0700 Subject: [PATCH 8/9] unroll: Drop zero-tree-depth gate that can block legitimate proofs The per-fragment validateProofDescriptorShape rejected any Ancestry fragment whose TreeDepth scalar was zero. TreeDepth is expiry-timing metadata (see vtxo.Descriptor.MaxTreeDepth and vtxo/expiry.go); the proof assembler walks TreePath.Root directly and never reads the scalar. Because incoming OOR ancestry is built from indexer RPC data by copying p.GetTreeDepth() verbatim, a malicious or version-skewed indexer that supplies a non-empty TreePath with TreeDepth omitted or forged to zero can persist a VTXO that accepts and validates fine on receive but is permanently rejected at unroll time. That turns an indexer-controlled scalar into a fund-stranding lever on the cooperative-operator-unavailable path, which is the exact threat model unilateral exit exists to defend against. Drop the gate so a zero TreeDepth no longer blocks proof assembly, and document the receive-side ingest boundary as the proper place to validate the scalar against TreePath.Depth() (issue #370). Closes #372. --- unroll/descriptor_resolver.go | 7 ++--- unroll/proof_assembler.go | 16 ++++++++--- unroll/proof_assembler_test.go | 49 +++++++++++++++++++++++++++++----- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/unroll/descriptor_resolver.go b/unroll/descriptor_resolver.go index 640e20334..e59b72dd5 100644 --- a/unroll/descriptor_resolver.go +++ b/unroll/descriptor_resolver.go @@ -133,9 +133,10 @@ func (r *DescriptorLineageResolver) resolveValidatedLineage(ctx context.Context, } // validateProofDescriptorShape upstream gates every fragment for - // non-nil TreePath, non-empty tree, non-zero CommitmentTxID, and - // non-zero TreeDepth, so we can append every fragment - // unconditionally here. + // non-nil TreePath, non-empty tree, and non-zero CommitmentTxID, + // so we can append every fragment unconditionally here. TreeDepth + // is deliberately not part of that gate (it is expiry-timing + // metadata, not proof material). for _, a := range desc.Ancestry { mat.TreePaths = append(mat.TreePaths, a.TreePath) } diff --git a/unroll/proof_assembler.go b/unroll/proof_assembler.go index 733b6c924..3c6930d51 100644 --- a/unroll/proof_assembler.go +++ b/unroll/proof_assembler.go @@ -344,6 +344,18 @@ func validateProofDescriptorShape(desc *vtxo.Descriptor) error { // with a confusing "tree path missing root" — at which point the // FSM has already advanced into AwaitingMaterialization with a // proof set it can never assemble. Fail fast at the boundary. + // + // Note: TreeDepth is intentionally NOT checked here. The proof + // assembler walks TreePath.Root directly, so the scalar TreeDepth + // is purely expiry-timing metadata (see vtxo.Descriptor.MaxTreeDepth + // and vtxo/expiry.go). An untrusted indexer that returns a + // non-empty TreePath with TreeDepth defaulted/forged to zero must + // NOT block unilateral exit — that would let the operator strand + // otherwise-recoverable funds. Receive-side validation of TreeDepth + // against TreePath.Depth() lives at the ingest boundary + // (oor.validateIncomingAncestry / darepod.ancestryFromRPC); the + // unroll path stays liberal about the scalar so legitimate proofs + // always assemble. for i, frag := range desc.Ancestry { switch { case frag.TreePath == nil: @@ -357,10 +369,6 @@ func validateProofDescriptorShape(desc *vtxo.Descriptor) error { case frag.CommitmentTxID == (chainhash.Hash{}): return fmt.Errorf("%w: ancestry fragment %d missing "+ "commitment txid", ErrUnrollProofUnavailable, i) - - case frag.TreeDepth == 0: - return fmt.Errorf("%w: ancestry fragment %d has zero "+ - "tree depth", ErrUnrollProofUnavailable, i) } } diff --git a/unroll/proof_assembler_test.go b/unroll/proof_assembler_test.go index b8f849885..5f4b2d9bd 100644 --- a/unroll/proof_assembler_test.go +++ b/unroll/proof_assembler_test.go @@ -68,13 +68,6 @@ func TestValidateProofDescriptorRejectsMalformedAncestry(t *testing.T) { }, wantReason: "fragment 0 missing commitment txid", }, - { - name: "fragment 0 zero tree depth", - mutate: func(d *vtxo.Descriptor) { - d.Ancestry[0].TreeDepth = 0 - }, - wantReason: "ancestry fragment 0 has zero tree depth", - }, { name: "fragment 1 nil tree path (multi-fragment)", mutate: func(d *vtxo.Descriptor) { @@ -102,6 +95,48 @@ func TestValidateProofDescriptorRejectsMalformedAncestry(t *testing.T) { } } +// TestValidateProofDescriptorAcceptsZeroTreeDepth is the regression +// guard for #372 ("Untrusted zero tree depth can block unroll +// proofs"). TreeDepth is expiry-timing metadata, not proof material — +// the proof assembler walks TreePath directly. A malicious indexer +// that supplies a non-empty TreePath but a defaulted/forged TreeDepth +// of zero must NOT prevent unilateral exit; otherwise the operator +// can strand otherwise-recoverable funds simply by zeroing one +// scalar. +// +// The test also asserts the gate has no sticky state: repeated calls +// with the same zero-depth descriptor keep succeeding, so an unroll +// retry after a transient failure earlier in the pipeline still +// reaches proof assembly. +func TestValidateProofDescriptorAcceptsZeroTreeDepth(t *testing.T) { + t.Parallel() + + desc := &vtxo.Descriptor{ + CommitmentTxID: chainhash.HashH([]byte("commit")), + RoundID: "round-1", + CreatedHeight: 100, + BatchExpiry: 1000, + RelativeExpiry: 144, + Status: vtxo.VTXOStatusLive, + Ancestry: []vtxo.Ancestry{{ + TreePath: &tree.Tree{ + Root: &tree.Node{}, + }, + CommitmentTxID: chainhash.HashH([]byte("frag")), + // Zero TreeDepth: hostile/legacy/forged indexer value. + // Proof assembly only needs TreePath, so this must + // pass. + TreeDepth: 0, + }}, + } + + // Two calls in a row exercise the "no sticky state" invariant: + // the unroll boundary cannot persist a rejection from one call + // into the next. + require.NoError(t, validateProofDescriptor(desc)) + require.NoError(t, validateProofDescriptor(desc)) +} + // TestValidateProofDescriptorAcceptsWellFormedMultiFragment is the // positive companion to the rejection table above: a structurally clean // multi-fragment descriptor must pass validation cleanly so the unroll From 8f70cd2726ae9844996c4b01b850d461c7b05db0 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Fri, 15 May 2026 05:32:52 -0700 Subject: [PATCH 9/9] serverconn: Sign TLS leaf SPKI with mailbox key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448. --- darepod/outbound_clients.go | 8 ++ darepod/server.go | 148 +++++++++++++++++++++++--------- serverconn/mailbox_auth.go | 120 ++++++++++++++++++++++++++ serverconn/mailbox_auth_test.go | 75 ++++++++++++++++ serverconn/types.go | 54 ++++++++++-- serverconn/types_test.go | 42 +++++++++ 6 files changed, 397 insertions(+), 50 deletions(-) diff --git a/darepod/outbound_clients.go b/darepod/outbound_clients.go index bd4f37c7d..d0dd15532 100644 --- a/darepod/outbound_clients.go +++ b/darepod/outbound_clients.go @@ -81,6 +81,14 @@ func (s *Server) serverClientTLSCerts() ([]tls.Certificate, error) { return nil, fmt.Errorf("generate client TLS cert: %w", err) } + // Cache the leaf SubjectPublicKeyInfo bytes so the mailbox + // transport can sign over them and the server can verify the + // secp256k1 identity is bound to the TLS leaf it observes + // (issue #448). + if clientCert.Leaf != nil { + s.tlsLeafSPKI = clientCert.Leaf.RawSubjectPublicKeyInfo + } + return []tls.Certificate{clientCert}, nil } diff --git a/darepod/server.go b/darepod/server.go index 5b8bd0381..2733c07b2 100644 --- a/darepod/server.go +++ b/darepod/server.go @@ -219,6 +219,20 @@ type Server struct { // so response envelopes can include it without re-computing. authSigHex string + // tlsLeafSPKI is the DER-encoded SubjectPublicKeyInfo of the + // P-256 client TLS leaf certificate this daemon dialed with. + // It is captured during dialServer and used to compute the + // secp256k1 → TLS-leaf binding signature that the server + // verifies against the leaf it observes on the connection + // (issue #448). Empty when TLS is disabled (Server.Insecure). + tlsLeafSPKI []byte + + // tlsBindSigHex caches the hex-encoded Schnorr signature + // binding the client's secp256k1 identity to its TLS leaf + // SPKI, so direct (non-connector) response envelope sends + // can attach the binding header without re-signing. + tlsBindSigHex string + runtime *serverconn.Runtime ark *arkrpc.ArkServiceMailboxClient indexer *indexer.Client @@ -2705,13 +2719,19 @@ func (s *Server) handleInboundRPC(ctx context.Context, } // Include the auth signature in response headers so the - // server can verify identity on all envelopes. + // server can verify identity on all envelopes. Also include + // the TLS-binding signature so the server can complete + // first-contact registration even if this response is the + // first envelope it sees from us. if headers == nil { - headers = make(map[string]string, 1) + headers = make(map[string]string, 2) } if s.authSigHex != "" { headers[serverconn.AuthHeaderKey] = s.authSigHex } + if s.tlsBindSigHex != "" { + headers[serverconn.TLSBindHeaderKey] = s.tlsBindSigHex + } responseEnv := &mailboxpb.Envelope{ ProtocolVersion: env.ProtocolVersion, @@ -3006,6 +3026,24 @@ func (s *Server) connectAndBootstrapMailbox(ctx context.Context) error { s.authSigHex = hex.EncodeToString(authSig.Serialize()) + // When TLS is enabled (production / regtest with mTLS), bind + // the secp256k1 identity to the TLS leaf the server will + // observe. The server uses this on first-contact Send to + // reject envelopes whose claimed sender is not actually the + // owner of the connection's TLS leaf, closing the + // registration-time replay window described in issue #448. + var tlsBindSig *schnorr.Signature + if len(s.tlsLeafSPKI) > 0 { + tlsBindSig, err = s.signMailboxTLSBind(ctx, s.tlsLeafSPKI) + if err != nil { + return fmt.Errorf("sign mailbox tls bind: %w", err) + } + + s.tlsBindSigHex = hex.EncodeToString( + tlsBindSig.Serialize(), + ) + } + connCfg := serverconn.DefaultConnectorConfig() connCfg.Edge = edge connCfg.LocalMailboxID = s.localMailboxID @@ -3014,6 +3052,7 @@ func (s *Server) connectAndBootstrapMailbox(ctx context.Context) error { connCfg.Store = s.deliveryStore connCfg.Dispatchers = dispatchers connCfg.AuthSignature = authSig + connCfg.TLSBindSignature = tlsBindSig connCfg.InitAuthHeader() connCfg.DurableUnaryBuilder = &serverDurableUnaryBuilder{ server: s, @@ -3816,28 +3855,59 @@ func (s *Server) fetchOperatorPubKeyDirect(ctx context.Context) ( func (s *Server) signMailboxAuth(ctx context.Context, recipientMailboxID string) (*schnorr.Signature, error) { + msg := serverconn.MailboxAuthMessage( + s.clientKeyDesc.PubKey, recipientMailboxID, + ) + tag := []byte(serverconn.MailboxAuthTagStr) + + return s.signTaggedSchnorr(ctx, msg, tag, "mailbox auth") +} + +// signMailboxTLSBind signs the BIP-340 tagged digest binding the +// client's secp256k1 mailbox identity to the SubjectPublicKeyInfo +// of the active TLS leaf certificate. The signature is sent in the +// x-mailbox-tls-bind-sig header on every outbound envelope so the +// server can verify, on first contact, that the secp256k1 holder +// chose this exact TLS leaf — preventing a captured Send from +// being replayed across a different TLS connection (issue #448). +func (s *Server) signMailboxTLSBind(ctx context.Context, tlsLeafSPKI []byte) ( + *schnorr.Signature, error) { + + msg := serverconn.MailboxTLSBindMessage( + s.clientKeyDesc.PubKey, tlsLeafSPKI, + ) + tag := []byte(serverconn.MailboxTLSBindTagStr) + + return s.signTaggedSchnorr(ctx, msg, tag, "mailbox tls bind") +} + +// signTaggedSchnorr produces a BIP-340 tagged Schnorr signature over +// msg under the client's identity key, dispatching to whichever +// wallet backend is configured (LND, lwwallet, or btcwallet). The +// opName label is woven into error messages so callers can tell +// which signing purpose (e.g. "mailbox auth", "mailbox tls bind") +// the failure originated from. Private key material never leaves +// the wallet — LND signs via its tagged SignMessage RPC, and the +// keyring-backed wallets sign via SignMessageSchnorr. +func (s *Server) signTaggedSchnorr(ctx context.Context, msg, tag []byte, + opName string) (*schnorr.Signature, error) { + var ( sig *schnorr.Signature err error ) - // In LND mode, use lnd's tagged Schnorr signing RPC. We - // pass the raw message and tag so LND computes the BIP-340 - // tagged hash internally, avoiding double-hashing. + // In LND mode, use lnd's tagged Schnorr signing RPC. We pass the + // raw message and tag so LND computes the BIP-340 tagged hash + // internally, avoiding double-hashing. s.lnd.WhenSome(func(lndSvc *lndclient.GrpcLndServices) { - msg := serverconn.MailboxAuthMessage( - s.clientKeyDesc.PubKey, recipientMailboxID, - ) - - tag := []byte(serverconn.MailboxAuthTagStr) - var rawSig []byte rawSig, err = lndSvc.Signer.SignMessage( ctx, msg, s.clientKeyDesc.KeyLocator, lndclient.SignSchnorr(nil), withSchnorrTag(tag), ) if err != nil { - err = fmt.Errorf("lnd sign mailbox auth: %w", err) + err = fmt.Errorf("lnd sign %s: %w", opName, err) return } @@ -3849,11 +3919,11 @@ func (s *Server) signMailboxAuth(ctx context.Context, return sig, err } - // In lwwallet mode, use the keyring's Schnorr signing - // directly — no private key extraction needed. + // In lwwallet mode, use the keyring's Schnorr signing directly + // — no private key extraction needed. s.lwWallet.WhenSome(func(w *lwwallet.Wallet) { - sig, err = s.signMailboxAuthViaKeyRing( - w.KeyRing(), recipientMailboxID, + sig, err = s.signTaggedSchnorrViaKeyRing( + w.KeyRing(), msg, tag, opName, ) }) @@ -3861,52 +3931,46 @@ func (s *Server) signMailboxAuth(ctx context.Context, return sig, err } - // In btcwallet mode, use the neutrino-backed keyring's - // Schnorr signing — same interface, no private key - // extraction. + // In btcwallet mode, use the neutrino-backed keyring's Schnorr + // signing — same interface, no private key extraction. s.btcwWallet.WhenSome(func(w *btcwbackend.Wallet) { - sig, err = s.signMailboxAuthViaKeyRing( - w.KeyRing(), recipientMailboxID, + sig, err = s.signTaggedSchnorrViaKeyRing( + w.KeyRing(), msg, tag, opName, ) }) if sig == nil && err == nil { - return nil, fmt.Errorf("no wallet backend available to sign " + - "mailbox auth") + return nil, fmt.Errorf("no wallet backend available to sign %s", + opName) } return sig, err } -// withSchnorrTag applies a BIP-340 tag to lnd's SignMessage request. -func withSchnorrTag(tag []byte) lndclient.SignMessageOption { - return func(req *signrpc.SignMessageReq) { - req.Tag = tag - } -} - -// signMailboxAuthViaKeyRing signs the mailbox auth digest using a -// keyring's SignMessageSchnorr method. This avoids extracting private -// keys — the keyring handles signing internally. The BIP-340 tagged -// hash is computed by the keyring via the tag parameter. -func (s *Server) signMailboxAuthViaKeyRing(keyRing keychain.SecretKeyRing, - recipientMailboxID string) (*schnorr.Signature, error) { - - msg := serverconn.MailboxAuthMessage( - s.clientKeyDesc.PubKey, recipientMailboxID, - ) - tag := []byte(serverconn.MailboxAuthTagStr) +// signTaggedSchnorrViaKeyRing signs msg using the keyring's +// SignMessageSchnorr method with the supplied BIP-340 tag, avoiding +// any private key extraction. opName is woven into the error +// message so the caller can tell which signing purpose failed. +func (s *Server) signTaggedSchnorrViaKeyRing(keyRing keychain.SecretKeyRing, + msg, tag []byte, opName string) (*schnorr.Signature, error) { sig, err := keyRing.SignMessageSchnorr( s.clientKeyDesc.KeyLocator, msg, false, nil, tag, ) if err != nil { - return nil, fmt.Errorf("keyring sign mailbox auth: %w", err) + return nil, fmt.Errorf("keyring sign %s: %w", opName, err) } return sig, nil } +// withSchnorrTag applies a BIP-340 tag to lnd's SignMessage request. +func withSchnorrTag(tag []byte) lndclient.SignMessageOption { + return func(req *signrpc.SignMessageReq) { + req.Tag = tag + } +} + // networkToLndclient maps our network string to the lndclient network type. func networkToLndclient(network string) (lndclient.Network, error) { switch network { diff --git a/serverconn/mailbox_auth.go b/serverconn/mailbox_auth.go index f27ffe599..eb637c0d3 100644 --- a/serverconn/mailbox_auth.go +++ b/serverconn/mailbox_auth.go @@ -14,11 +14,30 @@ import ( // to their claimed pubkey-derived mailbox ID. const AuthHeaderKey = "x-mailbox-auth-sig" +// TLSBindHeaderKey is the envelope header key that carries the +// Schnorr signature binding the sender's secp256k1 mailbox identity +// to the SubjectPublicKeyInfo of the P-256 TLS leaf certificate +// presented on the same connection. The server verifies this +// signature against the leaf pubkey it actually observed during the +// TLS handshake before recording the cert fingerprint binding for +// the mailbox identity (issue #448). Without it, an attacker who +// replays a captured Send envelope across a fresh TLS connection +// of their own would have their leaf fingerprint bound to the +// victim's mailbox ID. +const TLSBindHeaderKey = "x-mailbox-tls-bind-sig" + // MailboxAuthTagStr is the BIP-340 tagged hash domain separator used // when constructing the message digest for mailbox authentication // signatures. This prevents cross-protocol signature reuse. const MailboxAuthTagStr = "mailbox-auth" +// MailboxTLSBindTagStr is the BIP-340 tagged hash domain separator +// used when constructing the message digest that binds a +// secp256k1 mailbox identity to the TLS leaf SubjectPublicKeyInfo. +// Using a dedicated tag prevents a mailbox-auth signature from +// being reinterpreted as a TLS-binding signature and vice versa. +const MailboxTLSBindTagStr = "mailbox-tls-bind" + // MailboxAuthMessage returns the raw message bytes that are fed into // the BIP-340 tagged hash for mailbox authentication: // @@ -101,6 +120,107 @@ func VerifyMailboxAuth(senderPubKey *btcec.PublicKey, recipientMailboxID string, return nil } +// MailboxTLSBindMessage returns the raw message bytes that are fed +// into the BIP-340 tagged hash for binding a mailbox identity to a +// TLS leaf certificate: +// +// senderCompressedPubKey || tlsLeafSPKIDER +// +// The leaf is identified by its SubjectPublicKeyInfo DER bytes (i.e. +// the x509.Certificate.RawSubjectPublicKeyInfo field). SPKI is the +// stable, self-describing serialization of the public key plus +// algorithm parameters, and is what TLS uses internally to commit +// to the cert's key in the CertificateVerify proof of possession. +// Hashing the SPKI rather than just the raw key bytes also captures +// the curve/algorithm identifier, so a P-256 leaf cannot be +// confused with a leaf using a different curve carrying the same +// raw coordinates. +func MailboxTLSBindMessage(senderPubKey *btcec.PublicKey, + tlsLeafSPKI []byte) []byte { + + pubBytes := senderPubKey.SerializeCompressed() + msg := make([]byte, 0, len(pubBytes)+len(tlsLeafSPKI)) + msg = append(msg, pubBytes...) + msg = append(msg, tlsLeafSPKI...) + + return msg +} + +// MailboxTLSBindDigest constructs the BIP-340 tagged hash digest the +// client signs to bind its secp256k1 identity to the TLS leaf +// SubjectPublicKeyInfo: +// +// TaggedHash("mailbox-tls-bind", senderPubKey || tlsLeafSPKI) +// +// Using a dedicated tag keeps this digest disjoint from the regular +// MailboxAuthDigest so neither signature can be replayed across the +// two purposes. +func MailboxTLSBindDigest(senderPubKey *btcec.PublicKey, + tlsLeafSPKI []byte) [32]byte { + + msg := MailboxTLSBindMessage(senderPubKey, tlsLeafSPKI) + hash := chainhash.TaggedHash( + []byte(MailboxTLSBindTagStr), msg, + ) + + return *hash +} + +// SignMailboxTLSBind produces a Schnorr signature over the +// mailbox-to-TLS-leaf binding digest, proving the caller holds the +// secp256k1 private key for senderPubKey AND has chosen tlsLeafSPKI +// as the leaf the server should expect to observe on this +// connection. +func SignMailboxTLSBind(privKey *btcec.PrivateKey, + tlsLeafSPKI []byte) (*schnorr.Signature, error) { + + if len(tlsLeafSPKI) == 0 { + return nil, fmt.Errorf("tls leaf SPKI must not be empty") + } + + digest := MailboxTLSBindDigest(privKey.PubKey(), tlsLeafSPKI) + + sig, err := schnorr.Sign(privKey, digest[:]) + if err != nil { + return nil, fmt.Errorf("schnorr sign tls bind: %w", err) + } + + return sig, nil +} + +// VerifyMailboxTLSBind verifies that sigHex is a valid Schnorr +// signature binding senderPubKey to tlsLeafSPKI. Returns nil on +// success. The caller is responsible for sourcing tlsLeafSPKI from +// the observed TLS connection, not from the envelope or any other +// client-supplied field. +func VerifyMailboxTLSBind(senderPubKey *btcec.PublicKey, tlsLeafSPKI []byte, + sigHex string) error { + + if len(tlsLeafSPKI) == 0 { + return fmt.Errorf("tls leaf SPKI must not be empty") + } + + sigBytes, err := hex.DecodeString(sigHex) + if err != nil { + return fmt.Errorf("decode tls bind sig hex: %w", err) + } + + sig, err := schnorr.ParseSignature(sigBytes) + if err != nil { + return fmt.Errorf("parse tls bind sig: %w", err) + } + + digest := MailboxTLSBindDigest(senderPubKey, tlsLeafSPKI) + + if !sig.Verify(digest[:], senderPubKey) { + return fmt.Errorf("mailbox tls-bind signature verification "+ + "failed for sender %x", + senderPubKey.SerializeCompressed()) + } + + return nil +} + // ParseMailboxPubKey extracts the public key from a pubkey-derived // mailbox ID string (hex-encoded compressed SEC pubkey). func ParseMailboxPubKey(mailboxID string) (*btcec.PublicKey, error) { diff --git a/serverconn/mailbox_auth_test.go b/serverconn/mailbox_auth_test.go index 60669b8d7..9a891a495 100644 --- a/serverconn/mailbox_auth_test.go +++ b/serverconn/mailbox_auth_test.go @@ -103,6 +103,81 @@ func TestVerifyMailboxAuthBadHex(t *testing.T) { require.Contains(t, err.Error(), "decode auth sig hex") } +// TestSignVerifyMailboxTLSBind exercises the secp256k1 → TLS leaf +// SPKI binding signature: a valid signature passes against the +// signing key and leaf, and is rejected against any other key or +// any other leaf SPKI. The test also confirms that the binding +// digest is disjoint from the mailbox-auth digest, so the two +// signatures cannot be swapped between header slots. +func TestSignVerifyMailboxTLSBind(t *testing.T) { + t.Parallel() + + clientKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + otherKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + // Two distinct fake SPKI byte strings standing in for two + // different TLS leaves. The signing/verification path does + // not parse them, so opaque bytes are sufficient. + leafSPKI := []byte("leaf-spki-1") + otherLeafSPKI := []byte("leaf-spki-2") + + sig, err := SignMailboxTLSBind(clientKey, leafSPKI) + require.NoError(t, err) + require.NotNil(t, sig) + + sigHex := hex.EncodeToString(sig.Serialize()) + + // Correct key + correct leaf: pass. + err = VerifyMailboxTLSBind(clientKey.PubKey(), leafSPKI, sigHex) + require.NoError(t, err) + + // Wrong signing key: fail. + err = VerifyMailboxTLSBind(otherKey.PubKey(), leafSPKI, sigHex) + require.Error(t, err) + + // Wrong leaf SPKI: fail (this is the registration-replay + // case from issue #448). + err = VerifyMailboxTLSBind(clientKey.PubKey(), otherLeafSPKI, sigHex) + require.Error(t, err) + + // Empty SPKI: rejected upfront. + err = VerifyMailboxTLSBind(clientKey.PubKey(), nil, sigHex) + require.Error(t, err) + + // A mailbox-auth signature must not verify as a TLS-bind + // signature. Sign over the auth digest then attempt to + // verify with the TLS-bind verifier — different tag means + // digest mismatch and rejection. + authSig, err := SignMailboxAuth(clientKey, "recipient-mbid") + require.NoError(t, err) + + authSigHex := hex.EncodeToString(authSig.Serialize()) + + err = VerifyMailboxTLSBind( + clientKey.PubKey(), leafSPKI, authSigHex, + ) + require.Error(t, err) +} + +// TestSignMailboxTLSBindEmptySPKI guards against accidentally signing +// over an empty SPKI, which would otherwise produce a signature that +// any leaf could verify. +func TestSignMailboxTLSBindEmptySPKI(t *testing.T) { + t.Parallel() + + clientKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + _, err = SignMailboxTLSBind(clientKey, nil) + require.Error(t, err) + + _, err = SignMailboxTLSBind(clientKey, []byte{}) + require.Error(t, err) +} + // TestParseMailboxPubKeyInvalid verifies that invalid mailbox IDs // are rejected. func TestParseMailboxPubKeyInvalid(t *testing.T) { diff --git a/serverconn/types.go b/serverconn/types.go index a1ae80059..980791fa5 100644 --- a/serverconn/types.go +++ b/serverconn/types.go @@ -180,35 +180,68 @@ type ConnectorConfig struct { // server verifies this signature during client registration. AuthSignature *schnorr.Signature + // TLSBindSignature is the Schnorr signature binding the + // client's secp256k1 mailbox identity to the SPKI bytes of + // the TLS leaf certificate this connector dialed with. When + // non-nil, it is serialized as hex and included as the + // x-mailbox-tls-bind-sig header on every outbound envelope. + // The server uses this on first-contact Send to verify the + // TLS leaf it observes is the one the verified identity + // signed over, closing the registration-time replay window + // described in issue #448. + TLSBindSignature *schnorr.Signature + // authSigHex caches the hex-encoded auth signature string, // computed once by InitAuthHeader to avoid per-envelope // serialization. authSigHex string + // tlsBindSigHex caches the hex-encoded TLS-binding signature + // string, computed once by InitAuthHeader. Empty when no + // binding signature is configured. + tlsBindSigHex string + // authHeaderCache holds the singleton auth-only header map // for the common case where callers provide no extra headers. authHeaderCache map[string]string } // InitAuthHeader pre-computes the cached auth header state from -// AuthSignature. Must be called after AuthSignature is set and -// before the first mergeAuthHeaders call. +// AuthSignature and TLSBindSignature. Must be called after both +// signature fields are set (or left nil) and before the first +// mergeAuthHeaders call. func (c *ConnectorConfig) InitAuthHeader() { if c.AuthSignature == nil { + + // TLS binding is meaningful only alongside mailbox auth: + // the server verifies the binding against the same + // Schnorr-authenticated mailbox identity. return } c.authSigHex = hex.EncodeToString(c.AuthSignature.Serialize()) - c.authHeaderCache = map[string]string{ + + cache := map[string]string{ AuthHeaderKey: c.authSigHex, } + + if c.TLSBindSignature != nil { + c.tlsBindSigHex = hex.EncodeToString( + c.TLSBindSignature.Serialize(), + ) + cache[TLSBindHeaderKey] = c.tlsBindSigHex + } + + c.authHeaderCache = cache } // mergeAuthHeaders returns a new header map containing both src -// headers and the auth signature header. If AuthSignature is nil, -// src is returned unchanged. The auth signature header always takes -// precedence over any caller-provided header with the same key to -// prevent accidental or malicious signature replacement. +// headers and the auth signature headers (mailbox-auth and, if +// configured, the TLS-binding signature). If no auth signature is +// configured, src is returned unchanged. Server-bound auth headers +// always take precedence over any caller-provided header with the +// same key to prevent accidental or malicious signature +// replacement. func (c *ConnectorConfig) mergeAuthHeaders( src map[string]string) map[string]string { @@ -221,7 +254,7 @@ func (c *ConnectorConfig) mergeAuthHeaders( return c.authHeaderCache } - merged := make(map[string]string, len(src)+1) + merged := make(map[string]string, len(src)+len(c.authHeaderCache)) // Copy caller-provided headers first. for k, v := range src { @@ -231,6 +264,11 @@ func (c *ConnectorConfig) mergeAuthHeaders( // Auth signature always wins over caller-provided headers. merged[AuthHeaderKey] = c.authSigHex + // TLS-binding signature, if configured, also wins. + if c.tlsBindSigHex != "" { + merged[TLSBindHeaderKey] = c.tlsBindSigHex + } + return merged } diff --git a/serverconn/types_test.go b/serverconn/types_test.go index e7e5e362e..92fca397e 100644 --- a/serverconn/types_test.go +++ b/serverconn/types_test.go @@ -65,6 +65,48 @@ func TestMergeAuthHeadersAuthWins(t *testing.T) { require.Equal(t, "value", result["other"]) } +// TestMergeAuthHeadersIncludesTLSBindSig verifies that when both +// AuthSignature and TLSBindSignature are configured, mergeAuthHeaders +// emits both headers and the TLS-binding header survives a caller- +// supplied collision (same precedence rule as the auth header). +func TestMergeAuthHeadersIncludesTLSBindSig(t *testing.T) { + t.Parallel() + + privKey, err := btcec.NewPrivateKey() + require.NoError(t, err) + + authSig, err := SignMailboxAuth(privKey, "recipient") + require.NoError(t, err) + + bindSig, err := SignMailboxTLSBind(privKey, []byte("leaf-spki")) + require.NoError(t, err) + + cfg := &ConnectorConfig{ + AuthSignature: authSig, + TLSBindSignature: bindSig, + } + cfg.InitAuthHeader() + + // Fast path: nil src returns the cached singleton with both + // headers present. + result := cfg.mergeAuthHeaders(nil) + require.Contains(t, result, AuthHeaderKey) + require.Contains(t, result, TLSBindHeaderKey) + + expectedBindHex := hex.EncodeToString(bindSig.Serialize()) + require.Equal(t, expectedBindHex, result[TLSBindHeaderKey]) + + // Slow path: caller-supplied headers, with a hostile attempt + // to override the TLS-binding header. Real binding sig wins. + src := map[string]string{ + TLSBindHeaderKey: "fake-bind-sig", + "other": "value", + } + result = cfg.mergeAuthHeaders(src) + require.Equal(t, expectedBindHex, result[TLSBindHeaderKey]) + require.Equal(t, "value", result["other"]) +} + // TestPubKeyMailboxIDNilPanics verifies that a nil key causes a panic. func TestPubKeyMailboxIDNilPanics(t *testing.T) { t.Parallel()