c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4 fuzz: implement targets for PCP and NAT-PMP port mapping requests (Antoine Poinsot)
1695c8ab5bd3ea2dd0a065bcb8162a973dede7fe fuzz: in FuzzedSock::GetSockName(), return a random-length name (Antoine Poinsot)
0d472c19533a0c13ea8b79e84bcff49230179519 fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName (Antoine Poinsot)
39b7e2b5905255645264bc332b934b62441e55b9 fuzz: add steady clock mocking to FuzzedSock (Antoine Poinsot)
6fe1c35c05b353f5cc3f3811fdf46e3b220096e4 pcp: make NAT-PMP error codes uint16_t (Antoine Poinsot)
01906ce912e945c967316f829c1356f8ff38745f pcp: make the ToString method const (Antoine Poinsot)
Pull request description:
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.
Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.
We reuse the existing `FuzzedSock`, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.
ACKs for top commit:
laanwj:
re-ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4
marcofleon:
ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4
dergoegge:
utACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4
Tree-SHA512: 24cd4d958a0999946a0c3d164a242fc3f0a0b66770630252b881423ad0065d29fdaab765014d193b705d3eff397f201d51a88a3ca80c63fd3867745e6f21bb2b
7267ed051820b9227856143bdf767ae94a5be1d8 qt: Update `src/qt/locale/bitcoin_en.xlf` after string freeze (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](864386a744/doc/release-process.md) and implements the ["Translation string freeze"](https://github.com/bitcoin/bitcoin/issues/31029) step.
Steps to reproduce the diff on Ubuntu:
```
$ cmake --preset dev-mode -DWITH_USDT=OFF -DWITH_MULTIPROCESS=OFF
$ cmake --build build_dev_mode --target translate
```
At the moment, the multiprocess-specific code does not introduce any new translatable strings. Therefore, there is no need to build depends with `MULTIPROCESS=1` to review this PR.
ACKs for top commit:
stickies-v:
ACK 7267ed051820b9227856143bdf767ae94a5be1d8 - I get the same results when building the `translate` target.
pablomartin4btc:
tACK 7267ed051820b9227856143bdf767ae94a5be1d8
Tree-SHA512: dc3641d3288c00cb7802714680508de517e56c615716e52181555634ad489fbed676229063995170edb8efeaa4e900ef2d3d5a0f1b8ce7cec143ef364c96e1c0
d871d778251c35fd55d420ecf5c278f3edfea227 test: Remove non-portable IPv6 test (Hennadii Stepanov)
Pull request description:
On Illumos-based systems, such as OpenIndiana and OmniOS, the assumption that "the default zone ID of 0 can be omitted for the default scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)` returns the `EAI_NONAME` error instead of resolving to "fe80::1".
See: https://www.illumos.org/man/3SOCKET/getaddrinfo.
This PR removes the problematic code introduced in https://github.com/bitcoin/bitcoin/pull/19951.
ACKs for top commit:
fanquake:
ACK d871d778251c35fd55d420ecf5c278f3edfea227
Tree-SHA512: 2ef5c22f826d16661deb1d6d005cbda64179e8b83be43d3d6ac51caff02187cf224355c9da144ff110a6ae2cb68f0338ea9b62af8e0f9f1014a518cf9dad6ab5
ca6aa0b9bee3fdf355b7154a9a686a80977f2a02 doc: loadwallet loads from relative walletdir (am-sq)
Pull request description:
## Why this change?
https://github.com/bitcoin/bitcoin/issues/30269 describes a need for documentation improvement with the `loadwallet` RPC. Namely, [some users have found](https://bitcoin.stackexchange.com/questions/123331/how-do-you-load-a-regtest-wallet) the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.
The default wallet directory, depending on the machine OS, has the base directory defined here: 9c5cdf07f3/src/common/args.cpp (L699) which is then appended with `/wallets`. So for example, for MacOS, it would be `~/Library/Application Support/Bitcoin/wallets`.
## The changes implemented
1. Change the help text to indicate that the filename (or directory) passed in to `loadwallet` is relative to the base wallet directory
2. Adds additional examples to the help page showing how to fetch a wallet within a subdirectory of the base data directory for wallets, or from an absolute path
ACKs for top commit:
achow101:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
maflcko:
lgtm ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
rkrux:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
jonatack:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
Tree-SHA512: 123ae118c79ee1843ed65861e7a008658a53e47d4d14f2b7612561bba1b1dbdb6744f9aaac1587aac231c62d0c1804de848a6d732f1382788b437d9fe6474012
c4c5cf174883cb53256e869f0d1673e29576a97c cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree (Hennadii Stepanov)
eb540a262953baf9ed920f98efc4044a353b278a cmake: Remove `core_sanitizer_{cxx,linker}_flags` helper variables (Hennadii Stepanov)
Pull request description:
On the master branch @ 70e20ea024ce4f39abc4022e1ba19d5a6db2a207, the `APPEND_CPPFLAGS`, `APPEND_CFLAGS` and `APPEND_LDFLAGS` are not correctly applied when building C code in the `secp256k1` subtree, as intended.
This behaviour occurs due to two issues:
1. The command here: 70e20ea024/src/CMakeLists.txt (L77)
does not affect the code in `add_subdirectory(secp256k1)` above it.
2. `APPEND_LDFLAGS` is not passed to the subtree's build system at all.
This PR fixes both issues.
Additionally, the helper variables `core_sanitizer_cxx_flags` and `core_sanitizer_linker_flags` have been removed.
ACKs for top commit:
theuni:
utACK c4c5cf174883cb53256e869f0d1673e29576a97c.
TheCharlatan:
ACK c4c5cf174883cb53256e869f0d1673e29576a97c
Tree-SHA512: 707acfa623f0436e34e9e6ba8ce2979e0fde5e196e2242fd1cde4c50f433938549781193d8a06419a0866bbe6d69d76f8383d973afbd87d944407963b318c5c9
ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03 Revert "cmake: Ensure generated sources are up to date for `translate` target" (Hennadii Stepanov)
03b3166aac5acbf3eec95681c0ab98c4f94ab177 cmake: Exclude generated sources from translation (Hennadii Stepanov)
Pull request description:
This PR fixes an error encountered when building the `translate` target:
```
$ gmake -j $(nproc) -C depends MULTIPROCESS=1
$ cmake -G "Unix Makefiles" --preset dev-mode --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DWITH_USDT=OFF
$ cmake --build build_dev_mode -t translate
gmake[3]: *** No rule to make target 'src/test/ipc_test.capnp.c++', needed by 'src/qt/CMakeFiles/translate'. Stop.
gmake[2]: *** [CMakeFiles/Makefile2:1646: src/qt/CMakeFiles/translate.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1653: src/qt/CMakeFiles/translate.dir/rule] Error 2
gmake: *** [Makefile:699: translate] Error 2
```
The previous [attempt](864386a744) to address this issue worked only with Ninja generators and has been reverted.
Essentially, this PR modifies the `translate` target so that it ignores generated sources rather than attempting to update them.
At present, multiprocess-specific sources do not contain any translatable strings. Nonetheless, it is prudent to maintain a general approach.
ACKs for top commit:
TheCharlatan:
ACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03
pablomartin4btc:
tACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03
Tree-SHA512: 6471498a33b145e073f76bd007591b0449e5d520f141c3e3afeca02a09c160fd0f572ec7172dd84703cdc2a1175ad8f3c91e8b0bf705d671338d760786765f56
09b150bb8adae00854f02ece69fc6ef222fb07d9 In `InitHardwareRand`, do trail test for `RNDRRS` by `VerifyRNDRRS` (Eval EXEC)
Pull request description:
This PR want to fix#31817 by added a maximum retry limit (`max_retries`) to the `GetRNDRRS` function to prevent it from entering an infinite loop if the hardware random number generator fails to return a valid random number. This change improves stability and ensures that the function terminates after a predefined number of retries.
ACKs for top commit:
achow101:
ACK 09b150bb8adae00854f02ece69fc6ef222fb07d9
sipa:
utACK 09b150bb8adae00854f02ece69fc6ef222fb07d9
Tree-SHA512: 5626b6b182a55d344a3aba11b782259ecc6bbec513771d50077874c5f70934750e68add8f63aa0bf69c6b7b313112940a85508af5447622c703cc5e92439ab4a
e4dd5a351bde88a94326824945f4c8b1e4c15df2 test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9bf7109df78e46936e5a649c70703386 wallet: abandon inactive coinbase tx and their descendants during startup (furszy)
Pull request description:
Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).
This affects balance calculation as well as mempool rebroadcast (descendants shouldn't be relayed).
Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.
ACKs for top commit:
achow101:
ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
rkrux:
tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
mzumsande:
Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a
Improves the documentation of help output for loadwallet
to clarify that filename is relative to the default
wallet directory. Adds examples that get a wallet from
sub-directories.
`OBJECT` libraries have historically exhibited poor support for various
features, both in the past and now. For example, see one of the latest
issues:
- https://gitlab.kitware.com/cmake/cmake/-/issues/24058
Furthermore, CMake maintainers have acknowledged:
> In general, however, where there is a choice, static libraries will
> typically be the more convenient choice in CMake projects.
This change:
1. Converts the `bitcoin_clientversion` library from an `OBJECT` library
to a `STATIC` library.
2. Removes an obsolete workaround.
9cf746d6631739df9c9f80accd5812b319efcfec cmake: add optional source files to crc32c directly (Daniel Pfeifer)
9c7823c5b531ac1bbe5bdb9f2731bfae06cf695a cmake: add optional source files to bitcoin_crypto directly (Daniel Pfeifer)
Pull request description:
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.
fixes: #31697
ACKs for top commit:
s373nZ:
ACK 9cf746d6631739df9c9f80accd5812b319efcfec
hebasto:
re-ACK 9cf746d6631739df9c9f80accd5812b319efcfec.
TheCharlatan:
ACK 9cf746d6631739df9c9f80accd5812b319efcfec
Tree-SHA512: 04b468ccbd284d63fc83b382177bb8183b325369835c3b92e555e159955c73d71712a63a2e556f8da68a1232ac07d3845e11f1057c50666843db91db98fca979
a85e8c0e6158fad2408bda5cb1e36da707eb081b doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa17829c3f8ae4da739f526531c91f3ed87 doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a11b57cb5af0e8903b50927723fbb3fcd6 refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c3c65e47fc05545d9b9c919be945851d51 test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920ec98fc7d9f77abfd08fea17211b9ca7099 refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d05668922a28e4e2c78dab2d4737433cd52d6302 refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca53a05e7d4735a2207d1b200e1dcddc534 Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d4c1e7f310c6ba3b58373bc30679b491df Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19f863737518944950fc73f97d9c1399a46 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f433fc3f753a20833aebe54692cdfe5ed75 Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab3508064cd3135e1a356c884ae1269cda5250 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389917a8d744f1b1ada4556d3d4fe63c310e Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c70f7472d39e45d189df6c0cf6b676b761 Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899bc209921fb4bde02840359c3253663766 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66fd91c303d04004d861ecad183ff177823 Fix nonsensical -noseednode behavior (Ryan Ofsky)
Pull request description:
The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.
Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.
Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.
ACKs for top commit:
achow101:
ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
danielabrozzoni:
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
hodlinator:
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
f919d919eb8425ef2bb25aa0ebf61c90ab9b07fa fuzz: Add fuzzing for max_ret_len in DecodeBase58/DecodeBase58Check (Lőrinc)
635bc58f46b158cd6f77fda80001c2bccd5f83b0 test: Fuzz Base32/Base58/Base64 roundtrip conversions (Lőrinc)
5dd3a0d8a899e4c7263d5b999135f4d7584e1244 test: Extend base58_encode_decode.json with edge cases (Lőrinc)
ae40cf1a8e16462a8b9dfd076d440bc8ec796c2b test: Add padding tests for Base32/Base64 (Lőrinc)
Pull request description:
Added fuzzed roundtrips for `base[32|58|64]` encoding to make sure encoding/decoding are symmetric.
Note that if we omit the padding in `EncodeBase32` we won't be able to decode it with `DecodeBase32`.
Added dedicated padding tests to cover failure behavior
Also moved over the Base58 json test edge cases from https://github.com/bitcoin/bitcoin/pull/30035
ACKs for top commit:
hodlinator:
re-ACK f919d919eb8425ef2bb25aa0ebf61c90ab9b07fa
achow101:
ACK f919d919eb8425ef2bb25aa0ebf61c90ab9b07fa
Tree-SHA512: 6a6c63d0a659b70d42aad7a8f37ce6e372756e2c88c84e7be5c1ff1f2a7c58860ed7113acbe1a9658a7d19deb91f0abe2ec527ed660335845cd1e0a9380b4295
4818da809f0e300016390dd41e02c6067ffa008f wallet: fix rescanning inconsistency (Martin Zumsande)
Pull request description:
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.
This means that if rescanning was triggered with `cs_wallet` permanently held (`AttachChain`), additional blocks that were connected during the rescan will only be processed with the pending `blockConnected` notifications after the lock is released.
If rescanning without a permanent `cs_wallet` lock (`RescanFromTime`), additional blocks that were connected during the rescan can be re-processed here because `m_last_processed_block` was already updated by `blockConnected`.
Fixes#31474
ACKs for top commit:
psgreco:
Not that it matters much, but UTACK 4818da809f0e300016390dd41e02c6067ffa008f
achow101:
ACK 4818da809f0e300016390dd41e02c6067ffa008f
furszy:
utACK 4818da809f0e300016390dd41e02c6067ffa008f
Tree-SHA512: 8e7dbc9e00019aef4f80a11776f3089cd671e0eadd3c548cc6267b5c722433f80339a9b2b338ff9b611863de75ed0a817a845e1668e729b71af70c9038b075af
c9136ca90605bbe29f005f538b92ff96ca360a13 validation: fix issue with an interrupted -reindex (Martin Zumsande)
a2675897e2a499aacbd0183fdccf1401953e8de5 validation: Don't loop over all chainstates in LoadExternalBlock (Martin Zumsande)
Pull request description:
If a user interrupts a reindex while it is iterating over the block files, it will continue to reindex with the next node start (if the `-reindex` arg is dropped, otherwise it will start reindexing from scratch).
However, due to an early call to `ActivateBestChainState()` that only exists to connect the genesis block during
the original `-reindex`, it wil start connecting blocks immediately before having iterated through all block files.
Because later headers above the minchainwork threshold won't be loaded in this case, `-assumevalid` will not
be applied and the process is much slower due to script validation being done.
Fix this by only calling `ActivateBestChainState()` here if Genesis is not connected yet (equivalent to `ActiveHeight() == -1`).
Also simplify this spot by only doing this for the active chainstate instead of looping over all chainstates (first commit).
This issue was discussed in the thread below https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856824817, the impact on assumevalid was found by l0rinc.
The fix can be tested by manually aborting a `-reindex` e.g. on signet and observing in the debug log the order in which blockfiles are indexed / blocks are connected with this branch vs master.
ACKs for top commit:
achow101:
ACK c9136ca90605bbe29f005f538b92ff96ca360a13
ryanofsky:
Code review ACK c9136ca90605bbe29f005f538b92ff96ca360a13. Only comments changed since last review. Appreciate the new comments, I think they make a little clearer what things code is trying to do and what things are just side-effects.
TheCharlatan:
Re-ACK c9136ca90605bbe29f005f538b92ff96ca360a13
Tree-SHA512: 6f34abc317ad7e605ccc0c2f4615e4ea6978223d207f80f768f39cc135a9ac0adf31681fadfa2aed45324a5d27a4f68c5e118ee7eec18ca5c40ef177caa9cc47
fa8e0956c23789fb819ad1b31377711df091ec1b rpc: Remove deprecated dummy alias for listtransactions::label (MarcoFalke)
Pull request description:
The RPC arg is not a dummy, but a label, so offering an undocumented alias is inconsistent with all other label interfaces and confusing at best, if not entirely unused.
Fix it by removing the deprecated alias.
This pull is a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can't imagine anyone doing this, because in all other places where label args are accepted, they are called `label`. If someone really didn't use `label` here as named arg, it would be trivial and less confusing for them to fix it up.
ACKs for top commit:
achow101:
ACK fa8e0956c23789fb819ad1b31377711df091ec1b
rkrux:
tACK fa8e0956c23789fb819ad1b31377711df091ec1b
ryanofsky:
Code review ACK fa8e0956c23789fb819ad1b31377711df091ec1b
Tree-SHA512: 0d0f3f53237ff9fac8c065b7d0a4245f5ff86efa427dbeeca711765494b7315a9d72b44751d346c76422847daf3d7ff90dbccb5ba200b089fb96128bd95da9f0
cd4bfaee103591f212b88afd17969c16c1056eb6 net: reduce CAddress usage to CService or CNetAddr (Vasil Dimov)
Pull request description:
Using `CAddress` when only `CService` or `CNetAddr` is needed is excessive and confusing. Fix those occurrences to use the class they need:
* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`, thus change its argument.
* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a dummy `CAddress` from `CService`, so use `CService` instead.
* `GetBindAddress()` only needs to return `CService`.
* `CNode::addrBind` only needs to be `CService`.
ACKs for top commit:
Sjors:
ACK cd4bfaee10
achow101:
ACK cd4bfaee103591f212b88afd17969c16c1056eb6
hodlinator:
ACK cd4bfaee103591f212b88afd17969c16c1056eb6
laanwj:
Code review ACK cd4bfaee103591f212b88afd17969c16c1056eb6
Tree-SHA512: 0b41c1519784eeeaf9926c6a4d24f583b90c3376741f37a3199a3808b0dd6d143d3f929bd7c06f87b031f4fc1c2bd7a6dfc7d715ec1f79bf36b862c00fd67085
9b033bebb18dfd609c02736292f37cc6589fcc8d cmake: rename Kernel component to bitcoinkernel for consistency (Cory Fields)
2e0c92558e96b43351312d8fd403a20b9795acd4 cmake: add and use install_binary_component (Cory Fields)
0264c5d86c74c25407f0a07326d6f8722665fe74 cmake: use per-target components for bitcoin-qt and bitcoin-gui (Cory Fields)
fb0546b1c5ebb858605bef4c9fa001782e0ab213 ci: don't try to install for a fuzz build (Cory Fields)
Pull request description:
This makes it possible to build/install only the desired binaries regardless of the configuration.
For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed.
Additionally it fixes#31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).
Also fixes#31745.
Alternative to #31765 which is (imo) more correct/thorough.
Can be tested using (for ex):
```bash
$ cmake -B build
$ cmake --build build -t bitcoind -t bitcoin-cli
$ cmake --install build --component bitcoind
$ cmake --install build --component bitcoin-cli
```
ACKs for top commit:
hebasto:
ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d.
TheCharlatan:
Re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
stickies-v:
re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
Tree-SHA512: fd4818e76f190dbeafbf0c246b466f829771902c9d6d7111ed917093b811c8a5536a4a45e20708f73e7f581d6cb77c8e61cfa69e065788dcf0886792f553a355
70398ae05bc36a2788e87f67ae06962f43fe35a6 mapport: make ProcessPCP void (Antoine Poinsot)
9e6cba29882e72eabbe68307b2040d7919143b6f mapport: remove unnecessary 'g_mapport_enabled' (Antoine Poinsot)
8fb45fcda07aa1fa6d8420cbfe8e6b960b2df8fb mapport: remove unnecessary 'g_mapport_current' variable (Antoine Poinsot)
1b223cb19b4e04f9ab5fa0750e804fe44eba012f mapport: merge DispatchMapPort into StartMapPort (Antoine Poinsot)
9bd936fa34a2d8a1b0f71836456d7933fbf131a6 mapport: drop unnecessary function (Antoine Poinsot)
2a6536ceda7a28c5605005032bba32ffd3df082d mapport: rename 'use_pcp' to 'enable' (Antoine Poinsot)
c4e82b854cdd3702d20a6afc49ef26bda9d2ba24 mapport: make 'enabled' and 'current' bool (Antoine Poinsot)
Pull request description:
Followup to #31130, this does a couple cleanups to `src/mapport.*` to clarify the logic now that there is a single protocol option for port mapping.
ACKs for top commit:
laanwj:
Code review ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
TheCharlatan:
ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
Tree-SHA512: d9a3ab4fcd59a7cf4872415c40cc7ac3a98dfc5aa25e195d4df880bb588bac286c30c3471e9d9499de379a75f45dcd0a82019eba3cb9f342004ae1482d0ba075
af76664b12d8611b606a7e755a103a20542ee539 test: Test migration of a solvable script with no privkeys (Ava Chow)
17f01b0795e1dfa264fb42de65339bd18abb7f97 test: Test migration of taproot output scripts (Ava Chow)
1eb9a2a39fdb20262a5afe21d8feada9bdcbbd18 test: Test migration of miniscript in legacy wallets (Ava Chow)
e8c3efc7d8f06210dfef7c1a47bad6315714f4cd wallet migration: Determine Solvables with CanProvide (Ava Chow)
fa1b7cd6e2cfd581679d1fb6bc0fabcd6ee6e70a migration: Skip descriptors which do not parse (Ava Chow)
440ea1ab6393638a49a2ba6daefe0b29778bea7e legacy spkm: use IsMine() to extract watched output scripts (Ava Chow)
b777e84cd70838fee5e3624a182632e04cc1d24c legacy spkm: Move CanProvide to LegacyDataSPKM (Ava Chow)
b1ab927bbf2a620ad984910c1d8317ec9bb33297 tests: Test migration of additional P2WSH scripts (Ava Chow)
c39b3cfcd1bc5002aa06d1b79c948ce94f3ec8dc test: Extra verification that migratewallet migrates (Ava Chow)
Pull request description:
The legacy wallet `IsMine()` is essentially a black box that would tell us whether the wallet is watching an output script. In order to migrate legacy wallets to descriptor wallets, we need to be able to compute all of the output scripts that a legacy wallet would watch. The original approach for this was to understand `IsMine()` and write a function which would be its inverse. This was partially done in the original migration code, and attempted to be completed in #30328. However, further analysis of `IsMine()` has continued to reveal additional edge cases which make writing an inverse function increasingly difficult to verify correctness.
This PR instead changes migration to utilize `IsMine()` to produce the output scripts by first computing a superset of all of the output scripts that `IsMine()` would watch and testing each script against `IsMine()` to filter for the ones that actually are watched. The superset is constructed by computing all possible output scripts for the keys and scripts in the wallet - for keys, every key could be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH; for scripts, every script could be an output script, the redeemScript of a P2SH, the witnessScript of a P2WSH, and the witnessScript of a P2SH-P2WSH.
Additionally, the legacy wallet can contain scripts that are redeemScripts and witnessScripts, while not watching for any output script utilizing that script. These are known as solvable scripts and are migrated to a separate "solvables" wallet. The previous approach to identifying these solvables was similar to identifying output scripts - finding known solvable conditions and computing the scripts. However, this also can miss scripts, so the solvables are now identified in a manner similar to the output scripts but using the function `CanProvide()`. Using the same superset as before, all output scripts which are `ISMINE_NO` are put through `CanProvide()` which will perform a dummy signing and then a key lookup to determine whether the legacy wallet could provide any solving data for the output script. The scripts that pass will have their descriptors inferred and the script included in the solvables wallet.
The main downside of this approach is that `IsMine()` and `CanProvide()` can no longer be deleted. They will need to be refactored to be migration only code instead in #28710.
Lastly, I've added 2 test cases for the edge cases that prompted this change of approach. In particular, miniscript witnessScripts and `rawtr()` output scripts are solvable and signable in a legacy wallet, although never `ISMINE_SPENDABLE`.
ACKs for top commit:
sipa:
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.
brunoerg:
code review ACK af76664b12d8611b606a7e755a103a20542ee539
rkrux:
ACK af76664b12d8611b606a7e755a103a20542ee539
Tree-SHA512: 7f58a90de6f38fe9801fb6c2a520627072c8d66358652ad0872ff59deb678a82664b99babcfd874288bebcb1487d099a77821f03ae063c2b4cbf2d316e77d141
This commit does not change behavior, it just changes code to handle -noconnect
values explicitly with IsArgNegated() instead of implicitly with IsArgSet(),
and adds comments to make it clear what behavior is intended when -noconnect is
specified.
This commit does not change behavior because negation of -signetseednode and
-signetchallenge parameters has been disallowed since these were introduced in
#18267, so calling IsArgSet() is equivalent to checking if GetArgs() returns a
non-empty list.
This commit does not change behavior, it just drops unnecessary IsArgSet()
calls for -debug, -loglevel, and -vbparams options. The calls are unnecessary
because GetArgs() already returns empty arrays if these arguments are not
specified.
Treat specifying -noexternalip the same as not specifying -externalip, instead
of causing it to soft-set the -discover default to false.
Before this change, was -noexternalip basically an undocumented synonym for
-nodiscover.
After this change, specifying -noexternalip just clears previously specifed
-externalip options, restoring default behavior as if they were not were
specified.
The previous -noexternalip behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -nodiscover option, and inconsistent with
behavior of other list options.
Treat specifying -noonlynet the same as not specifying -onlynet, instead of
marking all networks unreachable.
Before this change, specifying -noonlynet cleared list of reachable networks
and did not allow connecting to any network. It was basically an undocumented
synonym for -noconnect.
After this change, specifying -nononlynet just clears previously specifed
-onlynet options and allows connecting to all networks, restoring default
behavior as if no -onlynet options were specified.
Before this change, there was no way to restore default behavior once an
-onlynet option was specified. So for example, if a config file specifed
onlynet settings, they couldn't be reset on the command line without disabling
the entire config file.
The previous -noonlynet behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -noconnect option, inconsistent with behavior
of other list options, and inconsistent with being able to use the command line
to selectively override config options. It was also probably unintended,
arising from use of the IsArgSet() method and its interaction with negated
options.
Treat specifying -norpcwallet the same as not specifying any -rpcwallet option,
instead of treating it like -rpcwallet=0 with 0 as the wallet name.
This restores previous behavior before 743077544b5420246ef29e0b708c90e3a8dfeeb6
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
Treat specifying -norpcwhitelist the same as not specifying -rpcwhitelist,
instead of behaving almost the same but flipping the default
-rpcwhitelistdefault value.
This is confusing because before this change if -norpcwhitelist was specified
it would block users from calling any RPC methods.
Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.
Also add code comment to clarify what intent of existing code is.
Treat specifying -noseednode the same as not specifying any -seednode value,
instead of enabling the seed node timeout and log messages, and waiting longer
to add other seeds.
