From cf594b2af2e73147a44f05c921aad12d769245d0 Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:43:03 -0400 Subject: [PATCH 1/7] vtxo: retract an unroll whose transaction the chain no longer has A vtxo is marked unrolled the moment its outpoint appears on chain, before any confirmation. That is deliberate and protective: the mark blocks the vtxo from being spent inside the Ark while its unroll is in flight. Nothing ever cleared it, so an unroll that is evicted or replaced and never mines left the vtxo wrongly unrolled forever, unspendable by its owner through three guards and unsweepable by the operator through two more. Retraction runs on positive evidence in both directions, never on absence alone, which is the discipline the spend reconciler already follows. An outpoint the wallet still lists as unspent exists on chain and no number of passes may retract past it. Only once that is absent does the transaction lookup decide, and only a backend that positively has no record of the transaction counts, over several passes, so a transaction that has been broadcast but has not reached our node yet is not mistaken for one that is gone. The wallet gains the signal that makes this possible. Asking whether a transaction is confirmed answered false both for one waiting in the mempool and for one the backend has never seen, and the difference is the whole point, so the response carries a not_found flag. It is phrased so false is the safe reading: an older wallet never sets it, every transaction reads as known, and no retraction can fire. The repository write is scoped to vtxos still believed unspent, so it can never clear the mark on one spent inside the Ark and then unrolled, which is the fraud path the sweeper resolves through spent_by. --- .../arkwallet/v1/bitcoin_wallet.openapi.json | 4 + .../arkwallet/v1/bitcoin_wallet.proto | 8 ++ .../gen/arkwallet/v1/bitcoin_wallet.pb.go | 28 +++- internal/core/application/indexer_test.go | 5 + internal/core/application/mocks_test.go | 28 ++++ internal/core/application/onchain_spend.go | 4 + internal/core/application/service.go | 7 + internal/core/application/sweeper_test.go | 8 ++ .../core/application/unroll_retraction.go | 127 +++++++++++++++++ .../application/unroll_retraction_test.go | 133 ++++++++++++++++++ internal/core/domain/vtxo_repo.go | 6 + internal/core/ports/scanner.go | 8 ++ .../infrastructure/db/badger/vtxo_repo.go | 22 +++ .../db/onchain_spend_repo_test.go | 35 +++++ .../db/postgres/sqlc/queries/query.sql.go | 19 +++ .../infrastructure/db/postgres/sqlc/query.sql | 8 ++ .../infrastructure/db/postgres/vtxo_repo.go | 19 +++ .../db/sqlite/sqlc/queries/query.sql.go | 19 +++ .../infrastructure/db/sqlite/sqlc/query.sql | 8 ++ .../infrastructure/db/sqlite/vtxo_repo.go | 19 +++ .../tx-builder/covenantless/mocks_test.go | 9 ++ .../infrastructure/wallet/wallet_client.go | 15 ++ .../interface/grpc/handlers/wallet_handler.go | 1 + 23 files changed, 534 insertions(+), 6 deletions(-) create mode 100644 internal/core/application/unroll_retraction.go create mode 100644 internal/core/application/unroll_retraction_test.go diff --git a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json index a266c54b7..a23b25afc 100644 --- a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json +++ b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json @@ -1712,6 +1712,10 @@ }, "confirmed": { "type": "boolean" + }, + "notFound": { + "type": "boolean", + "description": "not_found separates a transaction the backend has no record of from one it\nknows and sees unconfirmed. Both answer confirmed = false, which is why the\ndistinction needs its own field.\n\nAdded after the other fields, and phrased so false is the safe reading: an\nolder wallet never sets it, and a caller then sees \"not known to be\nmissing\" rather than \"missing\"." } } }, diff --git a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto index 6095a9226..94435165c 100644 --- a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto +++ b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto @@ -223,6 +223,14 @@ message IsTransactionConfirmedResponse { bool confirmed = 1; int64 blocknumber = 2; int64 blocktime = 3; + // not_found separates a transaction the backend has no record of from one it + // knows and sees unconfirmed. Both answer confirmed = false, which is why the + // distinction needs its own field. + // + // Added after the other fields, and phrased so false is the safe reading: an + // older wallet never sets it, and a caller then sees "not known to be + // missing" rather than "missing". + bool not_found = 4; } message GetOutpointStatusRequest { diff --git a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go index bb35ae939..d835b2cfd 100644 --- a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go +++ b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go @@ -147,10 +147,18 @@ func (x *IsTransactionConfirmedRequest) GetTxid() string { } type IsTransactionConfirmedResponse struct { - state protoimpl.MessageState `protogen:"open.v1"` - Confirmed bool `protobuf:"varint,1,opt,name=confirmed,proto3" json:"confirmed,omitempty"` - Blocknumber int64 `protobuf:"varint,2,opt,name=blocknumber,proto3" json:"blocknumber,omitempty"` - Blocktime int64 `protobuf:"varint,3,opt,name=blocktime,proto3" json:"blocktime,omitempty"` + state protoimpl.MessageState `protogen:"open.v1"` + Confirmed bool `protobuf:"varint,1,opt,name=confirmed,proto3" json:"confirmed,omitempty"` + Blocknumber int64 `protobuf:"varint,2,opt,name=blocknumber,proto3" json:"blocknumber,omitempty"` + Blocktime int64 `protobuf:"varint,3,opt,name=blocktime,proto3" json:"blocktime,omitempty"` + // not_found separates a transaction the backend has no record of from one it + // knows and sees unconfirmed. Both answer confirmed = false, which is why the + // distinction needs its own field. + // + // Added after the other fields, and phrased so false is the safe reading: an + // older wallet never sets it, and a caller then sees "not known to be + // missing" rather than "missing". + NotFound bool `protobuf:"varint,4,opt,name=not_found,json=notFound,proto3" json:"not_found,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -206,6 +214,13 @@ func (x *IsTransactionConfirmedResponse) GetBlocktime() int64 { return 0 } +func (x *IsTransactionConfirmedResponse) GetNotFound() bool { + if x != nil { + return x.NotFound + } + return false +} + type GetOutpointStatusRequest struct { state protoimpl.MessageState `protogen:"open.v1"` Txid string `protobuf:"bytes,1,opt,name=txid,proto3" json:"txid,omitempty"` @@ -3779,11 +3794,12 @@ const file_arkwallet_v1_bitcoin_wallet_proto_rawDesc = "" + "\x16GetReadyUpdateResponse\x12\x14\n" + "\x05ready\x18\x01 \x01(\bR\x05ready\"3\n" + "\x1dIsTransactionConfirmedRequest\x12\x12\n" + - "\x04txid\x18\x01 \x01(\tR\x04txid\"~\n" + + "\x04txid\x18\x01 \x01(\tR\x04txid\"\x9b\x01\n" + "\x1eIsTransactionConfirmedResponse\x12\x1c\n" + "\tconfirmed\x18\x01 \x01(\bR\tconfirmed\x12 \n" + "\vblocknumber\x18\x02 \x01(\x03R\vblocknumber\x12\x1c\n" + - "\tblocktime\x18\x03 \x01(\x03R\tblocktime\"B\n" + + "\tblocktime\x18\x03 \x01(\x03R\tblocktime\x12\x1b\n" + + "\tnot_found\x18\x04 \x01(\bR\bnotFound\"B\n" + "\x18GetOutpointStatusRequest\x12\x12\n" + "\x04txid\x18\x01 \x01(\tR\x04txid\x12\x12\n" + "\x04vout\x18\x02 \x01(\rR\x04vout\"1\n" + diff --git a/internal/core/application/indexer_test.go b/internal/core/application/indexer_test.go index fd9ee661e..86a79e642 100644 --- a/internal/core/application/indexer_test.go +++ b/internal/core/application/indexer_test.go @@ -1232,6 +1232,11 @@ func (m *mockVtxoRepoForIndexer) SpendVtxos( return nil } +func (m *mockVtxoRepoForIndexer) UnmarkVtxosUnrolled( + ctx context.Context, outpoints []domain.Outpoint, +) error { + return nil +} func (m *mockVtxoRepoForIndexer) UnrollVtxos( ctx context.Context, outpoints []domain.Outpoint, diff --git a/internal/core/application/mocks_test.go b/internal/core/application/mocks_test.go index 5a281fddd..076ad4b5b 100644 --- a/internal/core/application/mocks_test.go +++ b/internal/core/application/mocks_test.go @@ -74,6 +74,12 @@ func (m *mockedVtxoRepo) GetVtxoPubKeysByCommitmentTxids( return nil, args.Error(1) } +func (m *mockedVtxoRepo) UnmarkVtxosUnrolled( + ctx context.Context, outpoints []domain.Outpoint, +) error { + return m.Called(ctx, outpoints).Error(0) +} + func (m *mockedVtxoRepo) MarkVtxosOnchainSpent( ctx context.Context, spentBy map[domain.Outpoint]string, ) error { @@ -207,6 +213,11 @@ type mockedScanner struct { mu sync.Mutex // onchain-spend fixtures, read by the reconcile tests + // known maps a txid to whether the backend still has a record of it; a txid + // absent from the map reads as unknown, which is what a retraction needs. + known map[string]bool + knownErr error + knownCalls []string spendCh chan []ports.Spend spends []ports.Spend spendsErr error @@ -265,6 +276,23 @@ func (m *mockedScanner) RescanUtxos(_ context.Context, _ []wire.OutPoint) error return nil } +func (m *mockedScanner) IsTransactionKnown(_ context.Context, txid string) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + m.knownCalls = append(m.knownCalls, txid) + if m.knownErr != nil { + return false, m.knownErr + } + return m.known[txid], nil +} + +// KnownCalls returns every txid IsTransactionKnown was asked about. +func (m *mockedScanner) KnownCalls() []string { + m.mu.Lock() + defer m.mu.Unlock() + return append([]string(nil), m.knownCalls...) +} + func (m *mockedScanner) GetSpendNotificationChannel( _ context.Context, ) <-chan []ports.Spend { diff --git a/internal/core/application/onchain_spend.go b/internal/core/application/onchain_spend.go index 96c12535f..337d3356b 100644 --- a/internal/core/application/onchain_spend.go +++ b/internal/core/application/onchain_spend.go @@ -166,6 +166,10 @@ func (s *service) reconcileOnchainSpendsOnce(ctx context.Context, from *time.Tim } s.retractStaleOnchainSpends(ctx, recorded) + + // Same candidate set, the other direction: a spend that never confirmed is + // retracted above, an unroll that never confirmed is retracted here. + s.retractStaleUnrolls(ctx, candidates) } // retractStaleOnchainSpends undoes spends whose transaction is gone. diff --git a/internal/core/application/service.go b/internal/core/application/service.go index e834edafb..30bcf26c6 100644 --- a/internal/core/application/service.go +++ b/internal/core/application/service.go @@ -58,6 +58,12 @@ type service struct { // unrolled vtxos onchainSpendReconcileInterval time.Duration + // consecutive reconcile passes that found a vtxo's materialising tx unknown + // to the chain backend, keyed by outpoint. In memory on purpose: losing it + // on restart only delays a retraction, which is the safe direction. + unrollObservations map[domain.Outpoint]int + unrollObservationsMu sync.Mutex + operatorPrvkey *btcec.PrivateKey operatorPubkey *btcec.PublicKey @@ -168,6 +174,7 @@ func NewService( feeManager: feeManager, onchainSpendReconcileInterval: onchainSpendReconcileInterval, + unrollObservations: make(map[domain.Outpoint]int), } svc.sweeper.onSweepCheckpoint = svc.propagateTransactionEvent return svc, nil diff --git a/internal/core/application/sweeper_test.go b/internal/core/application/sweeper_test.go index 29970b01c..84b62d01b 100644 --- a/internal/core/application/sweeper_test.go +++ b/internal/core/application/sweeper_test.go @@ -370,6 +370,9 @@ func (m *mockWalletService) IsTransactionConfirmed( ) (bool, *ports.BlockTimestamp, error) { return false, nil, nil } +func (m *mockWalletService) IsTransactionKnown(ctx context.Context, txid string) (bool, error) { + return true, nil +} func (m *mockWalletService) RescanUtxos(ctx context.Context, outpoints []wire.OutPoint) error { return nil } @@ -438,6 +441,11 @@ func (m *mockVtxoRepository) SpendVtxos( ) error { return nil } +func (m *mockVtxoRepository) UnmarkVtxosUnrolled( + ctx context.Context, outpoints []domain.Outpoint, +) error { + return nil +} func (m *mockVtxoRepository) UnrollVtxos(ctx context.Context, outpoints []domain.Outpoint) error { return nil } diff --git a/internal/core/application/unroll_retraction.go b/internal/core/application/unroll_retraction.go new file mode 100644 index 000000000..4dbf84777 --- /dev/null +++ b/internal/core/application/unroll_retraction.go @@ -0,0 +1,127 @@ +package application + +import ( + "context" + + "github.com/arkade-os/arkd/internal/core/domain" + log "github.com/sirupsen/logrus" +) + +// unrollRetractionObservations is how many consecutive reconcile passes must +// find a vtxo's materialising tx unknown to the chain backend before its unroll +// is retracted. One pass is not enough: a transaction that has been broadcast +// but has not reached our node yet reads exactly like one that is gone, and +// retracting a live unroll would hand its owner back a vtxo whose output exists +// on chain. Several passes apart make that reading stable rather than a race +// with propagation. +const unrollRetractionObservations = 3 + +// retractStaleUnrolls clears the unrolled mark on vtxos whose materialising +// transaction the chain backend no longer has any record of. +// +// A vtxo is marked unrolled the moment its outpoint appears on chain, before any +// confirmation, which is deliberate: the mark is protective and blocks the vtxo +// from being spent inside the Ark while its unroll is in flight. Nothing ever +// cleared it, so an unroll that is evicted or replaced and never mines leaves +// the vtxo wrongly unrolled forever, unspendable by its owner and unsweepable by +// the operator. +// +// Retraction runs on positive evidence in both directions, never on absence +// alone. The outpoint being present in the wallet's unspent set says the output +// exists, and no number of passes can retract past it. Only once that is absent +// does the transaction lookup decide, and only a backend that positively has no +// record of the transaction counts, repeatedly. +func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.Vtxo) { + if len(candidates) == 0 { + s.forgetUnrollObservations(nil) + return + } + + // The unspent set is the stronger signal and is checked first: an outpoint + // listed here exists on chain, whatever the transaction lookup says. + unspent, err := s.scanner.GetUnspentOutpoints(ctx) + if err != nil { + log.WithError(err).Warn( + "unroll retraction: failed to fetch unspent outpoints, skipping this pass", + ) + return + } + + seen := make(map[domain.Outpoint]struct{}, len(candidates)) + stale := make([]domain.Outpoint, 0) + for _, vtxo := range candidates { + seen[vtxo.Outpoint] = struct{}{} + + if _, ok := unspent[vtxo.Outpoint]; ok { + s.recordUnrollObservation(vtxo.Outpoint, true) + continue + } + + known, err := s.scanner.IsTransactionKnown(ctx, vtxo.Txid) + if err != nil { + // Not evidence of anything. Leave the count untouched so a flapping + // backend cannot accumulate its way to a retraction. + log.WithError(err).Warnf( + "unroll retraction: failed to look up tx %s, leaving vtxo %s alone", + vtxo.Txid, vtxo.Outpoint, + ) + continue + } + + if s.recordUnrollObservation(vtxo.Outpoint, known) >= unrollRetractionObservations { + stale = append(stale, vtxo.Outpoint) + } + } + + // Anything no longer a candidate has been resolved by another path, so its + // count must not survive to be counted against a future unroll. + s.forgetUnrollObservations(seen) + + if len(stale) == 0 { + return + } + + if err := s.repoManager.Vtxos().UnmarkVtxosUnrolled(ctx, stale); err != nil { + log.WithError(err).Warn("unroll retraction: failed to retract unrolls") + return + } + + for _, outpoint := range stale { + s.recordUnrollObservation(outpoint, true) + log.Debugf( + "vtxo %s unroll retracted, its tx is no longer known to the chain", outpoint, + ) + } +} + +// recordUnrollObservation advances or clears the consecutive count for an +// outpoint and returns the count after the update. +func (s *service) recordUnrollObservation(outpoint domain.Outpoint, known bool) int { + s.unrollObservationsMu.Lock() + defer s.unrollObservationsMu.Unlock() + + if known { + delete(s.unrollObservations, outpoint) + return 0 + } + // Lazily built so a service assembled without NewService, as the tests do, + // still counts rather than panicking on a nil map. + if s.unrollObservations == nil { + s.unrollObservations = make(map[domain.Outpoint]int) + } + s.unrollObservations[outpoint]++ + return s.unrollObservations[outpoint] +} + +// forgetUnrollObservations drops counts for outpoints outside the given set, +// or all of them when the set is nil. +func (s *service) forgetUnrollObservations(keep map[domain.Outpoint]struct{}) { + s.unrollObservationsMu.Lock() + defer s.unrollObservationsMu.Unlock() + + for outpoint := range s.unrollObservations { + if _, ok := keep[outpoint]; !ok { + delete(s.unrollObservations, outpoint) + } + } +} diff --git a/internal/core/application/unroll_retraction_test.go b/internal/core/application/unroll_retraction_test.go new file mode 100644 index 000000000..594b964ae --- /dev/null +++ b/internal/core/application/unroll_retraction_test.go @@ -0,0 +1,133 @@ +package application + +import ( + "context" + "errors" + "testing" + + "github.com/arkade-os/arkd/internal/core/domain" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestRetractStaleUnrolls(t *testing.T) { + ctx := context.Background() + out := outpoint(unrolledVtxoTxid, 0) + candidates := []domain.Vtxo{{Outpoint: out, Unrolled: true}} + + t.Run("retracts once the tx has been unknown for enough passes", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + known: map[string]bool{}, + }) + vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) + + for range unrollRetractionObservations - 1 { + svc.retractStaleUnrolls(ctx, candidates) + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + } + svc.retractStaleUnrolls(ctx, candidates) + + vtxos.AssertCalled(t, "UnmarkVtxosUnrolled", mock.Anything, []domain.Outpoint{out}) + }) + + // The stronger of the two signals. An outpoint the wallet still lists as + // unspent exists on chain, so no number of passes may retract past it. + t.Run("never retracts while the outpoint is still unspent", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{out: {}}, + known: map[string]bool{}, + }) + + for range unrollRetractionObservations * 2 { + svc.retractStaleUnrolls(ctx, candidates) + } + + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + // A wallet predating the not-found field reports every tx as known, so it + // degrades to never retracting rather than to retracting blindly. + t.Run("a tx the backend still knows resets the count", func(t *testing.T) { + scanner := &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + known: map[string]bool{}, + } + svc, vtxos := unrollService(t, scanner) + + for range unrollRetractionObservations - 1 { + svc.retractStaleUnrolls(ctx, candidates) + } + scanner.known[unrolledVtxoTxid] = true + svc.retractStaleUnrolls(ctx, candidates) + scanner.known[unrolledVtxoTxid] = false + svc.retractStaleUnrolls(ctx, candidates) + + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + // A failing lookup is not evidence, so it must not accumulate towards a + // retraction the way a real "gone" answer does. + t.Run("a lookup failure does not advance the count", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + knownErr: errors.New("wallet down"), + }) + + for range unrollRetractionObservations * 2 { + svc.retractStaleUnrolls(ctx, candidates) + } + + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + t.Run("an unspent-set failure skips the pass without asking further", func(t *testing.T) { + scanner := &mockedScanner{unspentErr: errors.New("wallet down")} + svc, vtxos := unrollService(t, scanner) + + svc.retractStaleUnrolls(ctx, candidates) + + require.Empty(t, scanner.KnownCalls(), "no tx lookup without the stronger signal") + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + // A vtxo that leaves the candidate set has been resolved some other way, so + // its count must not survive to be counted against a later unroll. + t.Run("counts do not survive leaving the candidate set", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + known: map[string]bool{}, + }) + vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) + + for range unrollRetractionObservations - 1 { + svc.retractStaleUnrolls(ctx, candidates) + } + svc.retractStaleUnrolls(ctx, nil) + svc.retractStaleUnrolls(ctx, candidates) + + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + t.Run("no candidates is a no-op", func(t *testing.T) { + scanner := &mockedScanner{} + svc, vtxos := unrollService(t, scanner) + + svc.retractStaleUnrolls(ctx, nil) + + require.Empty(t, scanner.KnownCalls()) + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) +} + +// --- fixtures --- + +// unrollService builds a service wired to the given scanner, with a vtxo repo +// that records retraction calls. +func unrollService(t *testing.T, scanner *mockedScanner) (*service, *mockedVtxoRepo) { + t.Helper() + vtxos := &mockedVtxoRepo{} + rm := &mockedRepoManager{} + rm.On("Vtxos").Return(vtxos) + return &service{repoManager: rm, scanner: scanner}, vtxos +} diff --git a/internal/core/domain/vtxo_repo.go b/internal/core/domain/vtxo_repo.go index c800b58ce..1f10dba23 100644 --- a/internal/core/domain/vtxo_repo.go +++ b/internal/core/domain/vtxo_repo.go @@ -7,6 +7,12 @@ type VtxoRepository interface { SettleVtxos(ctx context.Context, spentVtxos map[Outpoint]string, commitmentTxid string) error SpendVtxos(ctx context.Context, spentVtxos map[Outpoint]string, arkTxid string) error UnrollVtxos(ctx context.Context, outpoints []Outpoint) error + // UnmarkVtxosUnrolled retracts the unrolled mark of vtxos whose materialising + // transaction the chain backend no longer has any record of, so an unroll + // that never confirmed stops making the vtxo permanently unspendable and + // permanently unsweepable. Scoped to vtxos still believed unspent, so it can + // never clear the mark on one spent inside the Ark and then unrolled. + UnmarkVtxosUnrolled(ctx context.Context, outpoints []Outpoint) error // MarkVtxosOnchainSpent records unrolled vtxos spent onchain, outside the // Ark, mapping each outpoint to the txid that spent it. It also re-points an // already onchain-spent vtxo at a new spender, so an RBF replacement is diff --git a/internal/core/ports/scanner.go b/internal/core/ports/scanner.go index 62296068b..20c7594d2 100644 --- a/internal/core/ports/scanner.go +++ b/internal/core/ports/scanner.go @@ -31,6 +31,14 @@ type BlockchainScanner interface { IsTransactionConfirmed( ctx context.Context, txid string, ) (isConfirmed bool, blockTimestamp *BlockTimestamp, err error) + // IsTransactionKnown reports whether the chain backend has any record of the + // transaction, confirmed or still in the mempool. It is deliberately not + // IsTransactionConfirmed: that answers false both for a transaction waiting + // in the mempool and for one the backend has never seen, and the difference + // is the whole signal here. A false answer is positive evidence the + // transaction is gone, which is what separates a replaced or evicted one + // from one merely waiting. + IsTransactionKnown(ctx context.Context, txid string) (bool, error) // GetSpends returns every watched output spent by a confirmed or unconfirmed // transaction, windowed from the given instant when one is supplied. GetSpends(ctx context.Context, from *time.Time) ([]Spend, error) diff --git a/internal/infrastructure/db/badger/vtxo_repo.go b/internal/infrastructure/db/badger/vtxo_repo.go index 67c51fb26..f1eb705d5 100644 --- a/internal/infrastructure/db/badger/vtxo_repo.go +++ b/internal/infrastructure/db/badger/vtxo_repo.go @@ -106,6 +106,28 @@ func (r *VtxoRepository) UnrollVtxos( return nil } +// UnmarkVtxosUnrolled retracts an unroll whose transaction the chain backend no +// longer has any record of. +// +// ExpiresAt is deliberately not restored: unrollVtxo zeroes it and the original +// value is gone, so a retracted vtxo keeps a zero expiry here where the SQL +// backends keep the value they never cleared. That divergence predates this +// method and is recorded rather than papered over. +func (r *VtxoRepository) UnmarkVtxosUnrolled( + ctx context.Context, outpoints []domain.Outpoint, +) error { + return r.inChunkedTx(outpoints, func(tx *badger.Txn, outpoint domain.Outpoint) error { + vtxo, err := r.getVtxoTx(tx, outpoint) + if err != nil || vtxo == nil || !vtxo.Unrolled || vtxo.Spent { + return err + } + + vtxo.Unrolled = false + + return r.updateVtxoTx(tx, vtxo) + }) +} + func (r *VtxoRepository) MarkVtxosOnchainSpent( ctx context.Context, spentBy map[domain.Outpoint]string, ) error { diff --git a/internal/infrastructure/db/onchain_spend_repo_test.go b/internal/infrastructure/db/onchain_spend_repo_test.go index aaca97a7e..e3dd536a4 100644 --- a/internal/infrastructure/db/onchain_spend_repo_test.go +++ b/internal/infrastructure/db/onchain_spend_repo_test.go @@ -145,6 +145,41 @@ func TestOnchainSpendRepository(t *testing.T) { require.False(t, containsOutpoint(recorded, unspent.Outpoint)) }) + t.Run("retracts an unroll and leaves an in-Ark spend alone", func(t *testing.T) { + // The unroll mark is set the moment the outpoint appears on + // chain, so an unroll that never confirms leaves it set forever. + retracted := onchainSpendVtxo(randomString(32)) + require.NoError(t, repo.AddVtxos(ctx, []domain.Vtxo{retracted})) + require.NoError(t, repo.UnrollVtxos(ctx, []domain.Outpoint{retracted.Outpoint})) + require.True(t, getOnchainSpendVtxo(t, repo, retracted.Outpoint).Unrolled) + + require.NoError(t, repo.UnmarkVtxosUnrolled( + ctx, []domain.Outpoint{retracted.Outpoint}, + )) + + got := getOnchainSpendVtxo(t, repo, retracted.Outpoint) + require.False(t, got.Unrolled, "a retracted unroll must free the vtxo again") + require.False(t, got.Spent) + + // A vtxo spent inside the Ark and then unrolled is the fraud + // path, whose spent_by the sweeper resolves as a checkpoint tx. + // Clearing its unroll would hide it from that path entirely. + fraud := onchainSpendVtxo(randomString(32)) + require.NoError(t, repo.AddVtxos(ctx, []domain.Vtxo{fraud})) + require.NoError(t, repo.SpendVtxos( + ctx, map[domain.Outpoint]string{fraud.Outpoint: "checkpointtxid"}, "arktxid", + )) + require.NoError(t, repo.UnrollVtxos(ctx, []domain.Outpoint{fraud.Outpoint})) + + require.NoError(t, repo.UnmarkVtxosUnrolled( + ctx, []domain.Outpoint{fraud.Outpoint}, + )) + + got = getOnchainSpendVtxo(t, repo, fraud.Outpoint) + require.True(t, got.Unrolled, "an in-Ark spend must keep its unroll mark") + require.Equal(t, "arktxid", got.ArkTxid) + }) + // A rejoined unrolled vtxo is spent onchain by the commitment tx // itself, and that spend can be noticed before the round is // projected. The settlement is the authoritative record and must diff --git a/internal/infrastructure/db/postgres/sqlc/queries/query.sql.go b/internal/infrastructure/db/postgres/sqlc/queries/query.sql.go index 3b7e3ff3d..49a835799 100644 --- a/internal/infrastructure/db/postgres/sqlc/queries/query.sql.go +++ b/internal/infrastructure/db/postgres/sqlc/queries/query.sql.go @@ -2804,6 +2804,25 @@ func (q *Queries) UpdateVtxoSpent(ctx context.Context, arg UpdateVtxoSpentParams return err } +const updateVtxoUnrollRetracted = `-- name: UpdateVtxoUnrollRetracted :exec +UPDATE vtxo SET unrolled = false, updated_at = (EXTRACT(EPOCH FROM NOW()) * 1000)::BIGINT +WHERE txid = $1 AND vout = $2 AND unrolled = true AND spent = false +` + +type UpdateVtxoUnrollRetractedParams struct { + Txid string + Vout int32 +} + +// Retracts an unroll whose materialising transaction the chain backend no longer +// has any record of. Scoped to a vtxo still believed unspent, so it can never +// clear the flag on one that was spent inside the Ark and then unrolled, which +// is the fraud path the sweeper resolves through spent_by. +func (q *Queries) UpdateVtxoUnrollRetracted(ctx context.Context, arg UpdateVtxoUnrollRetractedParams) error { + _, err := q.db.ExecContext(ctx, updateVtxoUnrollRetracted, arg.Txid, arg.Vout) + return err +} + const updateVtxoUnrolled = `-- name: UpdateVtxoUnrolled :exec UPDATE vtxo SET unrolled = true, updated_at = (EXTRACT(EPOCH FROM NOW()) * 1000)::BIGINT WHERE txid = $1 AND vout = $2 ` diff --git a/internal/infrastructure/db/postgres/sqlc/query.sql b/internal/infrastructure/db/postgres/sqlc/query.sql index 35063be99..83f1f4eff 100644 --- a/internal/infrastructure/db/postgres/sqlc/query.sql +++ b/internal/infrastructure/db/postgres/sqlc/query.sql @@ -103,6 +103,14 @@ UPDATE vtxo SET expires_at = @expires_at WHERE txid = @txid AND vout = @vout; -- name: UpdateVtxoUnrolled :exec UPDATE vtxo SET unrolled = true, updated_at = (EXTRACT(EPOCH FROM NOW()) * 1000)::BIGINT WHERE txid = @txid AND vout = @vout; +-- Retracts an unroll whose materialising transaction the chain backend no longer +-- has any record of. Scoped to a vtxo still believed unspent, so it can never +-- clear the flag on one that was spent inside the Ark and then unrolled, which +-- is the fraud path the sweeper resolves through spent_by. +-- name: UpdateVtxoUnrollRetracted :exec +UPDATE vtxo SET unrolled = false, updated_at = (EXTRACT(EPOCH FROM NOW()) * 1000)::BIGINT +WHERE txid = @txid AND vout = @vout AND unrolled = true AND spent = false; + -- name: UpdateVtxoSettled :exec UPDATE vtxo SET spent = true, spent_by = @spent_by, settled_by = @settled_by, updated_at = (EXTRACT(EPOCH FROM NOW()) * 1000)::BIGINT WHERE txid = @txid AND vout = @vout; diff --git a/internal/infrastructure/db/postgres/vtxo_repo.go b/internal/infrastructure/db/postgres/vtxo_repo.go index 8cda60778..eef4466e9 100644 --- a/internal/infrastructure/db/postgres/vtxo_repo.go +++ b/internal/infrastructure/db/postgres/vtxo_repo.go @@ -309,6 +309,25 @@ func (v *vtxoRepository) GetCheckpointTxsByVtxoPubKeys( return txs, nil } +func (v *vtxoRepository) UnmarkVtxosUnrolled( + ctx context.Context, vtxos []domain.Outpoint, +) error { + txBody := func(querierWithTx *queries.Queries) error { + for _, vtxo := range vtxos { + if err := querierWithTx.UpdateVtxoUnrollRetracted( + ctx, + queries.UpdateVtxoUnrollRetractedParams{Txid: vtxo.Txid, Vout: int32(vtxo.VOut)}, + ); err != nil { + return err + } + } + + return nil + } + + return execTx(ctx, v.db, txBody) +} + func (v *vtxoRepository) UnrollVtxos(ctx context.Context, vtxos []domain.Outpoint) error { txBody := func(querierWithTx *queries.Queries) error { for _, vtxo := range vtxos { diff --git a/internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go b/internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go index 0223c1d4e..b1c3a2fbf 100644 --- a/internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go +++ b/internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go @@ -2993,6 +2993,25 @@ func (q *Queries) UpdateVtxoSpent(ctx context.Context, arg UpdateVtxoSpentParams return err } +const updateVtxoUnrollRetracted = `-- name: UpdateVtxoUnrollRetracted :exec +UPDATE vtxo SET unrolled = false, updated_at = (CAST((strftime('%s','now') || substr(strftime('%f','now'),4,3)) AS INTEGER)) +WHERE txid = ?1 AND vout = ?2 AND unrolled = true AND spent = false +` + +type UpdateVtxoUnrollRetractedParams struct { + Txid string + Vout int64 +} + +// Retracts an unroll whose materialising transaction the chain backend no longer +// has any record of. Scoped to a vtxo still believed unspent, so it can never +// clear the flag on one that was spent inside the Ark and then unrolled, which +// is the fraud path the sweeper resolves through spent_by. +func (q *Queries) UpdateVtxoUnrollRetracted(ctx context.Context, arg UpdateVtxoUnrollRetractedParams) error { + _, err := q.db.ExecContext(ctx, updateVtxoUnrollRetracted, arg.Txid, arg.Vout) + return err +} + const updateVtxoUnrolled = `-- name: UpdateVtxoUnrolled :exec UPDATE vtxo SET unrolled = true, updated_at = (CAST((strftime('%s','now') || substr(strftime('%f','now'),4,3)) AS INTEGER)) WHERE txid = ?1 AND vout = ?2 ` diff --git a/internal/infrastructure/db/sqlite/sqlc/query.sql b/internal/infrastructure/db/sqlite/sqlc/query.sql index f622f66bd..498c6f7d3 100644 --- a/internal/infrastructure/db/sqlite/sqlc/query.sql +++ b/internal/infrastructure/db/sqlite/sqlc/query.sql @@ -103,6 +103,14 @@ UPDATE vtxo SET expires_at = @expires_at WHERE txid = @txid AND vout = @vout; -- name: UpdateVtxoUnrolled :exec UPDATE vtxo SET unrolled = true, updated_at = (CAST((strftime('%s','now') || substr(strftime('%f','now'),4,3)) AS INTEGER)) WHERE txid = @txid AND vout = @vout; +-- Retracts an unroll whose materialising transaction the chain backend no longer +-- has any record of. Scoped to a vtxo still believed unspent, so it can never +-- clear the flag on one that was spent inside the Ark and then unrolled, which +-- is the fraud path the sweeper resolves through spent_by. +-- name: UpdateVtxoUnrollRetracted :exec +UPDATE vtxo SET unrolled = false, updated_at = (CAST((strftime('%s','now') || substr(strftime('%f','now'),4,3)) AS INTEGER)) +WHERE txid = @txid AND vout = @vout AND unrolled = true AND spent = false; + -- name: UpdateVtxoSettled :exec UPDATE vtxo SET spent = true, spent_by = @spent_by, settled_by = @settled_by, updated_at = (CAST((strftime('%s','now') || substr(strftime('%f','now'),4,3)) AS INTEGER)) WHERE txid = @txid AND vout = @vout; diff --git a/internal/infrastructure/db/sqlite/vtxo_repo.go b/internal/infrastructure/db/sqlite/vtxo_repo.go index e53f1c425..5acf9b4c6 100644 --- a/internal/infrastructure/db/sqlite/vtxo_repo.go +++ b/internal/infrastructure/db/sqlite/vtxo_repo.go @@ -365,6 +365,25 @@ func (v *vtxoRepository) GetCheckpointTxsByVtxoPubKeys( return txs, nil } +func (v *vtxoRepository) UnmarkVtxosUnrolled( + ctx context.Context, vtxos []domain.Outpoint, +) error { + txBody := func(querierWithTx *queries.Queries) error { + for _, vtxo := range vtxos { + if err := querierWithTx.UpdateVtxoUnrollRetracted( + ctx, + queries.UpdateVtxoUnrollRetractedParams{Txid: vtxo.Txid, Vout: int64(vtxo.VOut)}, + ); err != nil { + return err + } + } + + return nil + } + + return execTx(ctx, v.db.Write(), txBody) +} + func (v *vtxoRepository) UnrollVtxos(ctx context.Context, vtxos []domain.Outpoint) error { txBody := func(querierWithTx *queries.Queries) error { for _, vtxo := range vtxos { diff --git a/internal/infrastructure/tx-builder/covenantless/mocks_test.go b/internal/infrastructure/tx-builder/covenantless/mocks_test.go index e795762dd..f2b649b93 100644 --- a/internal/infrastructure/tx-builder/covenantless/mocks_test.go +++ b/internal/infrastructure/tx-builder/covenantless/mocks_test.go @@ -198,6 +198,15 @@ func (m *mockedWallet) GetDustAmount(ctx context.Context) (uint64, error) { return res, args.Error(1) } +func (m *mockedWallet) IsTransactionKnown(ctx context.Context, txid string) (bool, error) { + args := m.Called(ctx, txid) + var res bool + if a := args.Get(0); a != nil { + res = a.(bool) + } + return res, args.Error(1) +} + func (m *mockedWallet) IsTransactionConfirmed( ctx context.Context, txid string, ) (bool, *ports.BlockTimestamp, error) { diff --git a/internal/infrastructure/wallet/wallet_client.go b/internal/infrastructure/wallet/wallet_client.go index bb10a2c19..9d85bfac8 100644 --- a/internal/infrastructure/wallet/wallet_client.go +++ b/internal/infrastructure/wallet/wallet_client.go @@ -442,6 +442,21 @@ func castSpends(spends []*arkwalletv1.SpendInfo) []ports.Spend { return out } +// IsTransactionKnown reads the not_found flag the wallet sets when its backend +// has no record of the transaction. An older wallet never sets it, so this +// reports the transaction as known and no caller can act on a missing one. +func (w *walletDaemonClient) IsTransactionKnown( + ctx context.Context, txid string, +) (bool, error) { + resp, err := w.client.IsTransactionConfirmed( + ctx, &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + if err != nil { + return false, err + } + return !resp.GetNotFound(), nil +} + func (w *walletDaemonClient) IsTransactionConfirmed( ctx context.Context, txid string, ) (bool, *ports.BlockTimestamp, error) { diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go index 113e553b5..643f4742c 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go @@ -350,6 +350,7 @@ func (h *walletHandler) IsTransactionConfirmed( Confirmed: false, Blocknumber: 0, Blocktime: 0, + NotFound: true, }, nil } return nil, err From 306ce1476129bbc6ad853d317a601e1dab4698ed Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:49:58 -0400 Subject: [PATCH 2/7] test: cover the three seams the retraction actually depends on Self-review found the feature could be silently disconnected and its safety properties were unpinned. Removing the retraction's one call site from the reconcile pass left every test green, because each case drove the function directly. That call is now pinned by a case that runs the reconcile pass. The two wallet-side seams had no coverage at all. The handler is what sets the not-found flag, and setting it on the wrong branch would report a live transaction as gone; the client is what reads it, and the reading is what makes an older wallet degrade to never retracting rather than to retracting blindly. Both are covered, and both fail when the behaviour they describe is inverted. Also drops a comment that justified production code by pointing at the tests. --- .../core/application/unroll_retraction.go | 4 +- .../application/unroll_retraction_test.go | 22 +++++ .../wallet/wallet_client_test.go | 70 ++++++++++++++ .../grpc/handlers/wallet_handler_test.go | 96 +++++++++++++++++++ 4 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go diff --git a/internal/core/application/unroll_retraction.go b/internal/core/application/unroll_retraction.go index 4dbf84777..3f37cac06 100644 --- a/internal/core/application/unroll_retraction.go +++ b/internal/core/application/unroll_retraction.go @@ -104,8 +104,8 @@ func (s *service) recordUnrollObservation(outpoint domain.Outpoint, known bool) delete(s.unrollObservations, outpoint) return 0 } - // Lazily built so a service assembled without NewService, as the tests do, - // still counts rather than panicking on a nil map. + // Built on demand so a zero-value service counts correctly rather than + // panicking on a nil map. if s.unrollObservations == nil { s.unrollObservations = make(map[domain.Outpoint]int) } diff --git a/internal/core/application/unroll_retraction_test.go b/internal/core/application/unroll_retraction_test.go index 594b964ae..5ebeea5b4 100644 --- a/internal/core/application/unroll_retraction_test.go +++ b/internal/core/application/unroll_retraction_test.go @@ -109,6 +109,28 @@ func TestRetractStaleUnrolls(t *testing.T) { vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) + // Without this the retraction is unreachable in production: every unit above + // calls retractStaleUnrolls directly, so removing its one call site from the + // reconcile pass would leave them all green. + t.Run("the reconcile pass drives the retraction", func(t *testing.T) { + vtxos := &mockedVtxoRepo{} + vtxos.On("GetUnrolledUnspentVtxos", mock.Anything).Return(candidates, nil) + vtxos.On("GetOnchainSpentVtxos", mock.Anything).Return([]domain.Vtxo{}, nil) + vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) + rm := &mockedRepoManager{} + rm.On("Vtxos").Return(vtxos) + svc := &service{repoManager: rm, scanner: &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + known: map[string]bool{}, + }} + + for range unrollRetractionObservations { + svc.reconcileOnchainSpendsOnce(ctx, nil) + } + + vtxos.AssertCalled(t, "UnmarkVtxosUnrolled", mock.Anything, []domain.Outpoint{out}) + }) + t.Run("no candidates is a no-op", func(t *testing.T) { scanner := &mockedScanner{} svc, vtxos := unrollService(t, scanner) diff --git a/internal/infrastructure/wallet/wallet_client_test.go b/internal/infrastructure/wallet/wallet_client_test.go index 90d6dabde..b91a151a4 100644 --- a/internal/infrastructure/wallet/wallet_client_test.go +++ b/internal/infrastructure/wallet/wallet_client_test.go @@ -294,6 +294,76 @@ func TestNotificationStreamIsShared(t *testing.T) { } } +// TestIsTransactionKnown pins the reading that keeps an unroll retraction safe. +// The wallet answers "not confirmed" both for a transaction waiting in the +// mempool and for one its backend has never seen, so the caller depends on the +// not_found flag to tell those apart, and on an older wallet that never sets it +// reading as known rather than as missing. +func TestIsTransactionKnown(t *testing.T) { + const txid = "4ba63c204f39841e3a7c98e458586307cf6d33bbed9a9a520c827ab043f32701" + + t.Run("a transaction the backend has no record of is not known", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{NotFound: true}, + }} + + known, err := w.IsTransactionKnown(t.Context(), txid) + + require.NoError(t, err) + require.False(t, known) + }) + + t.Run("an unconfirmed transaction the backend has is known", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{Confirmed: false}, + }} + + known, err := w.IsTransactionKnown(t.Context(), txid) + + require.NoError(t, err) + require.True(t, known, "unconfirmed is not the same as missing") + }) + + // The compatibility case. A wallet predating the flag leaves it unset, and + // the caller must read that as known, so no retraction can ever fire against + // an old wallet. + t.Run("an older wallet that never sets the flag reads as known", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{}, + }} + + known, err := w.IsTransactionKnown(t.Context(), txid) + + require.NoError(t, err) + require.True(t, known) + }) + + t.Run("a transport failure is reported, not read as missing", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{err: errors.New("wallet down")}} + + known, err := w.IsTransactionKnown(t.Context(), txid) + + require.Error(t, err) + require.False(t, known) + }) +} + +// confirmFakeClient answers IsTransactionConfirmed with a fixed response. +type confirmFakeClient struct { + arkwalletv1.WalletServiceClient + resp *arkwalletv1.IsTransactionConfirmedResponse + err error +} + +func (f *confirmFakeClient) IsTransactionConfirmed( + _ context.Context, _ *arkwalletv1.IsTransactionConfirmedRequest, _ ...grpc.CallOption, +) (*arkwalletv1.IsTransactionConfirmedResponse, error) { + if f.err != nil { + return nil, f.err + } + return f.resp, nil +} + // fakeNotificationStream replays a fixed set of responses, then blocks until // its context is cancelled so the reader goroutine behaves like a live stream // rather than terminating immediately. diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go new file mode 100644 index 000000000..e97f03584 --- /dev/null +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go @@ -0,0 +1,96 @@ +package handlers + +import ( + "context" + "errors" + "testing" + + arkwalletv1 "github.com/arkade-os/arkd/api-spec/protobuf/gen/arkwallet/v1" + "github.com/arkade-os/arkd/pkg/arkd-wallet/core/application" + "github.com/stretchr/testify/require" +) + +// TestIsTransactionConfirmedNotFound pins the flag that separates a transaction +// the backend has never seen from one it has and sees unconfirmed. Both answer +// confirmed = false, so without the flag a caller cannot tell an evicted or +// replaced transaction from one still waiting, and anything acting on that +// difference would either never act or act wrongly. +func TestIsTransactionConfirmedNotFound(t *testing.T) { + const txid = "4ba63c204f39841e3a7c98e458586307cf6d33bbed9a9a520c827ab043f32701" + + t.Run("a transaction the backend does not have is flagged not found", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{err: application.ErrTransactionNotFound}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err, "a missing transaction is an ordinary answer, not a failure") + require.False(t, resp.GetConfirmed()) + require.True(t, resp.GetNotFound()) + }) + + // The case the flag exists to distinguish: known to the backend, just not + // mined yet. Flagging this one would let a caller treat a live transaction + // as gone. + t.Run("an unconfirmed transaction is not flagged not found", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.False(t, resp.GetConfirmed()) + require.False(t, resp.GetNotFound()) + }) + + t.Run("a confirmed transaction is not flagged not found", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{ + confirmed: true, blockHeight: 964276, blockTime: 1700000000, + }} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.True(t, resp.GetConfirmed()) + require.False(t, resp.GetNotFound()) + require.EqualValues(t, 964276, resp.GetBlocknumber()) + }) + + // Any other failure stays a failure. Reporting it as not found would tell + // the caller the transaction is gone when the backend simply could not say. + t.Run("another failure is returned as an error", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{err: errors.New("backend down")}} + + _, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.Error(t, err) + }) +} + +// --- fixtures --- + +// confirmScanner answers IsTransactionConfirmed with a fixed result. The +// embedded interface is nil, so any other method panics rather than silently +// returning a zero value. +type confirmScanner struct { + application.BlockchainScanner + confirmed bool + blockHeight int64 + blockTime int64 + err error +} + +func (s *confirmScanner) IsTransactionConfirmed( + _ context.Context, _ string, +) (bool, int64, int64, error) { + if s.err != nil { + return false, 0, 0, s.err + } + return s.confirmed, s.blockHeight, s.blockTime, nil +} From eb7f00d3c2e19163442bfad4be64c56530516867 Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:11:15 -0400 Subject: [PATCH 3/7] vtxo: key the retraction on the replacement signal, not on absence Testing against a live NBXplorer showed the retraction as committed could never fire. The design assumed a transaction that will not confirm eventually becomes unknown to the backend. It does not. A transaction replaced by a higher-fee conflict, dropped by the node, keeps answering HTTP 200 with zero confirmations indefinitely, so asking whether the backend still knows it always answered yes and no unroll was ever retracted. What the backend does say is replacedBy, naming the transaction that superseded this one, populated once the replacement confirms. That is positive evidence of the kind this reconciler already insists on, and stronger than absence: it says what happened rather than that something is missing. The wallet already parsed the field and dropped it on the floor. So the signal is now "the backend has positively dropped this transaction", either replaced or unknown, rather than "the backend has forgotten it". Both halves keep the property that an older wallet, setting neither, reads as still live, so no retraction can fire against one. Verified end to end against the running regtest stack: a transaction replaced by a confirmed conflict reports its replacement through the real client, and a confirmed transaction reports none. Still not covered, and now known rather than assumed: a transaction that simply expires from the mempool without being replaced is never reported as either replaced or unknown, so it produces no signal at all. --- .../arkwallet/v1/bitcoin_wallet.openapi.json | 4 ++ .../arkwallet/v1/bitcoin_wallet.proto | 8 +++ .../gen/arkwallet/v1/bitcoin_wallet.pb.go | 23 +++++++- internal/core/application/mocks_test.go | 38 ++++++------ internal/core/application/sweeper_test.go | 4 +- .../core/application/unroll_retraction.go | 6 +- .../application/unroll_retraction_test.go | 31 +++++----- internal/core/ports/scanner.go | 19 +++--- .../tx-builder/covenantless/mocks_test.go | 2 +- .../infrastructure/wallet/wallet_client.go | 11 ++-- .../wallet/wallet_client_test.go | 58 ++++++++++++------- .../core/application/scanner/service.go | 12 ++++ pkg/arkd-wallet/core/application/types.go | 3 + .../core/infrastructure/nbxplorer/service.go | 1 + pkg/arkd-wallet/core/ports/nbxplorer.go | 5 ++ .../interface/grpc/handlers/wallet_handler.go | 16 +++++ .../grpc/handlers/wallet_handler_test.go | 35 +++++++++++ 17 files changed, 199 insertions(+), 77 deletions(-) diff --git a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json index a23b25afc..f761cfbd1 100644 --- a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json +++ b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json @@ -1716,6 +1716,10 @@ "notFound": { "type": "boolean", "description": "not_found separates a transaction the backend has no record of from one it\nknows and sees unconfirmed. Both answer confirmed = false, which is why the\ndistinction needs its own field.\n\nAdded after the other fields, and phrased so false is the safe reading: an\nolder wallet never sets it, and a caller then sees \"not known to be\nmissing\" rather than \"missing\"." + }, + "replacedBy": { + "type": "string", + "description": "replaced_by names the transaction that superseded this one. It is the\nbackend's only positive statement that a transaction will not confirm: a\nreplaced transaction keeps answering confirmed = false and not_found =\nfalse forever, so neither of those can stand in for it.\n\nEmpty when the transaction was not replaced, which is also what an older\nwallet returns, so the safe reading is the zero value here too." } } }, diff --git a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto index 94435165c..d5c709f9a 100644 --- a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto +++ b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto @@ -231,6 +231,14 @@ message IsTransactionConfirmedResponse { // older wallet never sets it, and a caller then sees "not known to be // missing" rather than "missing". bool not_found = 4; + // replaced_by names the transaction that superseded this one. It is the + // backend's only positive statement that a transaction will not confirm: a + // replaced transaction keeps answering confirmed = false and not_found = + // false forever, so neither of those can stand in for it. + // + // Empty when the transaction was not replaced, which is also what an older + // wallet returns, so the safe reading is the zero value here too. + string replaced_by = 5; } message GetOutpointStatusRequest { diff --git a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go index d835b2cfd..c223389b7 100644 --- a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go +++ b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go @@ -158,7 +158,15 @@ type IsTransactionConfirmedResponse struct { // Added after the other fields, and phrased so false is the safe reading: an // older wallet never sets it, and a caller then sees "not known to be // missing" rather than "missing". - NotFound bool `protobuf:"varint,4,opt,name=not_found,json=notFound,proto3" json:"not_found,omitempty"` + NotFound bool `protobuf:"varint,4,opt,name=not_found,json=notFound,proto3" json:"not_found,omitempty"` + // replaced_by names the transaction that superseded this one. It is the + // backend's only positive statement that a transaction will not confirm: a + // replaced transaction keeps answering confirmed = false and not_found = + // false forever, so neither of those can stand in for it. + // + // Empty when the transaction was not replaced, which is also what an older + // wallet returns, so the safe reading is the zero value here too. + ReplacedBy string `protobuf:"bytes,5,opt,name=replaced_by,json=replacedBy,proto3" json:"replaced_by,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -221,6 +229,13 @@ func (x *IsTransactionConfirmedResponse) GetNotFound() bool { return false } +func (x *IsTransactionConfirmedResponse) GetReplacedBy() string { + if x != nil { + return x.ReplacedBy + } + return "" +} + type GetOutpointStatusRequest struct { state protoimpl.MessageState `protogen:"open.v1"` Txid string `protobuf:"bytes,1,opt,name=txid,proto3" json:"txid,omitempty"` @@ -3794,12 +3809,14 @@ const file_arkwallet_v1_bitcoin_wallet_proto_rawDesc = "" + "\x16GetReadyUpdateResponse\x12\x14\n" + "\x05ready\x18\x01 \x01(\bR\x05ready\"3\n" + "\x1dIsTransactionConfirmedRequest\x12\x12\n" + - "\x04txid\x18\x01 \x01(\tR\x04txid\"\x9b\x01\n" + + "\x04txid\x18\x01 \x01(\tR\x04txid\"\xbc\x01\n" + "\x1eIsTransactionConfirmedResponse\x12\x1c\n" + "\tconfirmed\x18\x01 \x01(\bR\tconfirmed\x12 \n" + "\vblocknumber\x18\x02 \x01(\x03R\vblocknumber\x12\x1c\n" + "\tblocktime\x18\x03 \x01(\x03R\tblocktime\x12\x1b\n" + - "\tnot_found\x18\x04 \x01(\bR\bnotFound\"B\n" + + "\tnot_found\x18\x04 \x01(\bR\bnotFound\x12\x1f\n" + + "\vreplaced_by\x18\x05 \x01(\tR\n" + + "replacedBy\"B\n" + "\x18GetOutpointStatusRequest\x12\x12\n" + "\x04txid\x18\x01 \x01(\tR\x04txid\x12\x12\n" + "\x04vout\x18\x02 \x01(\rR\x04vout\"1\n" + diff --git a/internal/core/application/mocks_test.go b/internal/core/application/mocks_test.go index 076ad4b5b..22ba191c1 100644 --- a/internal/core/application/mocks_test.go +++ b/internal/core/application/mocks_test.go @@ -213,17 +213,17 @@ type mockedScanner struct { mu sync.Mutex // onchain-spend fixtures, read by the reconcile tests - // known maps a txid to whether the backend still has a record of it; a txid - // absent from the map reads as unknown, which is what a retraction needs. - known map[string]bool - knownErr error - knownCalls []string - spendCh chan []ports.Spend - spends []ports.Spend - spendsErr error - spendsFrom []*time.Time - unspent map[domain.Outpoint]struct{} - unspentErr error + // dropped maps a txid to whether the backend says it will not confirm; a + // txid absent from the map reads as still live, the safe default. + dropped map[string]bool + droppedErr error + droppedCalls []string + spendCh chan []ports.Spend + spends []ports.Spend + spendsErr error + spendsFrom []*time.Time + unspent map[domain.Outpoint]struct{} + unspentErr error } func (m *mockedScanner) WatchScripts( @@ -276,21 +276,21 @@ func (m *mockedScanner) RescanUtxos(_ context.Context, _ []wire.OutPoint) error return nil } -func (m *mockedScanner) IsTransactionKnown(_ context.Context, txid string) (bool, error) { +func (m *mockedScanner) IsTransactionDropped(_ context.Context, txid string) (bool, error) { m.mu.Lock() defer m.mu.Unlock() - m.knownCalls = append(m.knownCalls, txid) - if m.knownErr != nil { - return false, m.knownErr + m.droppedCalls = append(m.droppedCalls, txid) + if m.droppedErr != nil { + return false, m.droppedErr } - return m.known[txid], nil + return m.dropped[txid], nil } -// KnownCalls returns every txid IsTransactionKnown was asked about. -func (m *mockedScanner) KnownCalls() []string { +// DroppedCalls returns every txid IsTransactionDropped was asked about. +func (m *mockedScanner) DroppedCalls() []string { m.mu.Lock() defer m.mu.Unlock() - return append([]string(nil), m.knownCalls...) + return append([]string(nil), m.droppedCalls...) } func (m *mockedScanner) GetSpendNotificationChannel( diff --git a/internal/core/application/sweeper_test.go b/internal/core/application/sweeper_test.go index 84b62d01b..dbb252a88 100644 --- a/internal/core/application/sweeper_test.go +++ b/internal/core/application/sweeper_test.go @@ -370,8 +370,8 @@ func (m *mockWalletService) IsTransactionConfirmed( ) (bool, *ports.BlockTimestamp, error) { return false, nil, nil } -func (m *mockWalletService) IsTransactionKnown(ctx context.Context, txid string) (bool, error) { - return true, nil +func (m *mockWalletService) IsTransactionDropped(ctx context.Context, txid string) (bool, error) { + return false, nil } func (m *mockWalletService) RescanUtxos(ctx context.Context, outpoints []wire.OutPoint) error { return nil diff --git a/internal/core/application/unroll_retraction.go b/internal/core/application/unroll_retraction.go index 3f37cac06..f280ee0e5 100644 --- a/internal/core/application/unroll_retraction.go +++ b/internal/core/application/unroll_retraction.go @@ -8,7 +8,7 @@ import ( ) // unrollRetractionObservations is how many consecutive reconcile passes must -// find a vtxo's materialising tx unknown to the chain backend before its unroll +// find a vtxo's materialising tx dropped by the chain backend before its unroll // is retracted. One pass is not enough: a transaction that has been broadcast // but has not reached our node yet reads exactly like one that is gone, and // retracting a live unroll would hand its owner back a vtxo whose output exists @@ -57,7 +57,7 @@ func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.V continue } - known, err := s.scanner.IsTransactionKnown(ctx, vtxo.Txid) + dropped, err := s.scanner.IsTransactionDropped(ctx, vtxo.Txid) if err != nil { // Not evidence of anything. Leave the count untouched so a flapping // backend cannot accumulate its way to a retraction. @@ -68,7 +68,7 @@ func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.V continue } - if s.recordUnrollObservation(vtxo.Outpoint, known) >= unrollRetractionObservations { + if s.recordUnrollObservation(vtxo.Outpoint, !dropped) >= unrollRetractionObservations { stale = append(stale, vtxo.Outpoint) } } diff --git a/internal/core/application/unroll_retraction_test.go b/internal/core/application/unroll_retraction_test.go index 5ebeea5b4..faef85076 100644 --- a/internal/core/application/unroll_retraction_test.go +++ b/internal/core/application/unroll_retraction_test.go @@ -15,10 +15,10 @@ func TestRetractStaleUnrolls(t *testing.T) { out := outpoint(unrolledVtxoTxid, 0) candidates := []domain.Vtxo{{Outpoint: out, Unrolled: true}} - t.Run("retracts once the tx has been unknown for enough passes", func(t *testing.T) { + t.Run("retracts once the backend says the tx is gone for enough passes", func(t *testing.T) { svc, vtxos := unrollService(t, &mockedScanner{ unspent: map[domain.Outpoint]struct{}{}, - known: map[string]bool{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, }) vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) @@ -36,7 +36,7 @@ func TestRetractStaleUnrolls(t *testing.T) { t.Run("never retracts while the outpoint is still unspent", func(t *testing.T) { svc, vtxos := unrollService(t, &mockedScanner{ unspent: map[domain.Outpoint]struct{}{out: {}}, - known: map[string]bool{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, }) for range unrollRetractionObservations * 2 { @@ -46,21 +46,22 @@ func TestRetractStaleUnrolls(t *testing.T) { vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) - // A wallet predating the not-found field reports every tx as known, so it - // degrades to never retracting rather than to retracting blindly. - t.Run("a tx the backend still knows resets the count", func(t *testing.T) { + // An older wallet sets neither signal, so every transaction reads as still + // live and the reconciler degrades to never retracting rather than to + // retracting blindly. + t.Run("a tx the backend has not dropped resets the count", func(t *testing.T) { scanner := &mockedScanner{ unspent: map[domain.Outpoint]struct{}{}, - known: map[string]bool{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, } svc, vtxos := unrollService(t, scanner) for range unrollRetractionObservations - 1 { svc.retractStaleUnrolls(ctx, candidates) } - scanner.known[unrolledVtxoTxid] = true + scanner.dropped[unrolledVtxoTxid] = false svc.retractStaleUnrolls(ctx, candidates) - scanner.known[unrolledVtxoTxid] = false + scanner.dropped[unrolledVtxoTxid] = true svc.retractStaleUnrolls(ctx, candidates) vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) @@ -70,8 +71,8 @@ func TestRetractStaleUnrolls(t *testing.T) { // retraction the way a real "gone" answer does. t.Run("a lookup failure does not advance the count", func(t *testing.T) { svc, vtxos := unrollService(t, &mockedScanner{ - unspent: map[domain.Outpoint]struct{}{}, - knownErr: errors.New("wallet down"), + unspent: map[domain.Outpoint]struct{}{}, + droppedErr: errors.New("wallet down"), }) for range unrollRetractionObservations * 2 { @@ -87,7 +88,7 @@ func TestRetractStaleUnrolls(t *testing.T) { svc.retractStaleUnrolls(ctx, candidates) - require.Empty(t, scanner.KnownCalls(), "no tx lookup without the stronger signal") + require.Empty(t, scanner.DroppedCalls(), "no tx lookup without the stronger signal") vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) @@ -96,7 +97,7 @@ func TestRetractStaleUnrolls(t *testing.T) { t.Run("counts do not survive leaving the candidate set", func(t *testing.T) { svc, vtxos := unrollService(t, &mockedScanner{ unspent: map[domain.Outpoint]struct{}{}, - known: map[string]bool{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, }) vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) @@ -121,7 +122,7 @@ func TestRetractStaleUnrolls(t *testing.T) { rm.On("Vtxos").Return(vtxos) svc := &service{repoManager: rm, scanner: &mockedScanner{ unspent: map[domain.Outpoint]struct{}{}, - known: map[string]bool{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, }} for range unrollRetractionObservations { @@ -137,7 +138,7 @@ func TestRetractStaleUnrolls(t *testing.T) { svc.retractStaleUnrolls(ctx, nil) - require.Empty(t, scanner.KnownCalls()) + require.Empty(t, scanner.DroppedCalls()) vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) } diff --git a/internal/core/ports/scanner.go b/internal/core/ports/scanner.go index 20c7594d2..6b3707c36 100644 --- a/internal/core/ports/scanner.go +++ b/internal/core/ports/scanner.go @@ -31,14 +31,17 @@ type BlockchainScanner interface { IsTransactionConfirmed( ctx context.Context, txid string, ) (isConfirmed bool, blockTimestamp *BlockTimestamp, err error) - // IsTransactionKnown reports whether the chain backend has any record of the - // transaction, confirmed or still in the mempool. It is deliberately not - // IsTransactionConfirmed: that answers false both for a transaction waiting - // in the mempool and for one the backend has never seen, and the difference - // is the whole signal here. A false answer is positive evidence the - // transaction is gone, which is what separates a replaced or evicted one - // from one merely waiting. - IsTransactionKnown(ctx context.Context, txid string) (bool, error) + // IsTransactionDropped reports that the chain backend has positively + // determined the transaction will not confirm: either it was superseded by + // another transaction, or the backend has no record of it at all. + // + // It is deliberately not the negation of IsTransactionConfirmed, which + // answers false for a transaction merely waiting in the mempool. Verified + // against a live NBXplorer: a replaced transaction keeps answering zero + // confirmations indefinitely and is never reported as missing, so being + // superseded is the only positive statement the backend makes, and it makes + // it once the replacement confirms. + IsTransactionDropped(ctx context.Context, txid string) (bool, error) // GetSpends returns every watched output spent by a confirmed or unconfirmed // transaction, windowed from the given instant when one is supplied. GetSpends(ctx context.Context, from *time.Time) ([]Spend, error) diff --git a/internal/infrastructure/tx-builder/covenantless/mocks_test.go b/internal/infrastructure/tx-builder/covenantless/mocks_test.go index f2b649b93..216e42cd3 100644 --- a/internal/infrastructure/tx-builder/covenantless/mocks_test.go +++ b/internal/infrastructure/tx-builder/covenantless/mocks_test.go @@ -198,7 +198,7 @@ func (m *mockedWallet) GetDustAmount(ctx context.Context) (uint64, error) { return res, args.Error(1) } -func (m *mockedWallet) IsTransactionKnown(ctx context.Context, txid string) (bool, error) { +func (m *mockedWallet) IsTransactionDropped(ctx context.Context, txid string) (bool, error) { args := m.Called(ctx, txid) var res bool if a := args.Get(0); a != nil { diff --git a/internal/infrastructure/wallet/wallet_client.go b/internal/infrastructure/wallet/wallet_client.go index 9d85bfac8..a8e950c36 100644 --- a/internal/infrastructure/wallet/wallet_client.go +++ b/internal/infrastructure/wallet/wallet_client.go @@ -442,10 +442,11 @@ func castSpends(spends []*arkwalletv1.SpendInfo) []ports.Spend { return out } -// IsTransactionKnown reads the not_found flag the wallet sets when its backend -// has no record of the transaction. An older wallet never sets it, so this -// reports the transaction as known and no caller can act on a missing one. -func (w *walletDaemonClient) IsTransactionKnown( +// IsTransactionDropped reads the two ways the wallet says a transaction will +// not confirm: replaced by another one, or unknown to its backend. An older +// wallet sets neither, so every transaction reads as still live and no caller +// can act on one that is gone. +func (w *walletDaemonClient) IsTransactionDropped( ctx context.Context, txid string, ) (bool, error) { resp, err := w.client.IsTransactionConfirmed( @@ -454,7 +455,7 @@ func (w *walletDaemonClient) IsTransactionKnown( if err != nil { return false, err } - return !resp.GetNotFound(), nil + return resp.GetNotFound() || resp.GetReplacedBy() != "", nil } func (w *walletDaemonClient) IsTransactionConfirmed( diff --git a/internal/infrastructure/wallet/wallet_client_test.go b/internal/infrastructure/wallet/wallet_client_test.go index b91a151a4..23958c7ff 100644 --- a/internal/infrastructure/wallet/wallet_client_test.go +++ b/internal/infrastructure/wallet/wallet_client_test.go @@ -294,57 +294,73 @@ func TestNotificationStreamIsShared(t *testing.T) { } } -// TestIsTransactionKnown pins the reading that keeps an unroll retraction safe. -// The wallet answers "not confirmed" both for a transaction waiting in the -// mempool and for one its backend has never seen, so the caller depends on the -// not_found flag to tell those apart, and on an older wallet that never sets it -// reading as known rather than as missing. -func TestIsTransactionKnown(t *testing.T) { +// TestIsTransactionDropped pins the reading that keeps an unroll retraction +// safe. The wallet answers "not confirmed" for a transaction waiting in the +// mempool, for one that was replaced, and for one its backend has never seen, +// so the caller depends on the two explicit signals to tell them apart, and on +// an older wallet that sets neither reading as still live. +func TestIsTransactionDropped(t *testing.T) { const txid = "4ba63c204f39841e3a7c98e458586307cf6d33bbed9a9a520c827ab043f32701" - t.Run("a transaction the backend has no record of is not known", func(t *testing.T) { + // The case that actually fires in production. A replaced transaction is + // still known to the backend and still answers "not confirmed", so only the + // replacement signal distinguishes it from one merely waiting. + t.Run("a replaced transaction is dropped", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{ + ReplacedBy: "4dc2f8e63b9dc3825f69c8295a48a9b87ba4c663f42ea7b49fa335746d626246", + }, + }} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.NoError(t, err) + require.True(t, dropped) + }) + + t.Run("a transaction the backend has no record of is dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{NotFound: true}, }} - known, err := w.IsTransactionKnown(t.Context(), txid) + dropped, err := w.IsTransactionDropped(t.Context(), txid) require.NoError(t, err) - require.False(t, known) + require.True(t, dropped) }) - t.Run("an unconfirmed transaction the backend has is known", func(t *testing.T) { + t.Run("an unconfirmed transaction is not dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{Confirmed: false}, }} - known, err := w.IsTransactionKnown(t.Context(), txid) + dropped, err := w.IsTransactionDropped(t.Context(), txid) require.NoError(t, err) - require.True(t, known, "unconfirmed is not the same as missing") + require.False(t, dropped, "waiting in the mempool is not the same as gone") }) - // The compatibility case. A wallet predating the flag leaves it unset, and - // the caller must read that as known, so no retraction can ever fire against - // an old wallet. - t.Run("an older wallet that never sets the flag reads as known", func(t *testing.T) { + // The compatibility case. A wallet predating both signals sets neither, and + // the caller must read that as still live, so no retraction can ever fire + // against an old wallet. + t.Run("an older wallet that sets neither signal reads as live", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{}, }} - known, err := w.IsTransactionKnown(t.Context(), txid) + dropped, err := w.IsTransactionDropped(t.Context(), txid) require.NoError(t, err) - require.True(t, known) + require.False(t, dropped) }) - t.Run("a transport failure is reported, not read as missing", func(t *testing.T) { + t.Run("a transport failure is reported, not read as dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{err: errors.New("wallet down")}} - known, err := w.IsTransactionKnown(t.Context(), txid) + dropped, err := w.IsTransactionDropped(t.Context(), txid) require.Error(t, err) - require.False(t, known) + require.False(t, dropped) }) } diff --git a/pkg/arkd-wallet/core/application/scanner/service.go b/pkg/arkd-wallet/core/application/scanner/service.go index c7b35d945..5ade29a07 100644 --- a/pkg/arkd-wallet/core/application/scanner/service.go +++ b/pkg/arkd-wallet/core/application/scanner/service.go @@ -276,6 +276,18 @@ func (s *scanner) IsTransactionConfirmed(ctx context.Context, txid string) (isCo return details.Confirmations > 0, int64(details.Height), details.Timestamp, nil } +func (s *scanner) TransactionReplacedBy(ctx context.Context, txid string) (string, error) { + details, err := s.nbxplorer.GetTransaction(ctx, txid) + if err != nil { + return "", err + } + if details == nil { + return "", nil + } + + return details.ReplacedBy, nil +} + func (s *scanner) GetOutpointStatus(ctx context.Context, outpoint wire.OutPoint) (spent bool, err error) { spent, err = s.nbxplorer.IsSpent(ctx, outpoint) if err != nil { diff --git a/pkg/arkd-wallet/core/application/types.go b/pkg/arkd-wallet/core/application/types.go index a9556f8c1..caa799505 100644 --- a/pkg/arkd-wallet/core/application/types.go +++ b/pkg/arkd-wallet/core/application/types.go @@ -65,6 +65,9 @@ type BlockchainScanner interface { IsTransactionConfirmed( ctx context.Context, txid string, ) (isConfirmed bool, blockHeight, blockTime int64, err error) + // TransactionReplacedBy names the transaction the backend has recorded as + // having superseded this one, or empty when it has not been replaced. + TransactionReplacedBy(ctx context.Context, txid string) (string, error) GetOutpointStatus(ctx context.Context, outpoint wire.OutPoint) (spent bool, err error) // GetSpends returns every watched output spent by a confirmed or unconfirmed // transaction, windowed from the given instant when one is supplied. diff --git a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go index 3fc6044c3..5bcf2b8ba 100644 --- a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go +++ b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go @@ -195,6 +195,7 @@ func (n *nbxplorer) GetTransaction(ctx context.Context, txid string) (*ports.Tra Height: resp.Height, Timestamp: resp.Timestamp, Confirmations: resp.Confirmations, + ReplacedBy: resp.ReplacedBy, }, nil } diff --git a/pkg/arkd-wallet/core/ports/nbxplorer.go b/pkg/arkd-wallet/core/ports/nbxplorer.go index af88e3eb3..0a1819643 100644 --- a/pkg/arkd-wallet/core/ports/nbxplorer.go +++ b/pkg/arkd-wallet/core/ports/nbxplorer.go @@ -21,6 +21,11 @@ type TransactionDetails struct { Height uint32 Timestamp int64 Confirmations uint32 + // ReplacedBy names the transaction that superseded this one, and is the + // backend's only positive statement that a transaction will not confirm. It + // is populated once the replacement itself confirms; a transaction merely + // dropped from the mempool keeps reporting zero confirmations forever. + ReplacedBy string } type Utxo struct { diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go index 643f4742c..b6f3ce7d2 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go @@ -355,10 +355,26 @@ func (h *walletHandler) IsTransactionConfirmed( } return nil, err } + // Only an unconfirmed transaction can have been replaced, so the second + // lookup is skipped for the common case. A failure to answer is not + // reported as "not replaced": it is left empty, which reads as no signal. + var replacedBy string + if !confirmed { + replacement, err := h.scanner.TransactionReplacedBy(ctx, req.GetTxid()) + if err != nil { + log.WithError(err).Warnf( + "failed to check whether tx %s was replaced", req.GetTxid(), + ) + } else { + replacedBy = replacement + } + } + return &arkwalletv1.IsTransactionConfirmedResponse{ Confirmed: confirmed, Blocknumber: blocknumber, Blocktime: blocktime, + ReplacedBy: replacedBy, }, nil } diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go index e97f03584..839a09301 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go @@ -62,6 +62,35 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { // Any other failure stays a failure. Reporting it as not found would tell // the caller the transaction is gone when the backend simply could not say. + // The signal the retraction actually depends on: a replaced transaction is + // still known and still answers "not confirmed", so only this names it. + t.Run("a replaced transaction reports its replacement", func(t *testing.T) { + const replacement = "4dc2f8e63b9dc3825f69c8295a48a9b87ba4c663f42ea7b49fa335746d626246" + h := &walletHandler{scanner: &confirmScanner{replacedBy: replacement}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.False(t, resp.GetConfirmed()) + require.False(t, resp.GetNotFound()) + require.Equal(t, replacement, resp.GetReplacedBy()) + }) + + // A backend that cannot answer must not be read as "not replaced", so the + // field stays empty and the caller sees no signal rather than a false one. + t.Run("a failed replacement lookup leaves the field empty", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{replacedErr: errors.New("backend down")}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.Empty(t, resp.GetReplacedBy()) + }) + t.Run("another failure is returned as an error", func(t *testing.T) { h := &walletHandler{scanner: &confirmScanner{err: errors.New("backend down")}} @@ -83,7 +112,13 @@ type confirmScanner struct { confirmed bool blockHeight int64 blockTime int64 + replacedBy string err error + replacedErr error +} + +func (s *confirmScanner) TransactionReplacedBy(_ context.Context, _ string) (string, error) { + return s.replacedBy, s.replacedErr } func (s *confirmScanner) IsTransactionConfirmed( From 354e87e0e4d16c7011b2d2a6cd452af366c8cba8 Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:42:02 -0400 Subject: [PATCH 4/7] vtxo: ask the node whether it still holds the unroll transaction The replacement signal cannot carry this on its own. The transaction that marks a vtxo unrolled is the pre-signed tree transaction, broadcast as a package with a client-signed fee-bumping child. Nobody can produce a conflicting version of a musig2-signed transaction, so it is never replaced; it fails by sitting in the mempool until the node lets it go. Keying only on replacement left the retraction unable to fire for the way an unroll actually fails. NBXplorer cannot see that either. It keeps a transaction in its own index after the node has dropped it, reporting zero confirmations indefinitely, so one waiting and one gone look identical through it. The node is the only component that knows, and NBXplorer will forward the question. So the wallet now answers a single judgement, dropped, meaning the transaction will not confirm: superseded, unknown, or unconfirmed and no longer held by the node. It is computed wallet-side because only the wallet can see all three, and it is false whenever the wallet cannot tell, including on an older wallet that never sets it, so nothing can act on a live transaction. Verified against the running regtest stack through the real client: a transaction the node dropped reports false, one waiting in the mempool reports true, and a confirmed one reports false, which is why the answer combines the mempool question with the confirmation state instead of reading either alone. --- .../arkwallet/v1/bitcoin_wallet.openapi.json | 4 ++ .../arkwallet/v1/bitcoin_wallet.proto | 8 +++ .../gen/arkwallet/v1/bitcoin_wallet.pb.go | 22 ++++++- .../infrastructure/wallet/wallet_client.go | 2 +- .../wallet/wallet_client_test.go | 14 ++++- .../core/application/scanner/service.go | 4 ++ pkg/arkd-wallet/core/application/types.go | 4 ++ .../core/infrastructure/nbxplorer/service.go | 55 ++++++++++++++++++ pkg/arkd-wallet/core/ports/nbxplorer.go | 4 ++ .../interface/grpc/handlers/wallet_handler.go | 23 +++++++- .../grpc/handlers/wallet_handler_test.go | 57 ++++++++++++++++++- 11 files changed, 188 insertions(+), 9 deletions(-) diff --git a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json index f761cfbd1..4103b52eb 100644 --- a/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json +++ b/api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json @@ -1713,6 +1713,10 @@ "confirmed": { "type": "boolean" }, + "dropped": { + "type": "boolean", + "description": "dropped is the wallet's judgement that this transaction will not confirm:\nsuperseded, unknown to the backend, or unconfirmed and no longer held by\nthe node. It is computed here because only the wallet can see all three.\n\nFalse whenever the wallet cannot tell, including on an older wallet that\nnever sets it, so a caller acting on this can never act on a live\ntransaction." + }, "notFound": { "type": "boolean", "description": "not_found separates a transaction the backend has no record of from one it\nknows and sees unconfirmed. Both answer confirmed = false, which is why the\ndistinction needs its own field.\n\nAdded after the other fields, and phrased so false is the safe reading: an\nolder wallet never sets it, and a caller then sees \"not known to be\nmissing\" rather than \"missing\"." diff --git a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto index d5c709f9a..810870e76 100644 --- a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto +++ b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto @@ -239,6 +239,14 @@ message IsTransactionConfirmedResponse { // Empty when the transaction was not replaced, which is also what an older // wallet returns, so the safe reading is the zero value here too. string replaced_by = 5; + // dropped is the wallet's judgement that this transaction will not confirm: + // superseded, unknown to the backend, or unconfirmed and no longer held by + // the node. It is computed here because only the wallet can see all three. + // + // False whenever the wallet cannot tell, including on an older wallet that + // never sets it, so a caller acting on this can never act on a live + // transaction. + bool dropped = 6; } message GetOutpointStatusRequest { diff --git a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go index c223389b7..b5a2bec54 100644 --- a/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go +++ b/api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go @@ -166,7 +166,15 @@ type IsTransactionConfirmedResponse struct { // // Empty when the transaction was not replaced, which is also what an older // wallet returns, so the safe reading is the zero value here too. - ReplacedBy string `protobuf:"bytes,5,opt,name=replaced_by,json=replacedBy,proto3" json:"replaced_by,omitempty"` + ReplacedBy string `protobuf:"bytes,5,opt,name=replaced_by,json=replacedBy,proto3" json:"replaced_by,omitempty"` + // dropped is the wallet's judgement that this transaction will not confirm: + // superseded, unknown to the backend, or unconfirmed and no longer held by + // the node. It is computed here because only the wallet can see all three. + // + // False whenever the wallet cannot tell, including on an older wallet that + // never sets it, so a caller acting on this can never act on a live + // transaction. + Dropped bool `protobuf:"varint,6,opt,name=dropped,proto3" json:"dropped,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -236,6 +244,13 @@ func (x *IsTransactionConfirmedResponse) GetReplacedBy() string { return "" } +func (x *IsTransactionConfirmedResponse) GetDropped() bool { + if x != nil { + return x.Dropped + } + return false +} + type GetOutpointStatusRequest struct { state protoimpl.MessageState `protogen:"open.v1"` Txid string `protobuf:"bytes,1,opt,name=txid,proto3" json:"txid,omitempty"` @@ -3809,14 +3824,15 @@ const file_arkwallet_v1_bitcoin_wallet_proto_rawDesc = "" + "\x16GetReadyUpdateResponse\x12\x14\n" + "\x05ready\x18\x01 \x01(\bR\x05ready\"3\n" + "\x1dIsTransactionConfirmedRequest\x12\x12\n" + - "\x04txid\x18\x01 \x01(\tR\x04txid\"\xbc\x01\n" + + "\x04txid\x18\x01 \x01(\tR\x04txid\"\xd6\x01\n" + "\x1eIsTransactionConfirmedResponse\x12\x1c\n" + "\tconfirmed\x18\x01 \x01(\bR\tconfirmed\x12 \n" + "\vblocknumber\x18\x02 \x01(\x03R\vblocknumber\x12\x1c\n" + "\tblocktime\x18\x03 \x01(\x03R\tblocktime\x12\x1b\n" + "\tnot_found\x18\x04 \x01(\bR\bnotFound\x12\x1f\n" + "\vreplaced_by\x18\x05 \x01(\tR\n" + - "replacedBy\"B\n" + + "replacedBy\x12\x18\n" + + "\adropped\x18\x06 \x01(\bR\adropped\"B\n" + "\x18GetOutpointStatusRequest\x12\x12\n" + "\x04txid\x18\x01 \x01(\tR\x04txid\x12\x12\n" + "\x04vout\x18\x02 \x01(\rR\x04vout\"1\n" + diff --git a/internal/infrastructure/wallet/wallet_client.go b/internal/infrastructure/wallet/wallet_client.go index a8e950c36..185199898 100644 --- a/internal/infrastructure/wallet/wallet_client.go +++ b/internal/infrastructure/wallet/wallet_client.go @@ -455,7 +455,7 @@ func (w *walletDaemonClient) IsTransactionDropped( if err != nil { return false, err } - return resp.GetNotFound() || resp.GetReplacedBy() != "", nil + return resp.GetDropped(), nil } func (w *walletDaemonClient) IsTransactionConfirmed( diff --git a/internal/infrastructure/wallet/wallet_client_test.go b/internal/infrastructure/wallet/wallet_client_test.go index 23958c7ff..f20d599cd 100644 --- a/internal/infrastructure/wallet/wallet_client_test.go +++ b/internal/infrastructure/wallet/wallet_client_test.go @@ -305,10 +305,22 @@ func TestIsTransactionDropped(t *testing.T) { // The case that actually fires in production. A replaced transaction is // still known to the backend and still answers "not confirmed", so only the // replacement signal distinguishes it from one merely waiting. + t.Run("a transaction the node no longer holds is dropped", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{Dropped: true}, + }} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.NoError(t, err) + require.True(t, dropped) + }) + t.Run("a replaced transaction is dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{ ReplacedBy: "4dc2f8e63b9dc3825f69c8295a48a9b87ba4c663f42ea7b49fa335746d626246", + Dropped: true, }, }} @@ -320,7 +332,7 @@ func TestIsTransactionDropped(t *testing.T) { t.Run("a transaction the backend has no record of is dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ - resp: &arkwalletv1.IsTransactionConfirmedResponse{NotFound: true}, + resp: &arkwalletv1.IsTransactionConfirmedResponse{NotFound: true, Dropped: true}, }} dropped, err := w.IsTransactionDropped(t.Context(), txid) diff --git a/pkg/arkd-wallet/core/application/scanner/service.go b/pkg/arkd-wallet/core/application/scanner/service.go index 5ade29a07..f3e54d8cc 100644 --- a/pkg/arkd-wallet/core/application/scanner/service.go +++ b/pkg/arkd-wallet/core/application/scanner/service.go @@ -276,6 +276,10 @@ func (s *scanner) IsTransactionConfirmed(ctx context.Context, txid string) (isCo return details.Confirmations > 0, int64(details.Height), details.Timestamp, nil } +func (s *scanner) IsTransactionInMempool(ctx context.Context, txid string) (bool, error) { + return s.nbxplorer.IsInMempool(ctx, txid) +} + func (s *scanner) TransactionReplacedBy(ctx context.Context, txid string) (string, error) { details, err := s.nbxplorer.GetTransaction(ctx, txid) if err != nil { diff --git a/pkg/arkd-wallet/core/application/types.go b/pkg/arkd-wallet/core/application/types.go index caa799505..1ece32dba 100644 --- a/pkg/arkd-wallet/core/application/types.go +++ b/pkg/arkd-wallet/core/application/types.go @@ -68,6 +68,10 @@ type BlockchainScanner interface { // TransactionReplacedBy names the transaction the backend has recorded as // having superseded this one, or empty when it has not been replaced. TransactionReplacedBy(ctx context.Context, txid string) (string, error) + // IsTransactionInMempool reports whether the node is holding the + // transaction. Combined with the confirmation state it is what separates a + // transaction still waiting from one the node has dropped. + IsTransactionInMempool(ctx context.Context, txid string) (bool, error) GetOutpointStatus(ctx context.Context, outpoint wire.OutPoint) (spent bool, err error) // GetSpends returns every watched output spent by a confirmed or unconfirmed // transaction, windowed from the given instant when one is supplied. diff --git a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go index 5bcf2b8ba..63f25659a 100644 --- a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go +++ b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go @@ -36,6 +36,10 @@ const ( // an ordinary outcome rather than a failure. var errNotFound = errors.New("not found") +// rpcInvalidAddressOrKey is bitcoind's RPC_INVALID_ADDRESS_OR_KEY, which +// getmempoolentry returns for a transaction the node is not holding. +const rpcInvalidAddressOrKey = -5 + type nbxplorer struct { url string httpClient *http.Client @@ -1054,6 +1058,57 @@ func (n *nbxplorer) GetTxSpends(ctx context.Context, txid string) ([]ports.Spend // value would therefore report a mempool-spent output as unspent, and a caller // using this to retract spends would undo every mempool spend one tick after // recording it. +// IsInMempool asks the node, through NBXplorer's RPC proxy, whether it is +// holding the transaction. +// +// The node is the only component that knows this. NBXplorer keeps a transaction +// in its own index after the node has dropped it, reporting zero confirmations +// indefinitely, so its view cannot separate one waiting in the mempool from one +// that is gone. Verified against a live regtest node: a replaced transaction the +// node no longer holds answers "Transaction not in mempool", exactly as a +// confirmed one does, which is why the caller must combine this with the +// confirmation state rather than read it alone. +func (n *nbxplorer) IsInMempool(ctx context.Context, txid string) (bool, error) { + if _, err := chainhash.NewHashFromStr(txid); err != nil { + return false, fmt.Errorf("invalid txid format: %w", err) + } + + body := fmt.Sprintf( + `{"jsonrpc":"1.0","id":"arkd","method":"getmempoolentry","params":[%q]}`, txid, + ) + endpoint := fmt.Sprintf("/v1/cryptos/%s/rpc", btcCryptoCode) + data, err := n.makeRequest(ctx, "POST", endpoint, strings.NewReader(body)) + if err != nil { + return false, fmt.Errorf("failed to query the node mempool: %w", err) + } + + // A JSON-RPC failure arrives with an HTTP 200 and an error member, so the + // body has to be read rather than the status. + var resp struct { + Result json.RawMessage `json:"result"` + Error *struct { + Code int `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(data, &resp); err != nil { + return false, fmt.Errorf("failed to unmarshal mempool entry: %v", err) + } + + if resp.Error != nil { + // The node says it does not hold the transaction. Every other failure is + // reported, so a caller never reads "cannot tell" as "not there". + if resp.Error.Code == rpcInvalidAddressOrKey { + return false, nil + } + return false, fmt.Errorf( + "node rejected the mempool query: %s (code %d)", resp.Error.Message, resp.Error.Code, + ) + } + + return len(resp.Result) > 0 && string(resp.Result) != "null", nil +} + func (n *nbxplorer) GetUnspentOutpoints( ctx context.Context, ) (map[wire.OutPoint]struct{}, error) { diff --git a/pkg/arkd-wallet/core/ports/nbxplorer.go b/pkg/arkd-wallet/core/ports/nbxplorer.go index 0a1819643..ea6631d0c 100644 --- a/pkg/arkd-wallet/core/ports/nbxplorer.go +++ b/pkg/arkd-wallet/core/ports/nbxplorer.go @@ -87,6 +87,10 @@ type Nbxplorer interface { // here is positive evidence both that an outpoint is unspent and that its // script is still tracked, which is what makes it safe to retract a spend. GetUnspentOutpoints(ctx context.Context) (map[wire.OutPoint]struct{}, error) + // IsInMempool reports whether the node itself is holding the transaction. + // NBXplorer's own index keeps a transaction after the node has dropped it, + // so this is the only way to tell one waiting from one that is gone. + IsInMempool(ctx context.Context, txid string) (bool, error) WatchAddresses(ctx context.Context, addresses ...string) error UnwatchAddresses(ctx context.Context, addresses ...string) error GetAddressNotifications(ctx context.Context) (<-chan ChainNotification, error) diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go index b6f3ce7d2..4b03d2902 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go @@ -351,14 +351,17 @@ func (h *walletHandler) IsTransactionConfirmed( Blocknumber: 0, Blocktime: 0, NotFound: true, + Dropped: true, }, nil } return nil, err } - // Only an unconfirmed transaction can have been replaced, so the second - // lookup is skipped for the common case. A failure to answer is not - // reported as "not replaced": it is left empty, which reads as no signal. + // A confirmed transaction is settled, so neither extra question is asked. + // For an unconfirmed one both are, and a failure to answer either leaves the + // result empty or false rather than asserting anything: the caller must + // never read "cannot tell" as "gone". var replacedBy string + var dropped bool if !confirmed { replacement, err := h.scanner.TransactionReplacedBy(ctx, req.GetTxid()) if err != nil { @@ -368,6 +371,19 @@ func (h *walletHandler) IsTransactionConfirmed( } else { replacedBy = replacement } + + // The node is the only component that knows it has dropped a + // transaction: NBXplorer keeps reporting one at zero confirmations long + // after the node let it go. + inMempool, err := h.scanner.IsTransactionInMempool(ctx, req.GetTxid()) + if err != nil { + log.WithError(err).Warnf( + "failed to check whether tx %s is still in the mempool", req.GetTxid(), + ) + } else { + dropped = !inMempool + } + dropped = dropped || replacedBy != "" } return &arkwalletv1.IsTransactionConfirmedResponse{ @@ -375,6 +391,7 @@ func (h *walletHandler) IsTransactionConfirmed( Blocknumber: blocknumber, Blocktime: blocktime, ReplacedBy: replacedBy, + Dropped: dropped, }, nil } diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go index 839a09301..2c85294a2 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go @@ -34,7 +34,7 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { // mined yet. Flagging this one would let a caller treat a live transaction // as gone. t.Run("an unconfirmed transaction is not flagged not found", func(t *testing.T) { - h := &walletHandler{scanner: &confirmScanner{}} + h := &walletHandler{scanner: &confirmScanner{inMempool: true}} resp, err := h.IsTransactionConfirmed( context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, @@ -91,6 +91,55 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { require.Empty(t, resp.GetReplacedBy()) }) + // The signal that actually fires for an unroll. The materialising tx is + // pre-signed and cannot be replaced, so the way it fails is by being dropped + // from the mempool, and only the node knows that. + t.Run("an unconfirmed tx the node no longer holds is dropped", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{inMempool: false}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.True(t, resp.GetDropped()) + }) + + t.Run("a tx still in the mempool is not dropped", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{inMempool: true}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.False(t, resp.GetDropped(), "waiting is not the same as gone") + }) + + // Both the confirmed case and an unanswerable node must read as not dropped, + // so nothing acts on a transaction that may well be alive. + t.Run("a confirmed tx is not dropped", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{confirmed: true}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.False(t, resp.GetDropped()) + }) + + t.Run("a node that cannot answer leaves it not dropped", func(t *testing.T) { + h := &walletHandler{scanner: &confirmScanner{mempoolErr: errors.New("rpc disabled")}} + + resp, err := h.IsTransactionConfirmed( + context.Background(), &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + + require.NoError(t, err) + require.False(t, resp.GetDropped(), "cannot tell must never read as gone") + }) + t.Run("another failure is returned as an error", func(t *testing.T) { h := &walletHandler{scanner: &confirmScanner{err: errors.New("backend down")}} @@ -113,14 +162,20 @@ type confirmScanner struct { blockHeight int64 blockTime int64 replacedBy string + inMempool bool err error replacedErr error + mempoolErr error } func (s *confirmScanner) TransactionReplacedBy(_ context.Context, _ string) (string, error) { return s.replacedBy, s.replacedErr } +func (s *confirmScanner) IsTransactionInMempool(_ context.Context, _ string) (bool, error) { + return s.inMempool, s.mempoolErr +} + func (s *confirmScanner) IsTransactionConfirmed( _ context.Context, _ string, ) (bool, int64, int64, error) { From d94ebb1aab75396669e0f18a80d0933f2007e13c Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:36:20 -0400 Subject: [PATCH 5/7] vtxo: let the node's answer decide, not the backend's stale unspent set Driving this end to end on regtest showed the retraction correctly clearing six vtxos left stuck by earlier runs, and then refusing to clear the one the test had just unrolled and evicted. The unspent-set check was the reason. That check read as the stronger signal: an outpoint the wallet still lists as unspent exists on chain, so nothing may retract past it. But the list is built from the chain backend's own index, which keeps an unconfirmed transaction long after the node has dropped it. With the unroll evicted, the node reported no such output while the backend still listed the outpoint as unspent, so the guard vetoed exactly the case it was guarding. The node's answer already accounts for confirmation, so the check added no safety and only blocked the fix. Retraction now rests on that single piece of positive evidence, which also removes a wallet call from every reconcile pass. Verified on the rebuilt stack: the vtxo whose unroll was evicted is retracted within one reconcile pass, and one whose transaction the node still holds is not. --- .../core/application/unroll_retraction.go | 32 +++++++----------- .../application/unroll_retraction_test.go | 33 +++++++++++-------- 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/internal/core/application/unroll_retraction.go b/internal/core/application/unroll_retraction.go index f280ee0e5..e238155cf 100644 --- a/internal/core/application/unroll_retraction.go +++ b/internal/core/application/unroll_retraction.go @@ -26,37 +26,27 @@ const unrollRetractionObservations = 3 // the vtxo wrongly unrolled forever, unspendable by its owner and unsweepable by // the operator. // -// Retraction runs on positive evidence in both directions, never on absence -// alone. The outpoint being present in the wallet's unspent set says the output -// exists, and no number of passes can retract past it. Only once that is absent -// does the transaction lookup decide, and only a backend that positively has no -// record of the transaction counts, repeatedly. +// Retraction runs on one piece of positive evidence: the node itself no longer +// holds the transaction that would materialise the vtxo, and has not mined it. +// +// The wallet's unspent set is deliberately not consulted. It reads as the +// stronger signal and was used here at first, but it is built from the chain +// backend's own index, which keeps an unconfirmed transaction long after the +// node has dropped it. Verified on a live stack: with the unroll evicted, the +// node reported no such output while the backend still listed the outpoint as +// unspent, so consulting it vetoed exactly the retraction this exists to make. +// The node's answer already accounts for confirmation, so nothing is lost. func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.Vtxo) { if len(candidates) == 0 { s.forgetUnrollObservations(nil) return } - // The unspent set is the stronger signal and is checked first: an outpoint - // listed here exists on chain, whatever the transaction lookup says. - unspent, err := s.scanner.GetUnspentOutpoints(ctx) - if err != nil { - log.WithError(err).Warn( - "unroll retraction: failed to fetch unspent outpoints, skipping this pass", - ) - return - } - seen := make(map[domain.Outpoint]struct{}, len(candidates)) stale := make([]domain.Outpoint, 0) for _, vtxo := range candidates { seen[vtxo.Outpoint] = struct{}{} - if _, ok := unspent[vtxo.Outpoint]; ok { - s.recordUnrollObservation(vtxo.Outpoint, true) - continue - } - dropped, err := s.scanner.IsTransactionDropped(ctx, vtxo.Txid) if err != nil { // Not evidence of anything. Leave the count untouched so a flapping @@ -89,7 +79,7 @@ func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.V for _, outpoint := range stale { s.recordUnrollObservation(outpoint, true) log.Debugf( - "vtxo %s unroll retracted, its tx is no longer known to the chain", outpoint, + "vtxo %s unroll retracted, its tx is no longer held by the node", outpoint, ) } } diff --git a/internal/core/application/unroll_retraction_test.go b/internal/core/application/unroll_retraction_test.go index faef85076..eb333e46d 100644 --- a/internal/core/application/unroll_retraction_test.go +++ b/internal/core/application/unroll_retraction_test.go @@ -31,13 +31,30 @@ func TestRetractStaleUnrolls(t *testing.T) { vtxos.AssertCalled(t, "UnmarkVtxosUnrolled", mock.Anything, []domain.Outpoint{out}) }) - // The stronger of the two signals. An outpoint the wallet still lists as - // unspent exists on chain, so no number of passes may retract past it. - t.Run("never retracts while the outpoint is still unspent", func(t *testing.T) { + // The wallet's unspent set must not veto the node. It is built from the + // backend's own index, which keeps an unconfirmed transaction after the node + // has dropped it, so an outpoint listed there says nothing about whether the + // unroll will confirm. + t.Run("a stale unspent listing does not block the retraction", func(t *testing.T) { svc, vtxos := unrollService(t, &mockedScanner{ unspent: map[domain.Outpoint]struct{}{out: {}}, dropped: map[string]bool{unrolledVtxoTxid: true}, }) + vtxos.On("UnmarkVtxosUnrolled", mock.Anything, mock.Anything).Return(nil) + + for range unrollRetractionObservations { + svc.retractStaleUnrolls(ctx, candidates) + } + + vtxos.AssertCalled(t, "UnmarkVtxosUnrolled", mock.Anything, []domain.Outpoint{out}) + }) + + // The node still holding the transaction is what blocks a retraction. + t.Run("never retracts while the node still holds the tx", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + dropped: map[string]bool{unrolledVtxoTxid: false}, + }) for range unrollRetractionObservations * 2 { svc.retractStaleUnrolls(ctx, candidates) @@ -82,16 +99,6 @@ func TestRetractStaleUnrolls(t *testing.T) { vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) - t.Run("an unspent-set failure skips the pass without asking further", func(t *testing.T) { - scanner := &mockedScanner{unspentErr: errors.New("wallet down")} - svc, vtxos := unrollService(t, scanner) - - svc.retractStaleUnrolls(ctx, candidates) - - require.Empty(t, scanner.DroppedCalls(), "no tx lookup without the stronger signal") - vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) - }) - // A vtxo that leaves the candidate set has been resolved some other way, so // its count must not survive to be counted against a later unroll. t.Run("counts do not survive leaving the candidate set", func(t *testing.T) { From 45cf7229681e9f5ac0af04d7aedcdc7a2eae0b05 Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Fri, 11 Sep 2026 00:52:46 -0400 Subject: [PATCH 6/7] unroll-retraction: fix comment style in the new code The comments added by this branch break two of the repo's prose rules. Sixteen of them use colon syntax, where a clause after a colon carries the real explanation. Each is rewritten as two sentences or joined with "since", and the two that introduce a list use "namely" so the list is not left as a fragment. Three narrate the change rather than the code. The retraction doc said the unspent set "was used here at first", the badger doc said a divergence "predates this method and is recorded rather than papered over", and the retraction doc claimed "Nothing ever cleared it" when this branch is exactly what clears it now. The first two are dropped and the third is stated in the present tense. Two comments also sat above the wrong subtest, describing a replaced transaction above a case that has nothing to do with replacement, so they are moved to the subtests they explain. Separately, the IsInMempool doc comment was appended to the end of the GetUnspentOutpoints doc comment instead of being given its own block, which detached GetUnspentOutpoints from its documentation and gave IsInMempool an eight line preamble about unspent outputs. The GetUnspentOutpoints block moves back above its own function. Comments only, no behaviour changes. --- internal/core/application/onchain_spend.go | 2 +- internal/core/application/service.go | 5 ++- .../core/application/unroll_retraction.go | 37 ++++++++++--------- .../application/unroll_retraction_test.go | 6 +-- internal/core/ports/scanner.go | 14 +++---- .../infrastructure/db/badger/vtxo_repo.go | 7 ++-- .../infrastructure/wallet/wallet_client.go | 6 +-- .../wallet/wallet_client_test.go | 6 +-- .../core/infrastructure/nbxplorer/service.go | 24 ++++++------ .../interface/grpc/handlers/wallet_handler.go | 6 +-- .../grpc/handlers/wallet_handler_test.go | 12 +++--- 11 files changed, 63 insertions(+), 62 deletions(-) diff --git a/internal/core/application/onchain_spend.go b/internal/core/application/onchain_spend.go index 337d3356b..59970e141 100644 --- a/internal/core/application/onchain_spend.go +++ b/internal/core/application/onchain_spend.go @@ -167,7 +167,7 @@ func (s *service) reconcileOnchainSpendsOnce(ctx context.Context, from *time.Tim s.retractStaleOnchainSpends(ctx, recorded) - // Same candidate set, the other direction: a spend that never confirmed is + // Same candidate set, the other direction. A spend that never confirmed is // retracted above, an unroll that never confirmed is retracted here. s.retractStaleUnrolls(ctx, candidates) } diff --git a/internal/core/application/service.go b/internal/core/application/service.go index 30bcf26c6..eafaa7d6a 100644 --- a/internal/core/application/service.go +++ b/internal/core/application/service.go @@ -59,8 +59,9 @@ type service struct { onchainSpendReconcileInterval time.Duration // consecutive reconcile passes that found a vtxo's materialising tx unknown - // to the chain backend, keyed by outpoint. In memory on purpose: losing it - // on restart only delays a retraction, which is the safe direction. + // to the chain backend, keyed by outpoint. In memory on purpose, since + // losing it on restart only delays a retraction, which is the safe + // direction. unrollObservations map[domain.Outpoint]int unrollObservationsMu sync.Mutex diff --git a/internal/core/application/unroll_retraction.go b/internal/core/application/unroll_retraction.go index e238155cf..42c17eba9 100644 --- a/internal/core/application/unroll_retraction.go +++ b/internal/core/application/unroll_retraction.go @@ -9,33 +9,34 @@ import ( // unrollRetractionObservations is how many consecutive reconcile passes must // find a vtxo's materialising tx dropped by the chain backend before its unroll -// is retracted. One pass is not enough: a transaction that has been broadcast -// but has not reached our node yet reads exactly like one that is gone, and -// retracting a live unroll would hand its owner back a vtxo whose output exists -// on chain. Several passes apart make that reading stable rather than a race -// with propagation. +// is retracted. One pass is not enough, since a transaction that has been +// broadcast but has not reached our node yet reads exactly like one that is +// gone, and retracting a live unroll would hand its owner back a vtxo whose +// output exists on chain. Several passes apart make that reading stable rather +// than a race with propagation. const unrollRetractionObservations = 3 // retractStaleUnrolls clears the unrolled mark on vtxos whose materialising // transaction the chain backend no longer has any record of. // // A vtxo is marked unrolled the moment its outpoint appears on chain, before any -// confirmation, which is deliberate: the mark is protective and blocks the vtxo -// from being spent inside the Ark while its unroll is in flight. Nothing ever -// cleared it, so an unroll that is evicted or replaced and never mines leaves -// the vtxo wrongly unrolled forever, unspendable by its owner and unsweepable by -// the operator. +// confirmation. That is deliberate, since the mark is protective and blocks the +// vtxo from being spent inside the Ark while its unroll is in flight. Nothing +// else clears it, so an unroll that is evicted or replaced and never mines +// leaves the vtxo wrongly unrolled forever, unspendable by its owner and +// unsweepable by the operator. // -// Retraction runs on one piece of positive evidence: the node itself no longer -// holds the transaction that would materialise the vtxo, and has not mined it. +// Retraction runs on one piece of positive evidence, namely that the node itself +// no longer holds the transaction that would materialise the vtxo, and has not +// mined it. // // The wallet's unspent set is deliberately not consulted. It reads as the -// stronger signal and was used here at first, but it is built from the chain -// backend's own index, which keeps an unconfirmed transaction long after the -// node has dropped it. Verified on a live stack: with the unroll evicted, the -// node reported no such output while the backend still listed the outpoint as -// unspent, so consulting it vetoed exactly the retraction this exists to make. -// The node's answer already accounts for confirmation, so nothing is lost. +// stronger signal, but it is built from the chain backend's own index, which +// keeps an unconfirmed transaction long after the node has dropped it. On a live +// stack with the unroll evicted, the node reported no such output while the +// backend still listed the outpoint as unspent, so consulting it vetoed exactly +// the retraction this exists to make. The node's answer already accounts for +// confirmation, so nothing is lost. func (s *service) retractStaleUnrolls(ctx context.Context, candidates []domain.Vtxo) { if len(candidates) == 0 { s.forgetUnrollObservations(nil) diff --git a/internal/core/application/unroll_retraction_test.go b/internal/core/application/unroll_retraction_test.go index eb333e46d..08a7b7ace 100644 --- a/internal/core/application/unroll_retraction_test.go +++ b/internal/core/application/unroll_retraction_test.go @@ -117,9 +117,9 @@ func TestRetractStaleUnrolls(t *testing.T) { vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) }) - // Without this the retraction is unreachable in production: every unit above - // calls retractStaleUnrolls directly, so removing its one call site from the - // reconcile pass would leave them all green. + // Without this the retraction is unreachable in production, since every unit + // above calls retractStaleUnrolls directly, so removing its one call site + // from the reconcile pass would leave them all green. t.Run("the reconcile pass drives the retraction", func(t *testing.T) { vtxos := &mockedVtxoRepo{} vtxos.On("GetUnrolledUnspentVtxos", mock.Anything).Return(candidates, nil) diff --git a/internal/core/ports/scanner.go b/internal/core/ports/scanner.go index 6b3707c36..ff6bd7fd7 100644 --- a/internal/core/ports/scanner.go +++ b/internal/core/ports/scanner.go @@ -32,15 +32,15 @@ type BlockchainScanner interface { ctx context.Context, txid string, ) (isConfirmed bool, blockTimestamp *BlockTimestamp, err error) // IsTransactionDropped reports that the chain backend has positively - // determined the transaction will not confirm: either it was superseded by - // another transaction, or the backend has no record of it at all. + // determined the transaction will not confirm, namely that it was superseded + // by another transaction, or that the backend has no record of it at all. // // It is deliberately not the negation of IsTransactionConfirmed, which - // answers false for a transaction merely waiting in the mempool. Verified - // against a live NBXplorer: a replaced transaction keeps answering zero - // confirmations indefinitely and is never reported as missing, so being - // superseded is the only positive statement the backend makes, and it makes - // it once the replacement confirms. + // answers false for a transaction merely waiting in the mempool. Against a + // live NBXplorer a replaced transaction keeps answering zero confirmations + // indefinitely and is never reported as missing, so being superseded is the + // only positive statement the backend makes, and it makes it once the + // replacement confirms. IsTransactionDropped(ctx context.Context, txid string) (bool, error) // GetSpends returns every watched output spent by a confirmed or unconfirmed // transaction, windowed from the given instant when one is supplied. diff --git a/internal/infrastructure/db/badger/vtxo_repo.go b/internal/infrastructure/db/badger/vtxo_repo.go index f1eb705d5..563babc26 100644 --- a/internal/infrastructure/db/badger/vtxo_repo.go +++ b/internal/infrastructure/db/badger/vtxo_repo.go @@ -109,10 +109,9 @@ func (r *VtxoRepository) UnrollVtxos( // UnmarkVtxosUnrolled retracts an unroll whose transaction the chain backend no // longer has any record of. // -// ExpiresAt is deliberately not restored: unrollVtxo zeroes it and the original -// value is gone, so a retracted vtxo keeps a zero expiry here where the SQL -// backends keep the value they never cleared. That divergence predates this -// method and is recorded rather than papered over. +// ExpiresAt is deliberately not restored, since unrollVtxo zeroes it and the +// original value is gone, so a retracted vtxo keeps a zero expiry here where the +// SQL backends keep the value they never cleared. func (r *VtxoRepository) UnmarkVtxosUnrolled( ctx context.Context, outpoints []domain.Outpoint, ) error { diff --git a/internal/infrastructure/wallet/wallet_client.go b/internal/infrastructure/wallet/wallet_client.go index 185199898..a24bd3e6f 100644 --- a/internal/infrastructure/wallet/wallet_client.go +++ b/internal/infrastructure/wallet/wallet_client.go @@ -443,9 +443,9 @@ func castSpends(spends []*arkwalletv1.SpendInfo) []ports.Spend { } // IsTransactionDropped reads the two ways the wallet says a transaction will -// not confirm: replaced by another one, or unknown to its backend. An older -// wallet sets neither, so every transaction reads as still live and no caller -// can act on one that is gone. +// not confirm, namely replaced by another one, or unknown to its backend. An +// older wallet sets neither, so every transaction reads as still live and no +// caller can act on one that is gone. func (w *walletDaemonClient) IsTransactionDropped( ctx context.Context, txid string, ) (bool, error) { diff --git a/internal/infrastructure/wallet/wallet_client_test.go b/internal/infrastructure/wallet/wallet_client_test.go index f20d599cd..b255e8f55 100644 --- a/internal/infrastructure/wallet/wallet_client_test.go +++ b/internal/infrastructure/wallet/wallet_client_test.go @@ -302,9 +302,6 @@ func TestNotificationStreamIsShared(t *testing.T) { func TestIsTransactionDropped(t *testing.T) { const txid = "4ba63c204f39841e3a7c98e458586307cf6d33bbed9a9a520c827ab043f32701" - // The case that actually fires in production. A replaced transaction is - // still known to the backend and still answers "not confirmed", so only the - // replacement signal distinguishes it from one merely waiting. t.Run("a transaction the node no longer holds is dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{Dropped: true}, @@ -316,6 +313,9 @@ func TestIsTransactionDropped(t *testing.T) { require.True(t, dropped) }) + // The case that actually fires in production. A replaced transaction is + // still known to the backend and still answers "not confirmed", so only the + // replacement signal distinguishes it from one merely waiting. t.Run("a replaced transaction is dropped", func(t *testing.T) { w := &walletDaemonClient{client: &confirmFakeClient{ resp: &arkwalletv1.IsTransactionConfirmedResponse{ diff --git a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go index 63f25659a..6200f8352 100644 --- a/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go +++ b/pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go @@ -1050,24 +1050,16 @@ func (n *nbxplorer) GetTxSpends(ctx context.Context, txid string) ([]ports.Spend return castSpends(tx), nil } -// GetUnspentOutpoints returns the tracked outputs that are currently unspent. -// -// The subtraction matters: NBXplorer keeps a confirmed output in Confirmed.UtxOs -// even while the transaction spending it sits in the mempool, listing it in -// Unconfirmed.SpentOutpoints at the same time. Taking Confirmed.UtxOs at face -// value would therefore report a mempool-spent output as unspent, and a caller -// using this to retract spends would undo every mempool spend one tick after -// recording it. // IsInMempool asks the node, through NBXplorer's RPC proxy, whether it is // holding the transaction. // // The node is the only component that knows this. NBXplorer keeps a transaction // in its own index after the node has dropped it, reporting zero confirmations // indefinitely, so its view cannot separate one waiting in the mempool from one -// that is gone. Verified against a live regtest node: a replaced transaction the -// node no longer holds answers "Transaction not in mempool", exactly as a -// confirmed one does, which is why the caller must combine this with the -// confirmation state rather than read it alone. +// that is gone. Against a live regtest node a replaced transaction the node no +// longer holds answers "Transaction not in mempool", exactly as a confirmed one +// does, which is why the caller must combine this with the confirmation state +// rather than read it alone. func (n *nbxplorer) IsInMempool(ctx context.Context, txid string) (bool, error) { if _, err := chainhash.NewHashFromStr(txid); err != nil { return false, fmt.Errorf("invalid txid format: %w", err) @@ -1109,6 +1101,14 @@ func (n *nbxplorer) IsInMempool(ctx context.Context, txid string) (bool, error) return len(resp.Result) > 0 && string(resp.Result) != "null", nil } +// GetUnspentOutpoints returns the tracked outputs that are currently unspent. +// +// The subtraction matters: NBXplorer keeps a confirmed output in Confirmed.UtxOs +// even while the transaction spending it sits in the mempool, listing it in +// Unconfirmed.SpentOutpoints at the same time. Taking Confirmed.UtxOs at face +// value would therefore report a mempool-spent output as unspent, and a caller +// using this to retract spends would undo every mempool spend one tick after +// recording it. func (n *nbxplorer) GetUnspentOutpoints( ctx context.Context, ) (map[wire.OutPoint]struct{}, error) { diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go index 4b03d2902..5e29a05e1 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go @@ -358,8 +358,8 @@ func (h *walletHandler) IsTransactionConfirmed( } // A confirmed transaction is settled, so neither extra question is asked. // For an unconfirmed one both are, and a failure to answer either leaves the - // result empty or false rather than asserting anything: the caller must - // never read "cannot tell" as "gone". + // result empty or false rather than asserting anything, since the caller + // must never read "cannot tell" as "gone". var replacedBy string var dropped bool if !confirmed { @@ -373,7 +373,7 @@ func (h *walletHandler) IsTransactionConfirmed( } // The node is the only component that knows it has dropped a - // transaction: NBXplorer keeps reporting one at zero confirmations long + // transaction. NBXplorer keeps reporting one at zero confirmations long // after the node let it go. inMempool, err := h.scanner.IsTransactionInMempool(ctx, req.GetTxid()) if err != nil { diff --git a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go index 2c85294a2..741db20cf 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go @@ -30,9 +30,9 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { require.True(t, resp.GetNotFound()) }) - // The case the flag exists to distinguish: known to the backend, just not - // mined yet. Flagging this one would let a caller treat a live transaction - // as gone. + // The case the flag exists to distinguish, namely known to the backend, just + // not mined yet. Flagging this one would let a caller treat a live + // transaction as gone. t.Run("an unconfirmed transaction is not flagged not found", func(t *testing.T) { h := &walletHandler{scanner: &confirmScanner{inMempool: true}} @@ -60,9 +60,7 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { require.EqualValues(t, 964276, resp.GetBlocknumber()) }) - // Any other failure stays a failure. Reporting it as not found would tell - // the caller the transaction is gone when the backend simply could not say. - // The signal the retraction actually depends on: a replaced transaction is + // The signal the retraction actually depends on. A replaced transaction is // still known and still answers "not confirmed", so only this names it. t.Run("a replaced transaction reports its replacement", func(t *testing.T) { const replacement = "4dc2f8e63b9dc3825f69c8295a48a9b87ba4c663f42ea7b49fa335746d626246" @@ -140,6 +138,8 @@ func TestIsTransactionConfirmedNotFound(t *testing.T) { require.False(t, resp.GetDropped(), "cannot tell must never read as gone") }) + // Any other failure stays a failure. Reporting it as not found would tell + // the caller the transaction is gone when the backend simply could not say. t.Run("another failure is returned as an error", func(t *testing.T) { h := &walletHandler{scanner: &confirmScanner{err: errors.New("backend down")}} From fb348a2cac7ec80c731b0415788ac686cf125101 Mon Sep 17 00:00:00 2001 From: Bob Smith <5396652+bitcoin-coder-bob@users.noreply.github.com> Date: Fri, 11 Sep 2026 01:01:02 -0400 Subject: [PATCH 7/7] arkwallet: drop colon syntax from the proto comments The Go pass did not look at the proto, so three field comments kept their colon sentences. Two continue as a new sentence and one reads better joined. --- api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto index 810870e76..c5c9a3208 100644 --- a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto +++ b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto @@ -227,20 +227,20 @@ message IsTransactionConfirmedResponse { // knows and sees unconfirmed. Both answer confirmed = false, which is why the // distinction needs its own field. // - // Added after the other fields, and phrased so false is the safe reading: an + // Added after the other fields, and phrased so false is the safe reading. An // older wallet never sets it, and a caller then sees "not known to be // missing" rather than "missing". bool not_found = 4; // replaced_by names the transaction that superseded this one. It is the - // backend's only positive statement that a transaction will not confirm: a - // replaced transaction keeps answering confirmed = false and not_found = + // backend's only positive statement that a transaction will not confirm, since + // a replaced transaction keeps answering confirmed = false and not_found = // false forever, so neither of those can stand in for it. // // Empty when the transaction was not replaced, which is also what an older // wallet returns, so the safe reading is the zero value here too. string replaced_by = 5; - // dropped is the wallet's judgement that this transaction will not confirm: - // superseded, unknown to the backend, or unconfirmed and no longer held by + // dropped is the wallet's judgement that this transaction will not confirm, + // whether superseded, unknown to the backend, or unconfirmed and no longer held by // the node. It is computed here because only the wallet can see all three. // // False whenever the wallet cannot tell, including on an older wallet that