Skip to content

Commit f7e5114

Browse files
ethicnologyethicnology
committed
fix(wallet): enforce frozen-coin invariant and RBF default at repository
Two hardenings surfaced by review of the TxBuilder reassignment fix: 1. D7 defense in depth: BDK's documented semantics let a manually added utxo (TxBuilder.addUtxos) override the unspendable filter — the inverse of the app-level D7 invariant that a frozen coin must never be spendable. Production was protected only by PrepareBitcoinSendUsecase stripping frozen coins from the selection; any future caller bypassing that usecase would not be. BitcoinWalletRepository.buildPsbt now strips selected ∩ unspendable itself, so the invariant holds at the repository boundary for every caller. The datasource keeps raw BDK semantics (thin wrapper); its regression test comment now documents that inversion explicitly. 2. replaceByFee null default flipped from false to true: harmless while the datasource's setExactSequence call discarded its result, but wrong once fixed — a caller omitting the flag would have started disabling RBF by default, diverging from the datasource default and BDK's default sequence (0xFFFFFFFD). Adds repository unit tests (mocktail) covering both: frozen selected coins never reach the datasource (exact outpoint match, txId alone not enough), and an omitted RBF flag forwards true while an explicit false is preserved. Both new tests fail on the previous repository code.
1 parent dabd2ca commit f7e5114

3 files changed

Lines changed: 262 additions & 10 deletions

File tree

‎lib/core/wallet/data/repositories/bitcoin_wallet_repository.dart‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,40 @@ class BitcoinWalletRepository {
5252
id: metadata.id,
5353
)
5454
as PublicBdkWalletModel;
55+
56+
// D7 defense in depth: a frozen coin must never be spendable. BDK's
57+
// documented semantics are the opposite — a manually added utxo
58+
// (TxBuilder.addUtxos) overrides the unspendable filter — and the
59+
// datasource deliberately preserves those raw semantics. Enforce the
60+
// app-level invariant here at the repository boundary, so a selected
61+
// coin that is also unspendable is stripped for ANY caller, not only
62+
// those going through PrepareBitcoinSendUsecase (which does its own
63+
// stripping against the frozen set).
64+
final unspendableKeys = {
65+
for (final outpoint in unspendable ?? const <({String txId, int vout})>[])
66+
'${outpoint.txId}:${outpoint.vout}',
67+
};
68+
final spendableSelected = selected
69+
?.where(
70+
(utxo) => !unspendableKeys.contains('${utxo.txId}:${utxo.vout}'),
71+
)
72+
.toList();
73+
5574
final psbt = await _bdkWallet.buildPsbt(
5675
wallet: wallet,
5776
address: address,
5877
amountSat: amountSat,
5978
networkFee: networkFee,
6079
drain: drain,
6180
unspendable: unspendable,
62-
selected: selected
81+
selected: spendableSelected
6382
?.map((utxo) => WalletUtxoMapper.fromEntity(utxo))
6483
.toList(),
65-
replaceByFee: replaceByFee ?? false,
84+
// Default to RBF-enabled, matching both the datasource default and
85+
// BDK's own default sequence (0xFFFFFFFD). `?? false` would disable
86+
// RBF for any caller omitting the flag — harmless while
87+
// setExactSequence's result was discarded, wrong now that it works.
88+
replaceByFee: replaceByFee ?? true,
6689
);
6790

6891
return psbt;

‎test/core_test/wallet/bdk_wallet_datasource_test.dart‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -199,14 +199,23 @@ void main() {
199199
final datasource = BdkWalletDatasource();
200200

201201
// The LARGE utxo (200k) is marked unspendable — i.e. excluded from
202-
// BDK's own automatic coin-selection pool, exactly like a
203-
// user-frozen coin (see the D7 guarantee in
204-
// PrepareBitcoinSendUsecase). It is ALSO the one manually `selected`
205-
// here, which per BDK's coin-control semantics is supposed to force
206-
// it into the transaction as a mandatory input regardless of the
207-
// unspendable flag (mirroring real coin control: a coin the user
208-
// explicitly picks must be spendable even if it's otherwise
209-
// frozen-by-default for automatic selection).
202+
// BDK's own automatic coin-selection pool. It is ALSO the one
203+
// manually `selected` here, which per BDK's documented semantics
204+
// forces it into the transaction as a mandatory input REGARDLESS of
205+
// the unspendable flag.
206+
//
207+
// NOTE: this selected-overrides-unspendable behavior is raw BDK
208+
// semantics that this datasource deliberately preserves as a thin
209+
// wrapper — it is the INVERSE of the app-level D7 invariant ("a
210+
// frozen coin must never be spendable"). D7 is enforced one layer
211+
// up: BitcoinWalletRepository.buildPsbt strips selected ∩
212+
// unspendable before calling down (see
213+
// bitcoin_wallet_repository_test.dart), and
214+
// PrepareBitcoinSendUsecase additionally strips frozen coins from
215+
// the selection. The combination is exploited here ONLY because it
216+
// is a deterministic discriminator for the addUtxos-reassignment
217+
// regression, with no dependency on BDK's coin-selection
218+
// heuristics.
210219
//
211220
// The only utxo left in the automatic pool is the SMALL one (30k),
212221
// which alone cannot cover the 150k send. This makes the two code

‎test/core_test/wallet/bitcoin_wallet_repository_test.dart‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
// Unit tests for the repository-boundary guarantees of
2+
// `BitcoinWalletRepository.buildPsbt`:
3+
//
4+
// 1. D7 defense in depth — "a frozen coin must never be spendable".
5+
// BDK's documented semantics are the opposite: a manually added utxo
6+
// (`TxBuilder.addUtxos`) overrides the `unspendable` filter, and
7+
// `BdkWalletDatasource` deliberately preserves those raw semantics
8+
// (see bdk_wallet_datasource_test.dart, which asserts them). The
9+
// repository must therefore strip any selected coin that is also in
10+
// the unspendable set before calling down, so the app-level invariant
11+
// holds for ANY caller — not only those going through
12+
// PrepareBitcoinSendUsecase's own frozen-set stripping.
13+
//
14+
// 2. RBF defaults to ENABLED when the caller omits the flag. The
15+
// previous `replaceByFee ?? false` was harmless while the datasource's
16+
// `setExactSequence` call discarded its result (a silent no-op), but
17+
// became wrong once that call was fixed: a null flag would have
18+
// started disabling RBF by default, diverging from both the
19+
// datasource's own default and BDK's default sequence (0xFFFFFFFD).
20+
import 'dart:typed_data';
21+
22+
import 'package:bb_mobile/core/fees/domain/fees_entity.dart';
23+
import 'package:bb_mobile/core/seed/data/datasources/seed_datasource.dart';
24+
import 'package:bb_mobile/core/storage/tables/wallet_metadata_table.dart';
25+
import 'package:bb_mobile/core/wallet/data/datasources/bdk_wallet_datasource.dart';
26+
import 'package:bb_mobile/core/wallet/data/datasources/wallet_metadata_datasource.dart';
27+
import 'package:bb_mobile/core/wallet/data/models/wallet_metadata_model.dart';
28+
import 'package:bb_mobile/core/wallet/data/models/wallet_model.dart';
29+
import 'package:bb_mobile/core/wallet/data/models/wallet_utxo_model.dart';
30+
import 'package:bb_mobile/core/wallet/data/repositories/bitcoin_wallet_repository.dart';
31+
import 'package:bb_mobile/core/wallet/domain/entities/wallet_utxo.dart';
32+
import 'package:flutter_test/flutter_test.dart';
33+
import 'package:mocktail/mocktail.dart';
34+
35+
class _MockWalletMetadataDatasource extends Mock
36+
implements WalletMetadataDatasource {}
37+
38+
class _MockSeedDatasource extends Mock implements SeedDatasource {}
39+
40+
class _MockBdkWalletDatasource extends Mock implements BdkWalletDatasource {}
41+
42+
// Encodes as a bitcoin testnet BIP84 origin so
43+
// WalletMetadataModelExtension.isBitcoin decodes to true.
44+
const _walletId = 'wpkh([73c5da0a/84h/1h/0h])';
45+
46+
WalletUtxo _utxo({required String txId, required int vout}) =>
47+
WalletUtxo.bitcoin(
48+
walletId: _walletId,
49+
txId: txId,
50+
vout: vout,
51+
scriptPubkey: Uint8List(0),
52+
amountSat: BigInt.from(100000),
53+
address: 'tb1-test-address',
54+
);
55+
56+
void main() {
57+
late _MockWalletMetadataDatasource metadataDatasource;
58+
late _MockBdkWalletDatasource bdkDatasource;
59+
late BitcoinWalletRepository repository;
60+
61+
const metadata = WalletMetadataModel(
62+
id: _walletId,
63+
masterFingerprint: '73c5da0a',
64+
xpubFingerprint: 'deadbeef',
65+
isEncryptedVaultTested: false,
66+
isPhysicalBackupTested: false,
67+
xpub: 'tpub-test',
68+
externalPublicDescriptor: 'wpkh(external)',
69+
internalPublicDescriptor: 'wpkh(internal)',
70+
signer: Signer.local,
71+
isDefault: true,
72+
);
73+
74+
setUpAll(() {
75+
registerFallbackValue(
76+
const WalletModel.publicBdk(
77+
id: _walletId,
78+
externalDescriptor: 'wpkh(external)',
79+
internalDescriptor: 'wpkh(internal)',
80+
isTestnet: true,
81+
),
82+
);
83+
registerFallbackValue(const NetworkFee.relativeSatPerKwu(1000));
84+
});
85+
86+
setUp(() {
87+
metadataDatasource = _MockWalletMetadataDatasource();
88+
bdkDatasource = _MockBdkWalletDatasource();
89+
repository = BitcoinWalletRepository(
90+
walletMetadataDatasource: metadataDatasource,
91+
seedDatasource: _MockSeedDatasource(),
92+
bdkWalletDatasource: bdkDatasource,
93+
);
94+
95+
when(
96+
() => metadataDatasource.fetch(_walletId),
97+
).thenAnswer((_) async => metadata);
98+
when(
99+
() => bdkDatasource.buildPsbt(
100+
wallet: any(named: 'wallet'),
101+
address: any(named: 'address'),
102+
amountSat: any(named: 'amountSat'),
103+
networkFee: any(named: 'networkFee'),
104+
drain: any(named: 'drain'),
105+
unspendable: any(named: 'unspendable'),
106+
selected: any(named: 'selected'),
107+
replaceByFee: any(named: 'replaceByFee'),
108+
),
109+
).thenAnswer((_) async => 'psbt');
110+
});
111+
112+
List<WalletUtxoModel>? capturedSelected() =>
113+
verify(
114+
() => bdkDatasource.buildPsbt(
115+
wallet: any(named: 'wallet'),
116+
address: any(named: 'address'),
117+
amountSat: any(named: 'amountSat'),
118+
networkFee: any(named: 'networkFee'),
119+
drain: any(named: 'drain'),
120+
unspendable: any(named: 'unspendable'),
121+
selected: captureAny(named: 'selected'),
122+
replaceByFee: any(named: 'replaceByFee'),
123+
),
124+
).captured.single
125+
as List<WalletUtxoModel>?;
126+
127+
bool capturedReplaceByFee() =>
128+
verify(
129+
() => bdkDatasource.buildPsbt(
130+
wallet: any(named: 'wallet'),
131+
address: any(named: 'address'),
132+
amountSat: any(named: 'amountSat'),
133+
networkFee: any(named: 'networkFee'),
134+
drain: any(named: 'drain'),
135+
unspendable: any(named: 'unspendable'),
136+
selected: any(named: 'selected'),
137+
replaceByFee: captureAny(named: 'replaceByFee'),
138+
),
139+
).captured.single
140+
as bool;
141+
142+
group('D7 defense in depth — selected ∩ unspendable is stripped', () {
143+
test(
144+
'a selected coin that is also unspendable never reaches the datasource',
145+
() async {
146+
await repository.buildPsbt(
147+
walletId: _walletId,
148+
address: 'tb1-destination',
149+
amountSat: 25000,
150+
networkFee: const NetworkFee.relativeSatPerKwu(1000),
151+
unspendable: const [(txId: 'tx-frozen', vout: 0)],
152+
selected: [
153+
_utxo(txId: 'tx-frozen', vout: 0), // must be stripped
154+
_utxo(txId: 'tx-free', vout: 1), // must pass through
155+
],
156+
);
157+
158+
final selected = capturedSelected();
159+
expect(selected, hasLength(1));
160+
expect(selected!.single.txId, 'tx-free');
161+
expect(selected.single.vout, 1);
162+
},
163+
);
164+
165+
test('same txId but different vout is NOT stripped', () async {
166+
await repository.buildPsbt(
167+
walletId: _walletId,
168+
address: 'tb1-destination',
169+
amountSat: 25000,
170+
networkFee: const NetworkFee.relativeSatPerKwu(1000),
171+
unspendable: const [(txId: 'tx-shared', vout: 0)],
172+
selected: [_utxo(txId: 'tx-shared', vout: 1)],
173+
);
174+
175+
final selected = capturedSelected();
176+
expect(selected, hasLength(1));
177+
expect(selected!.single.vout, 1);
178+
});
179+
180+
test('no unspendable set leaves the selection untouched', () async {
181+
await repository.buildPsbt(
182+
walletId: _walletId,
183+
address: 'tb1-destination',
184+
amountSat: 25000,
185+
networkFee: const NetworkFee.relativeSatPerKwu(1000),
186+
selected: [
187+
_utxo(txId: 'tx-a', vout: 0),
188+
_utxo(txId: 'tx-b', vout: 1),
189+
],
190+
);
191+
192+
expect(capturedSelected(), hasLength(2));
193+
});
194+
});
195+
196+
group('replaceByFee default', () {
197+
test('omitted flag defaults to RBF ENABLED (true)', () async {
198+
await repository.buildPsbt(
199+
walletId: _walletId,
200+
address: 'tb1-destination',
201+
amountSat: 25000,
202+
networkFee: const NetworkFee.relativeSatPerKwu(1000),
203+
);
204+
205+
expect(capturedReplaceByFee(), isTrue);
206+
});
207+
208+
test('explicit false is forwarded unchanged', () async {
209+
await repository.buildPsbt(
210+
walletId: _walletId,
211+
address: 'tb1-destination',
212+
amountSat: 25000,
213+
networkFee: const NetworkFee.relativeSatPerKwu(1000),
214+
replaceByFee: false,
215+
);
216+
217+
expect(capturedReplaceByFee(), isFalse);
218+
});
219+
});
220+
}

0 commit comments

Comments
 (0)