bf77fea3c1 test: fix incorrect named args in txpackage tests (fanquake)
Pull request description:
Final non-scripted-diff commit split from #24661.
Could be tested with: `./autogen.sh && ./configure CC=clang-12 CXX=clang++-12 && make clean && bear make -j9 && ( cd ./src/ && run-clang-tidy-12 -j9 )`.
Motivation:
> Incorrect named args are source of bugs, like https://github.com/bitcoin/bitcoin/pull/22979.
> To allow them being checked by clang-tidy, use a format it can understand.
ACKs for top commit:
ajtowns:
ACK bf77fea3c1
Tree-SHA512: a13bfb5fc70424b13fbeec7f164d7a0d3b72b27ebec11dfd4115b7782a0037f26e9349e06eef8a6b17b8f529e0c7f43ae37a9c252bde65706dd164704d207d5f
21520b9551 fuzz: add target for coinselection (Martin Zumsande)
Pull request description:
This adds a fuzz target for the coinselection algorithms by creating random `OutputGroup`s and running all three coin selection algorithms for them.
It does not fuzz higher-level wallet logic for selecting eligible coins (as in `SelectCoins()`), thought it probably would make sense to have a fuzz target for that too.
ACKs for top commit:
achow101:
ACK 21520b9551
vasild:
ACK 21520b9551
Tree-SHA512: c763003cf5ff5317f929d3d0b2f06fa739ae41dd642042d9a5c5c96e6cb9b349a6c7aeabc77bc2b846d12c8bcb60e07ee20a9f38539429c65723ab76aeee6b2e
0c12f0116c wallet: Postpone NotifyWalletLoaded() for encrypted wallets (Hennadii Stepanov)
aeee419c6a wallet, refactor: Add wallet::NotifyWalletLoaded() function (Hennadii Stepanov)
Pull request description:
Fixesbitcoin-core/gui#571.
`CWallet::Create()` notifies about wallet loading too early, that results the notification goes before `DescriptorScriptPubKeyMan`s were created and added to an encrypted wallet.
And `interfaces::Wallet::taprootEnabled()` in ecf692b466/src/qt/receivecoinsdialog.cpp (L100-L102) erroneously returns `false` for just created encrypted descriptor wallets.
ACKs for top commit:
Sjors:
utACK 0c12f0116c
achow101:
ACK 0c12f0116c
Tree-SHA512: 2694bacd12748cd5f6c95d9d3bf8bcf4502ee67fecd8d057f33236b72069c61401b08f49deb013fc71c3f1e51ae16bdfd827ddcbc2a083d7044589be7a78982e
71038a151e rpc: Fix documentation assertion for `getrawtransaction` (laanwj)
Pull request description:
When `getrawtransaction` is successfully used on a coinbase transaction, there is an assertion error. This is very unlikely but happens in the `interface_usdt_utxocache.py` test in #24358.
This does the following:
- Add missing "coinbase" documentation.
- Synchronize documentation between `getrawtransaction` and `decoderawtransaction`, the two users of `TxToUniv` that have detailed documentation. `decodepsbt` and `getblock` also uses it but fortunately elides this block.
- Change "vout[].amount" to `STR_AMOUNT` for consistency.
- Add maintainer comment to keep the two places synchronized. It might be possible to get smarter with deduplication, but there are some extra fields that prevent the obvious way.
ACKs for top commit:
jonatack:
ACK 71038a151e
Tree-SHA512: 962236130455d805190ff9a5c971e4e25c17db35614a90ce340264ec953b0ad7fb814eb33ae430b5073955a8a350f72bdd67ba93e35f9c70e5175b836a767a35
9563a645c2 refactor: add stdd:: includes to core_write (fanquake)
8b9efebb0a refactor: use named args when ScriptToUniv or TxToUniv are invoked (Michael Dietz)
22f25a6116 refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash (Michael Dietz)
828a094ecf refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function (Michael Dietz)
Pull request description:
I've cherry-picked some of the commits out of #22924, and made minor changes (like fixing named args).
ACKs for top commit:
MarcoFalke:
re-ACK 9563a645c2🕓
Tree-SHA512: 4f0e5b45c14cbf68b9e389bbe1211c125d95cbd3da5205b1cff6a4c44f15b15039ba2a5b25cd7e2580d9169404f1b7ff620d8a7e01f6112e3cb153ecfaef8916
2ef47ba6c5 util/check: stop using lambda for Assert/Assume (Anthony Towns)
7c9fe25c16 wallet: move Assert() check into constructor (Anthony Towns)
Pull request description:
Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.
Fixes#21596Fixes#24654
ACKs for top commit:
MarcoFalke:
ACK 2ef47ba6c5🚢
jonatack:
Tested re-ACK 2ef47ba6c5
Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
When `getrawtransaction` is successfully used on a coinbase transaction,
there is an assertion error. This is very unlikely but happens in the
test in #24358.
This does the following:
- Add missing "coinbase" documentation.
- Synchronize documentation between `getrawtransaction` and
`decoderawtransaction`, the two users of `TxToUniv` that have detailed
documentation. `decodepsbt` also uses it but fortunately elides this block.
- Change "vout[].amount" to `STR_AMOUNT` for consistency.
- Add maintainer comment to keep the two places synchronized. It might
be possible to get smarter with deduplication, but there are some
extra fields that prevent the obvious way.
bb84b7145b add tests for no recipient and using send_max while inputs are specified (ishaanam)
49090ec402 Add sendall RPC née sweep (Murch)
902793c777 Extract FinishTransaction from send() (Murch)
6d2208a3f6 Extract interpretation of fee estimation arguments (Murch)
a31d75e5fb Elaborate error messages for outdated options (Murch)
35ed094e4b Extract prevention of outdated option names (Murch)
Pull request description:
Add sendall RPC née sweep
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _given UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _given budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending a given set of
UTXOs such as paying the value from one or more specific UTXOs, emptying
a wallet, or burning dust, we realized that there are some cases in
which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual
computation of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific
subset of the wallet's UTXO pool (by default all of them) and assigns
the funds to one or more receivers. Receivers can either be specified
with a given amount or receive an equal share of the remaining
unassigned funds. At least one recipient must be provided without
assigned amount to collect the remainder. The `sendall` call will
never create change. The call has a `send_max` option that changes the
default behavior of spending all UTXOs ("no UTXO left behind"), to
maximizing the output amount of the transaction by skipping uneconomic
UTXOs. The `send_max` option is incompatible with providing a specific
set of inputs.
---
Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.
ACKs for top commit:
achow101:
re-ACK bb84b7145b
Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
f05a4cdf5a util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov)
Pull request description:
This PR fixes the current master (3297f5c11c) when running `bitcoin-qt` on Ubuntu 22.04 and quitting:
```
$ ./src/qt/bitcoin-qt -signet -sandbox=log-and-abort
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
ERROR: The syscall "inotify_rm_watch" (syscall number 255) is not allowed by the syscall sandbox in thread "main". Please report.
terminate called without an active exception
Aborted (core dumped)
```
Also see https://github.com/bitcoin/bitcoin/pull/24659#discussion_r835747166
ACKs for top commit:
fanquake:
ACK f05a4cdf5a - checked that qt is using this in it's filesystem watcher code.
Tree-SHA512: 9c7920a25422cd3a040bc1cbc487c12c3dc2b91358c3757f1030d6a1ff12c18c688a8e5b7466f683da88a5e4f5f15d442975660022d706e47021253c24c58f4a
d4ba2b2cbc compat: remove strnlen back-compat code (fanquake)
Pull request description:
This was needed for mingw (not mingw-w64), and some older versions of
macOS, which we no-longer support.
ACKs for top commit:
hebasto:
ACK d4ba2b2cbc
Tree-SHA512: d1beb9df58464feea3076091361d7d46e4a8901e347644a5fa6f24e052ca24ee0c7c0dd3f2a3d682b0204bf50430fa89eac62121691ea08af6dcf6b907bdec87
a40978dcbd [fuzz] Assert that Peer.m_tx_relay.m_relay_txs has been set correctly (John Newbery)
0bca5f2b46 [net processing] PushNodeVersion() takes a const Peer& (John Newbery)
21154ff927 net_processing: move CNode data access out of lock (John Newbery)
Pull request description:
#21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments.
ACKs for top commit:
MarcoFalke:
review ACK a40978dcbd
dergoegge:
ACK a40978dcbd
ajtowns:
ACK a40978dcbd
Tree-SHA512: 46624e275f918c5f32d0adab0766e9b3ef8ebdbc74a3c8886d8a2e2ff1079029dcc371b40ef0d787609e9c05219b7456f3e2dfe4fb0cb7bf23ef966769aef1a1
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
532c64a726 build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)
Pull request description:
Rebased #24415 with Luke's suggestion.
Fixes#24413.
ACKs for top commit:
hebasto:
ACK 532c64a726, tested on Mac mini (M1, 2020) + macOS Monterey 12.3 (21E230).
Tree-SHA512: 74f779695f6bbc45a2b7341a1402f747cc0d433d74825c7196cb9f156db0c0299895365f01665bd0bff12a8ebb5ea33a29b9a52f5eac0007ec35d1dca6544705
21db4eb3ff test: fix incorrect named args in wallet tests (fanquake)
8b0e776718 test: fix incorrect named args in coin_selection tests (fanquake)
6fc00f7331 bench: fix incorrect named args in coin_selection bench (fanquake)
Pull request description:
Should be one of the last changes split from #24661.
Motivation:
> Incorrect named args are source of bugs, like https://github.com/bitcoin/bitcoin/pull/22979.
> To allow them being checked by clang-tidy, use a format it can understand.
ACKs for top commit:
MarcoFalke:
Concept ACK 21db4eb3ff
Tree-SHA512: c29743a70f6118cf73dc37b56b30f45da55b7d7b3b8ed36859ad59f602c3e6692eb755e05d9a4dd17f05085bcd6cb5b8c4007090a76e4fbfb053f925322cf985
fc892c3a80 rpc: Fail to return undocumented or misdocumented JSON (MarcoFalke)
f4bc4a705a rpc: Add m_skip_type_check to RPCResult (MarcoFalke)
Pull request description:
This avoids documentation shortcomings such as the ones fixed in commit e7b6272b30, 138d55e6a0, 577bd51a4b, f8c84e047c, 0ee9a00f90, 13f41855c5, or faecb2ee0a
ACKs for top commit:
fanquake:
ACK fc892c3a80 - tested that this catches issue, i.e #24691:
Tree-SHA512: 9d0d7e6291bfc6f67541a4ff746d374ad8751fefcff6d103d8621c0298b190ab1d209ce96cfc3a0d4a6a5460a9f9bb790eb96027b16e5ff91f2512e40c92ca84
fac5a51c47 Move mempool RPCs to rpc/mempool (MarcoFalke)
fa0f666dd7 style: Add static keyword where possible in rpc/mempool (MarcoFalke)
Pull request description:
This moves the remaining mempool RPCs to `rpc/mempool`. Previously all mempool RPCs from the `blockchain` category have been moved. This patch moves the ones from the `rawtransactions` category.
In the future, as a follow-up to this refactoring patch, it could be considered whether a new `mempool` category should be introduced.
Beside a clearer code organization, this pull request should also reduce the compile time and space of the `rawtransactions.cpp` file.
ACKs for top commit:
promag:
Code review ACK fac5a51c47.
Tree-SHA512: 5578b894b68d0595869a9b03ed8dceebe3366f73dec5f090ccc36ff4002b1bc4d58af77546c2d71537c1be03694d9a28c4b1bfbb3569560997879293c5c0301e
7e22d80af3 addrman: fix incorrect named args (fanquake)
67f654ef61 doc: Document clang-tidy in dev notes (MarcoFalke)
Pull request description:
The documentation, and a single commit extracted from #24661.
Motivation:
> Incorrect named args are source of bugs, like https://github.com/bitcoin/bitcoin/pull/22979.
> To allow them being checked by clang-tidy, use a format it can understand.
ACKs for top commit:
glozow:
ACK 7e22d80af3
Tree-SHA512: 4037fcea59fdf583b171bce7ad350299fe5f9feb3c398413432168f3b9a185e51884d5b30e4b4ab9c6c5bb896c178cfaee1d78d5b4f0034cd70121c9ea4184b7
9053f64fcb [doc] release notes for random change target (glozow)
46f2fed6c5 [wallet] remove MIN_CHANGE (glozow)
a44236addd [wallet] randomly generate change targets (glozow)
1e52e6bd0a refactor coin selection for parameterizable change target (glozow)
Pull request description:
Closes#24458 - the wallet always chooses 1 million sats as its change target, making it easier to fingerprint transactions created by the Core wallet. Instead of using a fixed value, choose one randomly each time (within a range). Using 50ksat (around $20) as the lower bound and `min(1 million sat, 2 * average payment value)` as the upper bound.
RFC: If the payment is <25ksat, this doesn't work, so we're using the range (payment amount, 50ksat) instead.
ACKs for top commit:
achow101:
ACK 9053f64fcb
Xekyo:
reACK 9053f64fcb
Tree-SHA512: 45ce5d064697065549473347648e29935733f3deffc71a6ab995449431f60302d1f9911a0994dfdb960b48c48b5d8859f168b396ff2a62db67d535a7db041d35
da2bc865d6 [wallet] don't create long chains by default (glozow)
Pull request description:
Default mempool policy doesn't let you have chains longer than 25 transactions. This is locally configurable of course, but it's not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set `DEFAULT_WALLET_REJECT_LONG_CHAINS` to true.
Closes#9752Closes#10004
ACKs for top commit:
MarcoFalke:
re-ACK da2bc865d6 only change is fixing typos in tests 🎏
Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
3bb9627463 refactor: remove unused boost header include in bitcoin-util.cpp (Sebastian Falbesoner)
Pull request description:
This header was included since the introduction of bitcoin-util in
commit 13762bcc96, but boost was
actually never used (see `git log -S boost ./src/bitcoin-util.cpp`).
Cherry-picked out of #22953, which currently needs rebase. This commit could just be merged on its own.
ACKs for top commit:
MarcoFalke:
review ACK 3bb9627463
Tree-SHA512: 201ee1aa4d49074056654203db73a473479c2b92c49df8dbf8e35979f85178013c66540a665f0f6dc0a2efef88eb091e2b088bebff85d840033dffd8ae719349
This header was included since the introduction of bitcoin-util in
commit 13762bcc96, but boost was
actually never used (see `git log -S boost ./src/bitcoin-util.cpp`).
0346c26fca init: add missing cs_main lock (Anthony Towns)
Pull request description:
`BlockManager::m_block_tree_db` is protected by `cs_main`, so take the
`cs_main` lock while accessing it.
ACKs for top commit:
jonatack:
Code review ACK 0346c26fca
Tree-SHA512: d6dff0b2d58871c7fbb281558b59fa9ad26fa75b3ceca9232277fc49ab795325e5ac3d266db49e7bda33da6de0b014b1bdebdf2c2c4347d43e50c0433a2cf06c
The final step of send either produces a PSBT or the final transaction.
We extract these steps to a new helper function `FinishTransaction()` to
reuse them in `sendall`.
