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..4103b52eb 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,18 @@ }, "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\"." + }, + "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 6095a9226..810870e76 100644 --- a/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto +++ b/api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto @@ -223,6 +223,30 @@ 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; + // 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; + // 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 bb35ae939..b5a2bec54 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,34 @@ 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"` + // 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"` + // 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 } @@ -206,6 +230,27 @@ func (x *IsTransactionConfirmedResponse) GetBlocktime() int64 { return 0 } +func (x *IsTransactionConfirmedResponse) GetNotFound() bool { + if x != nil { + return x.NotFound + } + return false +} + +func (x *IsTransactionConfirmedResponse) GetReplacedBy() string { + if x != nil { + return x.ReplacedBy + } + 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"` @@ -3779,11 +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\"~\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\"B\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\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/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..22ba191c1 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,12 +213,17 @@ type mockedScanner struct { mu sync.Mutex // onchain-spend fixtures, read by the reconcile tests - 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( @@ -265,6 +276,23 @@ func (m *mockedScanner) RescanUtxos(_ context.Context, _ []wire.OutPoint) error return nil } +func (m *mockedScanner) IsTransactionDropped(_ context.Context, txid string) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + m.droppedCalls = append(m.droppedCalls, txid) + if m.droppedErr != nil { + return false, m.droppedErr + } + return m.dropped[txid], nil +} + +// 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.droppedCalls...) +} + 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..dbb252a88 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) IsTransactionDropped(ctx context.Context, txid string) (bool, error) { + return false, 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..e238155cf --- /dev/null +++ b/internal/core/application/unroll_retraction.go @@ -0,0 +1,117 @@ +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 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. +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 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 + } + + seen := make(map[domain.Outpoint]struct{}, len(candidates)) + stale := make([]domain.Outpoint, 0) + for _, vtxo := range candidates { + seen[vtxo.Outpoint] = struct{}{} + + 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. + 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, !dropped) >= 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 held by the node", 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 + } + // 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) + } + 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..eb333e46d --- /dev/null +++ b/internal/core/application/unroll_retraction_test.go @@ -0,0 +1,163 @@ +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 backend says the tx is gone for enough passes", func(t *testing.T) { + svc, vtxos := unrollService(t, &mockedScanner{ + unspent: map[domain.Outpoint]struct{}{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, + }) + 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 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) + } + + vtxos.AssertNotCalled(t, "UnmarkVtxosUnrolled", mock.Anything, mock.Anything) + }) + + // 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{}{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, + } + svc, vtxos := unrollService(t, scanner) + + for range unrollRetractionObservations - 1 { + svc.retractStaleUnrolls(ctx, candidates) + } + scanner.dropped[unrolledVtxoTxid] = false + svc.retractStaleUnrolls(ctx, candidates) + scanner.dropped[unrolledVtxoTxid] = true + 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{}{}, + droppedErr: errors.New("wallet down"), + }) + + for range unrollRetractionObservations * 2 { + svc.retractStaleUnrolls(ctx, candidates) + } + + 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{}{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, + }) + 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) + }) + + // 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{}{}, + dropped: map[string]bool{unrolledVtxoTxid: true}, + }} + + 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) + + svc.retractStaleUnrolls(ctx, nil) + + require.Empty(t, scanner.DroppedCalls()) + 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..6b3707c36 100644 --- a/internal/core/ports/scanner.go +++ b/internal/core/ports/scanner.go @@ -31,6 +31,17 @@ type BlockchainScanner interface { IsTransactionConfirmed( 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. + // + // 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/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..216e42cd3 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) IsTransactionDropped(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..185199898 100644 --- a/internal/infrastructure/wallet/wallet_client.go +++ b/internal/infrastructure/wallet/wallet_client.go @@ -442,6 +442,22 @@ func castSpends(spends []*arkwalletv1.SpendInfo) []ports.Spend { return out } +// 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( + ctx, &arkwalletv1.IsTransactionConfirmedRequest{Txid: txid}, + ) + if err != nil { + return false, err + } + return resp.GetDropped(), nil +} + func (w *walletDaemonClient) IsTransactionConfirmed( ctx context.Context, txid string, ) (bool, *ports.BlockTimestamp, error) { diff --git a/internal/infrastructure/wallet/wallet_client_test.go b/internal/infrastructure/wallet/wallet_client_test.go index 90d6dabde..f20d599cd 100644 --- a/internal/infrastructure/wallet/wallet_client_test.go +++ b/internal/infrastructure/wallet/wallet_client_test.go @@ -294,6 +294,104 @@ func TestNotificationStreamIsShared(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" + + // 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, + }, + }} + + 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, Dropped: true}, + }} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.NoError(t, err) + require.True(t, dropped) + }) + + t.Run("an unconfirmed transaction is not dropped", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{ + resp: &arkwalletv1.IsTransactionConfirmedResponse{Confirmed: false}, + }} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.NoError(t, err) + require.False(t, dropped, "waiting in the mempool is not the same as gone") + }) + + // 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{}, + }} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.NoError(t, err) + require.False(t, dropped) + }) + + t.Run("a transport failure is reported, not read as dropped", func(t *testing.T) { + w := &walletDaemonClient{client: &confirmFakeClient{err: errors.New("wallet down")}} + + dropped, err := w.IsTransactionDropped(t.Context(), txid) + + require.Error(t, err) + require.False(t, dropped) + }) +} + +// 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/core/application/scanner/service.go b/pkg/arkd-wallet/core/application/scanner/service.go index c7b35d945..f3e54d8cc 100644 --- a/pkg/arkd-wallet/core/application/scanner/service.go +++ b/pkg/arkd-wallet/core/application/scanner/service.go @@ -276,6 +276,22 @@ 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 { + 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..1ece32dba 100644 --- a/pkg/arkd-wallet/core/application/types.go +++ b/pkg/arkd-wallet/core/application/types.go @@ -65,6 +65,13 @@ 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) + // 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 3fc6044c3..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 @@ -195,6 +199,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 } @@ -1053,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 af88e3eb3..ea6631d0c 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 { @@ -82,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 113e553b5..4b03d2902 100644 --- a/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go @@ -350,14 +350,48 @@ func (h *walletHandler) IsTransactionConfirmed( Confirmed: false, Blocknumber: 0, Blocktime: 0, + NotFound: true, + Dropped: true, }, nil } return nil, err } + // 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 { + log.WithError(err).Warnf( + "failed to check whether tx %s was replaced", req.GetTxid(), + ) + } 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{ Confirmed: confirmed, 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 new file mode 100644 index 000000000..2c85294a2 --- /dev/null +++ b/pkg/arkd-wallet/interface/grpc/handlers/wallet_handler_test.go @@ -0,0 +1,186 @@ +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{inMempool: true}} + + 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. + // 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()) + }) + + // 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")}} + + _, 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 + 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) { + if s.err != nil { + return false, 0, 0, s.err + } + return s.confirmed, s.blockHeight, s.blockTime, nil +}