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/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/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/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], + ) +} 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/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 38b069ed4..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" @@ -242,6 +243,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 +266,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 { @@ -277,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 @@ -307,6 +346,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 +361,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 +392,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..2dcbc91a6 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 @@ -207,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) { @@ -248,3 +320,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.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..839ed1753 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,170 @@ 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 +} + +// 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 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..dadf95001 100644 --- a/oor/receive_limits_test.go +++ b/oor/receive_limits_test.go @@ -4,7 +4,10 @@ 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" ) @@ -34,6 +37,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( @@ -116,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 +} diff --git a/round/actor_test.go b/round/actor_test.go index 80962d64f..a5148ad25 100644 --- a/round/actor_test.go +++ b/round/actor_test.go @@ -1174,14 +1174,33 @@ 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 (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() require.NoError(t, err) + amount := int64(v.Amount) + if i == 0 { + amount -= operatorFeeSat + } + vtxoQuotes[i] = VTXOQuoteEntry{ PkScript: script, - AmountSat: int64(v.Amount), + AmountSat: amount, RecipientKey: v.SigningKey.PubKey.SerializeCompressed(), } } @@ -1193,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 cf58a8fbc..e3c3759c0 100644 --- a/round/quote_echo_test.go +++ b/round/quote_echo_test.go @@ -1,7 +1,9 @@ package round import ( + "bytes" "context" + "strings" "testing" "time" @@ -9,6 +11,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 +23,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 +53,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 +130,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 +148,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 +176,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 +194,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 +216,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 +238,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 +262,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 +281,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 +357,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 +404,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 +427,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 +459,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 +547,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 +565,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 +583,685 @@ 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 +} + +// 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/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..97faa2097 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 { @@ -900,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 @@ -934,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 + } } } @@ -955,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 + } } } @@ -1003,7 +1154,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 +1346,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{ 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() 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 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)