113a7a363faf1ace6b08a28cf4075dbce05f8f04 build: remove ENABLE_HARDENING cond from check-security (fanquake)
Pull request description:
This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
ACKs for top commit:
maflcko:
lgtm ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
TheCharlatan:
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04
hebasto:
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04.
Tree-SHA512: 46217e8ab238e23374d758b12e5b6bdc22353d8bf272aa0d2260cdea023b5b80aba972dccaa0a4fb8da21c8c665991848f7fd79966d20ac2489d499c68d95639
`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
fa3e409c9a084112fc2644a2bba9aa196bdb229d contrib: Add deterministic-fuzz-coverage (MarcoFalke)
Pull request description:
The goal of this script is to detect and debug the remaining fuzz determinism and stability issues (https://github.com/bitcoin/bitcoin/issues/29018).
ACKs for top commit:
marcofleon:
Tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d
brunoerg:
tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d
Tree-SHA512: f336537d64188d6bc3c53880f4552a09cc498841c539cb7b4f14e622c9542531b970c1a6910981f7506e7bf659d2ce83471d58f5f51b0a411868f4c11eaf6b2a
This check is only used in release builds, where hardening should always
be enabled. I can't think of a reason we'd want to silently skip these
checks if hardening was inadvertently disabled.
4080b66cbec2b6fc2fcfd7356941236f65d508e3 test: add test for utxo-to-sqlite conversion script (Sebastian Falbesoner)
ec99ed738083fc1ad4c9d85095e26e7e58372217 contrib: add tool to convert compact-serialized UTXO set to SQLite database (Sebastian Falbesoner)
Pull request description:
## Problem description
There is demand from users to get the UTXO set in form of a SQLite database (#24628). Bitcoin Core currently only supports dumping the UTXO set in a binary _compact-serialized_ format, which was crafted specifically for AssumeUTXO snapshots (see PR #16899), with the primary goal of being as compact as possible. Previous PRs tried to extend the `dumptxoutset` RPC with new formats, either in human-readable form (e.g. #18689, #24202), or most recently, directly as SQLite database (#24952). Both are not optimal: due to the huge size of the ever-growing UTXO set with already more than 80 million entries on mainnet, human-readable formats are practically useless, and very likely one of the first steps would be to put them in some form of database anyway. Directly adding SQLite3 dumping support on the other hand introduces an additional dependency to the non-wallet part of bitcoind and the risk of increased maintenance burden (see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1163551060, https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108469715).
## Proposed solution
This PR follows the "external tooling" route by adding a simple Python script for achieving the same goal in a two-step process (first create compact-serialized UTXO set via `dumptxoutset`, then convert it to SQLite via the new script). Executive summary:
- single file, no extra dependencies (sqlite3 is included in Python's standard library [1])
- ~150 LOC, mostly deserialization/decompression routines ported from the Core codebase and (probably the most difficult part) a little elliptic curve / finite field math to decompress pubkeys (essentialy solving the secp256k1 curve equation y^2 = x^3 + 7 for y given x, respecting the proper polarity as indicated by the compression tag)
- creates a database with only one table `utxos` with the following schema:
```(txid TEXT, vout INT, value INT, coinbase INT, height INT, scriptpubkey TEXT)```
- the resulting file has roughly 2x the size of the compact-serialized UTXO set (this is mostly due to encoding txids and scriptpubkeys as hex-strings rather than bytes)
[1] note that there are some rare cases of operating systems like FreeBSD though, where the sqlite3 module has to installed explicitly (see #26819)
A functional test is also added that creates UTXO set entries with various output script types (standard and also non-standard, for e.g. large scripts) and verifies that the UTXO sets of both formats match by comparing corresponding MuHashes. One MuHash is supplied by the bitcoind instance via `gettxoutsetinfo muhash`, the other is calculated in the test by reading back the created SQLite database entries and hashing them with the test framework's `MuHash3072` module.
## Manual test instructions
I'd suggest to do manual tests also by comparing MuHashes. For that, I've written a go tool some time ago which would calculate the MuHash of a sqlite database in the created format (I've tried to do a similar tool in Python, but it's painfully slow).
```
$ [run bitcoind instance with -coinstatsindex]
$ ./src/bitcoin-cli dumptxoutset ~/utxos.dat
$ ./src/bitcoin-cli gettxoutsetinfo muhash <block height returned in previous call>
(outputs MuHash calculated from node)
$ ./contrib/utxo-tools/utxo_to_sqlite.py ~/utxos.dat ~/utxos.sqlite
$ git clone https://github.com/theStack/utxo_dump_tools
$ cd utxo_dump_tools/calc_utxo_hash
$ go run calc_utxo_hash.go ~/utxos.sqlite
(outputs MuHash calculated from the SQLite UTXO set)
=> verify that both MuHashes are equal
```
For a demonstration what can be done with the resulting database, see https://github.com/bitcoin/bitcoin/pull/24952#pullrequestreview-956290477 for some example queries. Thanks go to LarryRuane who gave me to the idea of rewriting this script in Python and adding it to `contrib`.
ACKs for top commit:
ajtowns:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3 - light review
achow101:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
romanz:
tACK 4080b66cbe on signet (using [calc_utxo_hash](8981aa3e85/calc_utxo_hash/calc_utxo_hash.go)):
tdb3:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
Tree-SHA512: be8aa0369a28c8421a3ccdf1402e106563dd07c082269707311ca584d1c4c8c7b97d48c4fcd344696a36e7ab8cdb64a1d0ef9a192a15cff6d470baf21e46ee7b
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
c3fa043ae560289759b6f836ac62736d97ba91bf doc: build: Fix instructions for msvc gui builds (David Gumberg)
Pull request description:
If the instructions in `doc/build-windows-msvc.md` are followed as-is, and "Developer (PowerShell|Command Prompt) for VS 2022" is used to execute the suggested build commands, the root directory of vcpkg (e.g. in VS 2022 Community edition: `C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg`), is too long, and when vcpkg attempts to build any of the QT packages, it will fail because of build steps that require path lengths greater than Windows' `MAX_PATH` 260 character limit. This can be avoided without needing to move the vcpkg root dir by setting [`--x-buildtrees-root`](https://learn.microsoft.com/en-us/vcpkg/commands/common-options#buildtrees-root) to a short path, like `C:\vcpkg`.
See e.g. https://github.com/microsoft/vcpkg/issues/28451, https://github.com/microsoft/vcpkg/issues/28083, https://github.com/microsoft/vcpkg/issues/24751.
ACKs for top commit:
achow101:
ACK c3fa043ae560289759b6f836ac62736d97ba91bf
hebasto:
ACK c3fa043ae560289759b6f836ac62736d97ba91bf.
TheCharlatan:
ACK c3fa043ae560289759b6f836ac62736d97ba91bf
Tree-SHA512: 7de11d38b9125de63e72415f79d82f18818123a1b37f679f2229c4c82f5628dd7d1039dbc5dcdf1bc1c8c382e3e29de74a31d256e73872cbf1fa2687c52185ca
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
f0e5e4cdbec4df190c952d5a61d7a882a7e3b59e test: Add test for rpcwhitelistdefault (naiyoma)
Pull request description:
This PR adds tests for `rpcwhitelistdefault.` The implementation is a continuation of this [PR](https://github.com/bitcoin/bitcoin/pull/17805).
Applied suggestions to include the tests in` rpc_whitelist.py` and to use a single node.
PR covers three test cases:
- rpcwhitelistdefault = 0, no permissions
- rpcwhitelistdefault = 1, no permissions
- rpcwhitelistdefault = 1, with user permissions
I didn't add tests for rpcwhitelistdefault = 0 with user permissions since that is already tested here: [rpc_whitelist.py#L77](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_whitelist.py#L77).
ACKs for top commit:
achow101:
ACK f0e5e4cdbec4df190c952d5a61d7a882a7e3b59e
ryanofsky:
Code review ACK f0e5e4cdbec4df190c952d5a61d7a882a7e3b59e. PR seems very clear and simple, moving 1 test and adding 3 new tests.
ismaelsadeeq:
Tested and Code review ACK f0e5e4cdbec4df190c952d5a61d7a882a7e3b59e
Tree-SHA512: c3652940d2f23746e769ebe834e43dee47b7af8f258cbb133e38663aa8a05a1a8d0194d3008c3a10b0c54d11b5b95420c9cad0aa761c0fc1b9559277443b0696
a759ea3e920dcba52798755ba9f3891f914afbb0 doc: Improve dependencies documentation (Nicola Leonardo Susca)
Pull request description:
Initially there was a distinction between the compiler dependencies and
other required dependencies (refs https://github.com/bitcoin/bitcoin/pull/23565) but the distinction was
removed (refs https://github.com/bitcoin/bitcoin/pull/24585) which is why having two distinct tables could lead
to confusion now.
ACKs for top commit:
achow101:
ACK a759ea3e920dcba52798755ba9f3891f914afbb0
hodlinator:
re-ACK a759ea3e920dcba52798755ba9f3891f914afbb0
rkrux:
ACK a759ea3e920dcba52798755ba9f3891f914afbb0
Tree-SHA512: 14aaf9356d65bd150c9993dcbc51b1b98c835a760b95e6d91e69460c97c18f1dd10eb52b9f1d70129e6aa5e361af3a55619fd35787ed4e1ec48909568adbb604
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
81c174e3186efae084829dcde314b081cad3d3cb cmake: Refer to the configure log instead of printing PIE test error (Hennadii Stepanov)
65a0920ca6b11b4250b5cdcf174eaa9ea55fa6ed cmake: Add `CheckLinkerSupportsPIE` module (Hennadii Stepanov)
Pull request description:
This new module is a wrapper around CMake's `CheckPIESupported` module that fixes an upstream bug.
See: https://gitlab.kitware.com/cmake/cmake/-/issues/26463.
Fixes https://github.com/bitcoin/bitcoin/issues/30771.
ACKs for top commit:
theuni:
utACK 81c174e3186efae084829dcde314b081cad3d3cb.
vasild:
ACK 81c174e3186efae084829dcde314b081cad3d3cb
Tree-SHA512: 77d7022238551a4e69c59d1fe6b78975bb552cbbed5339459853d7ebf0086813036081f464fed230be330b3bd7d6cf8590b536b064028d2f786d6ae40f342f95
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
dead9086543671b07e6ce041821e4d2a6627075b cmake: Improve compatibility with Python version managers (Hennadii Stepanov)
Pull request description:
This PR resolves the issue [highlighted](https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547) in https://github.com/bitcoin/bitcoin/pull/31411:
> Here's another case where CMake just picks some other Python...
The fix leverages two [hints](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints) for the CMake `FindPython3` module:
1. `Python3_FIND_FRAMEWORK` is set to `LAST`. This ensures that Unix-style package components are preferred over frameworks on macOS. As a side effect, the `FindPython3` module reports a shim or symlink (e.g., from `pyenv`) rather than the underlying framework's binary. The module's output aligns with the result of the `which` command.
2. `Python3_FIND_UNVERSIONED_NAMES` is set to `FIRST`. This supports scenarios where tools like `pyenv`—which use shims—have multiple Python versions installed.
Here are examples of output on my macOS 15.1.1 (Intel) with installed Homebrew's [Python 3.13.0](https://formulae.brew.sh/formula/python@3.13):
- without any Python version manager:
```
% which -a python3
/usr/local/bin/python3
/usr/bin/python3
% cmake -B build
<snip>
-- Found Python3: /usr/local/bin/python3 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpreter
<snip>
```
- using `pyenv`:
```
% pyenv versions
system
* 3.10.14 (set by /Users/hebasto/dev/bitcoin/.python-version)
3.12.8
3.13.1
% which -a python3
/Users/hebasto/.pyenv/shims/python3
/usr/local/bin/python3
/usr/bin/python3
% cmake -B build
<snip>
-- Found Python3: /Users/hebasto/.pyenv/shims/python3 (found suitable version "3.10.14", minimum required is "3.10") found components: Interpreter
<snip>
```
Both variables, `Python3_FIND_FRAMEWORK` and `Python3_FIND_UNVERSIONED_NAMES`, can still be overridden by the user via the command line if needed.
ACKs for top commit:
theuni:
No opinion on the python selection changes themselves, but code-review ACK dead9086543671b07e6ce041821e4d2a6627075b
willcl-ark:
ACK dead9086543671b07e6ce041821e4d2a6627075b
Tree-SHA512: 69f8541223e5b6c35c892b4ba2a2dcfc24b41a10cf20accc75d3008b16434db8a9240c99c886c3a4566ba24269c5b0e0d856357891811f0a77b39f4afbee3634
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
2434aeab62ba07c5380112838f3600b3dbbceef2 depends: avoid an unset CMAKE_OBJDUMP (fanquake)
Pull request description:
Similar to #31840, currently our Linux toolchain file contains:
```bash
set(CMAKE_AR "aarch64-linux-gnu-ar")
set(CMAKE_RANLIB "aarch64-linux-gnu-ranlib")
set(CMAKE_STRIP "aarch64-linux-gnu-strip")
set(CMAKE_OBJCOPY "aarch64-linux-gnu-objcopy")
set(CMAKE_OBJDUMP "")
```
`objdump` is currently only used for the macOS cross build, where it's `llvm-objdump`, but we should be consistent in producing a toolchain file that points to actual tools, rather than leaving variables unset.
ACKs for top commit:
hebasto:
ACK 2434aeab62ba07c5380112838f3600b3dbbceef2.
theuni:
utACK 2434aeab62ba07c5380112838f3600b3dbbceef2
Tree-SHA512: 65f6b7b9cae79e9c0784c108709139125e52d8f2818afbea5f719bc1b6dc338b870abbdfcb174ae541c0027a7ac07cb56012735b7a37b58b9a6e55a48c0257cf
0a02e7fdeaca26455b3710ef76b11101ac816e53 test: deduplicates p2p_tx_download constants (Sergi Delgado Segura)
Pull request description:
Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py
ACKs for top commit:
i-am-yuvi:
re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
maflcko:
review ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53 🔖
danielabrozzoni:
re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
tdb3:
re ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
Tree-SHA512: 05fc114a32b6b42a7c57563a38f1a8921e0bb224c4b124ae9d395c3a1105ae6e9cdfc62f603f4f2dee55cef5f6a6ed400d328740ad84fbd3093c5e0f3fb2982a
bb0879ddabc8b3a7253bc774d23b842937d18015 test: check `scanning` field from `getwalletinfo` (brunoerg)
Pull request description:
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
ACKs for top commit:
maflcko:
lgtm ACK bb0879ddabc8b3a7253bc774d23b842937d18015
arejula27:
ACK [`bb0879d`](bb0879ddab)
achow101:
ACK bb0879ddabc8b3a7253bc774d23b842937d18015
BrandonOdiwuor:
Code Review ACK bb0879ddabc8b3a7253bc774d23b842937d18015
Prabhat1308:
re-ACK [`bb0879d`](bb0879ddab)
Tree-SHA512: 9bca1c1e813bf4f61a5621bdc0a5f5c2bcfb388ffe9dfacb821bf6954f6e0880140d72258dc93ab6b84efb54f55c682a17aebd42f6559d6cfac9998e6bc4e5b9
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.
Make sure -noconnect has same effect as -connect for disabling DNS seeding and
listening by default, and warning about -dnsseed being ignored with the -proxy
setting.
Initial implementation of https://github.com/bitcoin/bitcoin/pull/30529
accidentally broke this behavior, so having coverage may be useful to make sure
it does not break again.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
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.
