db2effaca4cf82bf806596d16f9797d3692e2da7 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0dd3517a71f3ddb38ed265a19693d4c wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be233b9cb6d5db886e8f1c1f058288fb5 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893fb3378b2ad39608fe254d33af6cce9f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca049ec7b054cb3047a167c7ce8dbd421 test: wallet: Split create and load (David Gumberg)
70dbc79b09acf7b1515532ee20c7533c938ffb70 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e011646266abb67b31027bc29e0ce1d08ad4 wallet: Create separate function for wallet load (David Gumberg)
bc69070416c62a88d8f4029280ec10d6f9ec8d20 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c69dafd6496ccb5aef4cd6d8898966b wallet: Remove redundant birth time update (David Gumberg)
b4a49cc7275efc16d4a4179ed34b50de5bb7367e wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618c53041e97ccfface3045a0642777e1 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d88a3e9a24ec2aa0b828b8cc533dde58 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf72815bdf2e176e790a4c63f745517c4bb4 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566ccaf9b81fe0684885972d9ee34afd3 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd723b9b3c84844bf999d6e08e163ef9d wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e4c4b5f3af0e453492077f4911e0132 scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4cf82bf806596d16f9797d3692e2da7
polespinasa:
approach ACK db2effaca4cf82bf806596d16f9797d3692e2da7
w0xlt:
reACK db2effaca4cf82bf806596d16f9797d3692e2da7
murchandamus:
ACK db2effaca4cf82bf806596d16f9797d3692e2da7
rkrux:
ACK db2effaca4cf82bf806596d16f9797d3692e2da7
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
b189a345574460f10165862eca9cc40ff3337dca test: add case where `TOTAL_TRIES` is exceeded yet solution remains (yancy)
Pull request description:
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently named `bnb_test`.
ACKs for top commit:
frankomosh:
Code review ACK b189a34
achow101:
ACK b189a345574460f10165862eca9cc40ff3337dca
murchandamus:
ACK b189a345574460f10165862eca9cc40ff3337dca
Tree-SHA512: 1df0b6e29ae219edbeed14cfa97f0ad4688d6bf97ed946719ba3c3b69e004f3dee82991578eb5aceb554914b70c5b68feff9e321283c1fc8bc0fedf08df2cb4c
On Windows, the `winnt.h` header defines `DELETE` as a macro for a
"Standard Access Right" bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before this enum definition,
the preprocessor expands `DELETE` into a numeric literal, causing
syntax errors.
Rename the enumerator to `DELETE_FLAG` to remove this fragility and
avoid the collision entirely.
fad042235bd6054d99d3f5a07529276b0138b484 refactor: Remove remaining std::bind, check via clang-tidy (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbose in a meaningless way
* Overriden args are silently accepted and dropped at runtime without a compile error. Same for accidental duplicates.
One could use `std::bind_front` similar to commit fa267551c4eaef577db92e248c4b6d31d0c8bc77. Though, I think the remaining cases are better off with lambdas.
So do that here, and enable the `modernize-avoid-bind` clang-tidy rule to avoid `std::bind` bugs in the future.
ACKs for top commit:
fjahr:
Code review ACK fad042235bd6054d99d3f5a07529276b0138b484
purpleKarrot:
Code review ACK fad042235bd6054d99d3f5a07529276b0138b484
Tree-SHA512: 38b17e26eda3ae47d84a8c34298309dc1eeb4ed434fda58b5803ef031c4c2edfb17222f5208f37af727bf340e32b37c7f81784f461d2b65fbc6227f3cd53eea4
1f60ca360eb83fa7982b1aac402eaaf477294197 wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande)
Pull request description:
`removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again.
The added test should fail on master.
ACKs for top commit:
achow101:
ACK 1f60ca360eb83fa7982b1aac402eaaf477294197
fjahr:
tACK 1f60ca360eb83fa7982b1aac402eaaf477294197
furszy:
utACK 1f60ca360eb83fa7982b1aac402eaaf477294197
vasild:
ACK 1f60ca360eb83fa7982b1aac402eaaf477294197
Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
3400db80401d65ba16b52e5055486c75cd1412ff doc: add missing param description to SRD (yancy)
Pull request description:
The params documentation is missing `change_fee` and the description is lacking recent changes.
ACKs for top commit:
murchandamus:
ACK 3400db80401d65ba16b52e5055486c75cd1412ff
brunoerg:
code review ACK 3400db80401d65ba16b52e5055486c75cd1412ff
Tree-SHA512: 8f6fac0d92873c5c9f77b19fbc0c6ecfb425b2a6b3d5f5ad69c82ed706b21cf4627e68c71acbc43661000e6063e8f8dbcd3b8ff60e3c727bdcba497d13ee1383
removeprunedfunds removes all entries from mapTxSpends for the
inputs of the pruned tx. However, this is incorrect, because there could be
multiple entries from conflicting transactions (that shouldn't be
removed as well). This could lead to the wallet creating invalid
transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because
the wallet trusts its internal accounting instead of calling
AddToSpends again.
fab2f3df4beb230eef63bdcf5042b6417c0012dc fuzz: Exclude too expensive inputs in descriptor_parse targets (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example, those will take several seconds (!) and the flamegraph shows that base58 encoding is the cause:
```
curl -fLO 'f5abf41608'
curl -fLO '78cb317546'
FUZZ=mocked_descriptor_parse ./bld-cmake/bin/fuzz ./f5abf41608addcef3538da61d8096c2050235032
FUZZ=descriptor_parse ./bld-cmake/bin/fuzz ./78cb3175467f53b467b949883ee6072e92dbb267
```
This will also break 32-bit fuzzing, see https://github.com/bitcoin/bitcoin/issues/34110#issuecomment-3759461248.
Fix all issues by checking for `HasTooLargeLeafSize`.
Sorry for creating several pull requests to fix this class of issue, but I think this one should be the last one. 😅
ACKs for top commit:
brunoerg:
reACK fab2f3df4beb230eef63bdcf5042b6417c0012dc
frankomosh:
re-ACK fab2f3df4beb230eef63bdcf5042b6417c0012dc
Tree-SHA512: 4ecf98ec4adc39f6e014370945fb1598cdd3ceba60f7209b00789ac1164b6d20e82a69d71f8419d9a40d57ee3fea36ef593c47fe48b584b6e8344c44f20a15c1
Aside from being more legible, changing the name of `CWallet::Create()`
also validates that every instance where a new wallet is `Create()`'ed
is handled in this branch.
-BEGIN VERIFY SCRIPT-
sed -i 's|\bCreate(|CreateNew(|g' src/wallet/wallet.cpp src/wallet/wallet.h src/wallet/test/util.cpp src/wallet/test/wallet_tests.cpp
-END VERIFY SCRIPT-
Writing the wallet's `CLIENT_VERSION` (which indicates the last version
to have touched a wallet) needs to be done on both wallet creation and
wallet loading.
The next commit removes the `PopulateWalletFromDatabase()` call from
wallet creation, but this behavior needs to be preserved, so this commit
factors setting `CLIENT_VERSION` out of `PopulateWalletFromDatabase()`
so that wallet creation can use it in the next commit.
Checking every SPKM in `CWallet::Create()` is not necessary, since the
only way presently for an SPKM to get added to `m_spk_managers` (the
return value of `GetAllScriptPubKeyMans()`) is through
`AddScriptPubKeyMan()`, which already invokes `MaybeUpdateBirthTime()`.
`m_keypool_size` must be set before `CWallet::PopulateWalletFromDB()`,
in order to move parsing of `-keypool` into `CWallet::LoadWalletArgs`,
`LoadWalletArgs()` invocation in `CWallet::Create()` must be moved
before `PopulateWalletFromDB()` is called.
This section is necessarily repetitive, makes CWallet::Create() easier
to read, and splits out functionality that will be useful when wallet
creation and loading are separated.
Review with `-color-moved=dimmed-zebra`
75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502632eb5c74df07e41af38582da6e884 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a9129da04e249e3938f643dc47bf060e7e wallet, rpc: Use HandleWalletError in createwallet (Ava Chow)
Pull request description:
We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet.
The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions.
Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only.
ACKs for top commit:
rkrux:
lgtm ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
polespinasa:
re ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
9c7e4771b13d4729fd20ea08b7e2e3209b134fff test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a685473712c1a822379effa42fd49223515 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f11b1b0f9e2a4e28124edbb1616af519 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61d417a567f152169f4ab21a491afb95 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e5e6f01c8810359b91041bc6865f1c7 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb6db39076a43b010c0c7898d50b8d92 descriptors: add HavePrivateKeys() (Novo)
Pull request description:
_TLDR:
Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possible to do so.
This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.
### Notes
- The new behaviour is applied to all descriptors including miniscript descriptors
- `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour https://github.com/bitcoin/bitcoin/pull/24361#discussion_r920801352
- Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.
### Relevant PRs
https://github.com/bitcoin/bitcoin/pull/24361https://github.com/bitcoin/bitcoin/pull/32186
### Testing
Functional tests were added to test the new behaviour
EDIT
**`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**
ACKs for top commit:
Sjors:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
achow101:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
Migration still needs to be able to restore unnamed wallets, so
allow_unnamed is added to RestoreWallet to explicitly allow that
behavior for migration only.
fa64d8424b8de49e219bffb842a33d484fb03212 refactor: Enforce readability-avoid-const-params-in-decls (MarcoFalke)
faf0c2d942c8de7868a3fd3afc7fc9ea700c91d4 refactor: Avoid copies by using const references or by move-construction (MarcoFalke)
Pull request description:
Top level `const` in declarations is problematic for many reasons:
* It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value.
* In constructors it prevents move construction.
* It can incorrectly imply some data is const, like in an imaginary example `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are *not* const.
* The compiler ignores the `const` from the declaration in the implementation.
* It isn't used consistently anyway, not even on the same line.
Fix some issues by:
* Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
* Using move-construction to avoid a copy
* Applying `readability-avoid-const-params-in-decls` via clang-tidy
ACKs for top commit:
l0rinc:
diff reACK fa64d8424b8de49e219bffb842a33d484fb03212
hebasto:
ACK fa64d8424b8de49e219bffb842a33d484fb03212, I have reviewed the code and it looks OK.
sedited:
ACK fa64d8424b8de49e219bffb842a33d484fb03212
Tree-SHA512: 293c000b4ebf8fdcc75259eb0283a2e4e7892c73facfb5c3182464d6cb6a868b7f4a6682d664426bf2edecd665cf839d790bef0bae43a8c3bf1ddfdd3d068d38
Modifying `fBroadcastTransactions` does not require any locks,
initialization of this wallet parameter can be relocated with all of the
other argument parsing in this function.
There are too many functions in CWallet with names like "Load" and
"Create", disambiguate what CWallet::LoadWallet does by renaming it to
PopulateWalletFromDB.
-BEGIN VERIFY SCRIPT-
sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp')
-END VERIFY SCRIPT-
d09a19fd41cb71a5d1c10297763e72bc551a7d3a test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad9517334a0848e6778243ddef1843f4 wallet: remove erroneous-on-reorg Assume() (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/34206
I'm not certain the test is worth keeping, but included it for now to show minimal example that crashes without fix. Can be removed.
ACKs for top commit:
bensig:
ACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
dergoegge:
utACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
Tree-SHA512: 7eac19e97be6db8e38af396c406066fdcec532332e685a38bb33f0a988701c7bd5a0967f51426737fd56972847b761a3d873495928ff66efa8512fb267a9622b
fa8d56f9f092fceab7dfb10533c4187e1b5fabfe fuzz: Reject too large descriptor leaf sizes in scriptpubkeyman target (MarcoFalke)
fabac1b3950e4bc9716f9b3c17b8f02952d6b974 fuzz: Reject some more "expensive" descriptors in the scriptpubkeyman target (MarcoFalke)
333333356f431d8ef318f685860d25ff99d4b457 fuzz: [refactor] Use std::span over FuzzBufferType in descriptor utils (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
Also, this may lead to problems, where the fuzz target can not be run at all on some platforms. See https://github.com/bitcoin/bitcoin/issues/34110.
Fixes https://github.com/bitcoin/bitcoin/issues/34110 by rejecting those useless and expensive inputs (via the third commit)
Can be tested by running the input and checking the time before and after the changes here:
```
curl -fLO '1cf91e0c6b'
FUZZ=scriptpubkeyman time ./bld-cmake/bin/fuzz ./1cf91e0c6bfff9dafcd4db5b0ba36b1e906f4cf5
```
Also, the second commit fixes https://github.com/bitcoin/bitcoin/issues/31066.
ACKs for top commit:
brunoerg:
code review ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
marcofleon:
ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
sipa:
ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
Tree-SHA512: e683cb89c3047358add438508c173f1cf647827bcadc3564ad42c757e4c99b8e9b777213fd38ebeb46f4c89a72363e0642f47435e20df3960eaeb5b8257dbd32
89372213048adf37a47427112a1ff836ee84c50e doc: add release notes for 29415 (Vasil Dimov)
582016fa5f013817db650bbba0a40d9195c18e2e test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e04896a86cad4e4b1bd9641afcc3a026c2 test: add functional test for private broadcast (Vasil Dimov)
818b780a05db126dcfe7efe12c46c84b5cfc3de6 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf13f7cb1d25a0db51409535cfe053b1 net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39db5a4a61d360a6a29c8853bb5c7ac0 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee742da11b0e3f6fc44c39f2b5b5929da1 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad35365af8545e8e766d75cf398968480 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48ce67bd517068550c78bfcab062a554 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2fd5a0033f37f4dda5d75bfc5f15c3b6 net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e240291077a7f440e9f57e61f30e158 net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8df6e8cab07965301382d2036ef2368 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944a672faccac5dd201c8d33a638d9091 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e21051bd469fda659fe7c495d5e264d221 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032c57714c62a4b31ff1fdd969751683f net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2b38b7a71c96b6222147f395e0c11d9 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b6ff1d0122319fc70e70a7e27e1a0ba init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a9a81b69a4751087918303163ba8869 log: introduce a new category for private broadcast (Vasil Dimov)
Pull request description:
_Parts of this PR are isolated in independent smaller PRs to ease review:_
* [x] _https://github.com/bitcoin/bitcoin/pull/29420_
* [x] _https://github.com/bitcoin/bitcoin/pull/33454_
* [x] _https://github.com/bitcoin/bitcoin/pull/33567_
* [x] _https://github.com/bitcoin/bitcoin/pull/33793_
---
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) to the P2P network only via Tor or I2P short-lived connections, or to IPv4/IPv6 peers but through the Tor network.
* Introduce a new connection type for private broadcast of transactions with the following properties:
* started whenever there are local transactions to be sent
* opened to Tor or I2P peers or IPv4/IPv6 via the Tor proxy
* opened regardless of max connections limits
* after handshake is completed one local transaction is pushed to the peer, `PING` is sent and after receiving `PONG` the connection is closed
* ignore all incoming messages after handshake is completed (except `PONG`)
* Broadcast transactions submitted via `sendrawtransaction` using this new mechanism, to a few peers. Keep doing this until we receive back this transaction from one of our ordinary peers (this takes about 1 second on mainnet).
* The transaction is stored in peerman and does not enter the mempool.
* Once we get an `INV` from one of our ordinary peers, then the normal flow executes: we request the transaction with `GETDATA`, receive it with a `TX` message, put it in our mempool and broadcast it to all our existent connections (as if we see it for the first time).
* After we receive the full transaction as a `TX` message, in reply to our `GETDATA` request, only then consider the transaction has propagated through the network and remove it from the storage in peerman, ending the private broadcast attempts.
The messages exchange should look like this:
```
tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient (dummy VERSION with no revealing data)
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe)
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe)
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe)
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- INV/TX --------> tx-recipient
tx-sender <--- GETDATA/TX ----< tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects
```
Whenever a new transaction is received from `sendrawtransaction` RPC, the node will send it to a few (`NUM_PRIVATE_BROADCAST_PER_TX`) recipients right away. If after some time we still have not heard anything about the transaction from the network, then it will be sent to 1 more peer (see `PeerManagerImpl::ReattemptPrivateBroadcast()`).
A few considerations:
* The short-lived private broadcast connections are very cheap and fast wrt network traffic. It is expected that some of those peers could blackhole the transaction. Just one honest/proper peer is enough for successful propagation.
* The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender.
---
<details>
<summary>How to test this?</summary>
Thank you, @stratospher and @andrewtoth!
Start `bitcoind` with `-privatebroadcast=1 -debug=privatebroadcast`.
Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
```bash
build/bin/bitcoin-cli -chain="signet" createwallet test
build/bin/bitcoin-cli -chain="signet" getnewaddress
```
Get a new address for the test transaction recipient:
```bash
build/bin/bitcoin-cli -chain="signet" loadwallet test
new_address=$(build/bin/bitcoin-cli -chain="signet" getnewaddress)
```
Create the transaction:
```bash
# Option 1: `createrawtransaction` and `signrawtransactionwithwallet`:
txid=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .txid')
vout=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .vout')
echo "txid: $txid"
echo "vout: $vout"
tx=$(build/bin/bitcoin-cli -chain="signet" createrawtransaction "[{\"txid\": \"$txid\", \"vout\": $vout}]" "[{\"$new_address\": 0.00001000}]" 0 false)
echo "tx: $tx"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" signrawtransactionwithwallet "$tx" | jq -r '.hex')
echo "signed_tx: $signed_tx"
# OR Option 2: `walletcreatefundedpsbt` and `walletprocesspsbt`:
# This makes it not have to worry about inputs and also automatically sends back change to the wallet.
# Start `bitcoind` with `-fallbackfee=0.00003000` for instance for 3 sat/vbyte fee.
psbt=$(build/bin/bitcoin-cli -chain="signet" walletcreatefundedpsbt "[]" "[{\"$new_address\": 0.00001000}]" | jq -r '.psbt')
echo "psbt: $psbt"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" walletprocesspsbt "$psbt" | jq -r '.hex')
echo "signed_tx: $signed_tx"
```
Finally, send the transaction:
```bash
raw_tx=$(build/bin/bitcoin-cli -chain="signet" sendrawtransaction "$signed_tx")
echo "raw_tx: $raw_tx"
```
</details>
---
<details>
<summary>High-level explanation of the commits</summary>
* New logging category and config option to enable private broadcast
* `log: introduce a new category for private broadcast`
* `init: introduce a new option to enable/disable private broadcast`
* Implement the private broadcast connection handling on the `CConnman` side:
* `net: introduce a new connection type for private broadcast`
* `net: implement opening PRIVATE_BROADCAST connections`
* Prepare `BroadcastTransaction()` for private broadcast requests:
* `net_processing: rename RelayTransaction to better describe what it does`
* `node: extend node::TxBroadcast with a 3rd option`
* `net_processing: store transactions for private broadcast in PeerManager`
* Implement the private broadcast connection handling on the `PeerManager` side:
* `net_processing: reorder the code that handles the VERSION message`
* `net_processing: move the debug log about receiving VERSION earlier`
* `net_processing: modernize PushNodeVersion()`
* `net_processing: move a debug check in VERACK processing earlier`
* `net_processing: handle ConnectionType::PRIVATE_BROADCAST connections`
* `net_processing: stop private broadcast of a transaction after round-trip`
* `net_processing: retry private broadcast`
* Engage the new functionality from `sendrawtransaction`:
* `rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON`
* New tests:
* `test: add functional test for private broadcast`
* `test: add unit test for the private broadcast storage`
</details>
---
**This PR would resolve the following issues:**
https://github.com/bitcoin/bitcoin/issues/3828 Clients leak IPs if they are recipients of a transaction
https://github.com/bitcoin/bitcoin/issues/14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
https://github.com/bitcoin/bitcoin/issues/19042 Tor-only transaction broadcast onlynet=onion alternative
https://github.com/bitcoin/bitcoin/issues/24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
https://github.com/bitcoin/bitcoin/issues/25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections
https://github.com/bitcoin/bitcoin/issues/32235 Tor: TX circuit isolation
**Issues that are related, but (maybe?) not to be resolved by this PR:**
https://github.com/bitcoin/bitcoin/issues/21876 Broadcast a transaction to specific nodes
https://github.com/bitcoin/bitcoin/issues/28636 new RPC: sendrawtransactiontopeer
---
Further extensions:
* Have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
* Have the `submitpackage` RPC do the private broadcast as well, [draft diff in the comment below](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733), thanks ismaelsadeeq!
* Add some stats via RPC, so that the user can better monitor what is going on during and after the broadcast. Currently this can be done via the debug log, but that is not convenient.
* Make the private broadcast storage, currently in peerman, persistent over node restarts.
* Add (optional) random delay before starting to broadcast the transaction in order to avoid correlating unrelated transactions based on the time when they were broadcast. Suggested independently of this PR [here](https://github.com/bitcoin/bitcoin/issues/30471).
* Consider periodically sending transactions that did not originate from the node as decoy, discussed [here](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2035414972).
* Consider waiting for peer's FEEFILTER message and if the transaction that was sent to the peer is below that threshold, then assume the peer is going to drop it. Then use this knowledge to retry more aggressively with another peer, instead of the current 10 min. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648).
* It may make sense to be able to override the default policy -- eg so submitrawtransaction can go straight to the mempool and relay, even if txs are normally privately relayed. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3427086681).
* As a side effect we have a new metric available - the time it takes for a transaction to reach a random node in the network (from the point of view of the private broadcast recipient the tx originator is a random node somewhere in the network). This can be useful for monitoring, unrelated to privacy characteristics of this feature.
---
_A previous incarnation of this can be found at https://github.com/bitcoin/bitcoin/pull/27509. It puts the transaction in the mempool and (tries to) hide it from the outside observers. This turned out to be too error prone or maybe even impossible._
ACKs for top commit:
l0rinc:
code review diff ACK 89372213048adf37a47427112a1ff836ee84c50e
andrewtoth:
ACK 89372213048adf37a47427112a1ff836ee84c50e
pinheadmz:
ACK 89372213048adf37a47427112a1ff836ee84c50e
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 89372213048adf37a47427112a1ff836ee84c50e
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
Pull request description:
As pointed out in https://github.com/bitcoin/bitcoin/pull/34156#issuecomment-3716728670, it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.
This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`.
ACKs for top commit:
waketraindev:
lgtm ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
polespinasa:
code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
rkrux:
Code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
willcl-ark:
ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
pablomartin4btc:
ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa8193a3e36f248dcc949e0cd41def191efac wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159c42008ac3b63d1c43d99d4f1316d2f1ef wallet: improve post-migration logging (furszy)
f011e0f0680a8c39988ae57dae57eb86e92dd449 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bde63286e19821a9e62cdff1712b6245dc7 test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28e80bf9af50b03a770b641fd309a801589 wallet: fix unnamed wallet migration failure (furszy)
4ed0693a3f2a427ef9e7ad016930ec29fa244995 wallet: RestoreWallet failure, erase only what was created (furszy)
Pull request description:
Minimal fix for #34128.
The issue occurs during the migration of a legacy unnamed wallet
(the legacy "default" wallet). When the migration fails, the cleanup
logic is triggered to roll back the state, which involves erasing the
newly created descriptor wallets directories. Normally, this only
affects the parent directories of named wallets, since they each
reside in their own directories. However, because the unnamed
wallet resides directly in the top-level `/wallets/` folder, this
logic accidentally deletes the main directory.
The fix ensures that only the wallet.dat file of the unnamed wallet
is touched and restored, preserving the wallet in BDB format and
leaving the main `/wallets/` directory intact.
#### Story Line:
#32273 fixed a different set of issues and, in doing so, uncovered
this one.
Before the mentioned PR, backups were stored in the same directory
as the wallet.dat file. On a migration failure, the backup was then
copied to the top-level `/wallets/` directory. For the unnamed legacy
wallet, the wallet directory is the `/wallets/` directory, so the source
and destination paths were identical. As a result, we threw early in the
`fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
were trying to copy the file onto itself. This caused the cleanup logic
to abort early on and never reach the removal line.
#### Testing Notes:
Cherry-pick the test commit on top of master and run it. You will
see the failure and realize the reason by reading the test code.
ACKs for top commit:
achow101:
ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
davidgumberg:
crACK b7c34d08dd
w0xlt:
ACK b7c34d08dd
willcl-ark:
ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
ToPrivateString() behaviour will be modified in the following commits.
In order to keep the scope of this PR limited to the RPC behaviour,
this commit updates wallet migration to use 'Descriptor::HavePrivateKeys()'
in place of 'Descriptor::ToPrivateString()' to determine watchonly descriptors.
A follow-up PR can be opened to update migration logic to exclude
descriptors with some private keys from the watchonly migration wallet.
Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used.
It returns `false` if there is at least one pubkey in the descriptor for which
the provider does not have a private key.
ToPrivateString() behaviour will change in the following commits to only
return `false` if no priv keys could be found for the pub keys in the descriptor.
HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining
if a descriptor is 'watchonly'.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
fafbc70d48e1fb42c4ed3da609e5f30c4cc39418 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke)
Pull request description:
It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer.
Can be tested via:
* Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero`
* Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"`
* Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server`
* Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1`
Before:
```
src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
Invalid parameter, version out of range(1~3)
```
After:
```
JSON integer out of range
ACKs for top commit:
bensig:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
achow101:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
rkrux:
utACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
theStack:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
