fa44f5119a ci: Clarify that previous_releases task is using DEBUG (MarcoFalke)
fad0f21c3c ci: Use clang in multiprocess task to avoid OOM (MarcoFalke)
faeabef4f3 ci: Enable D_GLIBCXX_DEBUG for multiprocess task (MarcoFalke)
Pull request description:
Enable `-D_GLIBCXX_DEBUG` via the depends `DEBUG` flag. Also `--enable-debug` to get debug symbols in traces.
ACKs for top commit:
hebasto:
ACK fa44f5119a, I have reviewed the code and it looks OK, I agree it can be merged, and CI is green.
Tree-SHA512: ab2a216bb44ee462f9dd181ec9025962502bd4201a1118ff52b6a193398e7ea3ca465a45a5eb341e308758fc3ef34ea3521f8a1f85ed64478ef3c1f6c1b8b141
142e2da440 net: add I2P seeds to chainparamsseeds (Jon Atack)
e01f173fb9 contrib: add a few I2P seed nodes (Jon Atack)
ea269c7ef1 contrib: parse I2P addresses in generate-seeds.py (Jon Atack)
Pull request description:
Follow-up to #21560 that updated the fixed seeds infra for BIP155 addresses and then added Tor v3 ones:
- Update contrib/generate-seeds.py to parse I2P addresses
- Add a few I2P nodes to contrib/seeds/nodes_main.txt
- Run generate-seeds.py and add the I2P seeds to chainparamsseeds.h
Reviewers, see contrib/seeds/README.md for more info and feel free to use the following CLI one-liner to check for and propose additional seeds for contrib/seeds/nodes_main.txt. You can also see how many I2P peers your node knows with cli -addrinfo.
```rake
bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".b32.i2p"))) | .address' | sort
```
I verified the I2P addresses are correctly BIP155-serialized/deserialized by building with all seeds removed from chainparamsseeds.h except those added here, restarting with `-datadir=newdir -dnsseed=0` and running rpc ` getnodeaddresses 0` that initially returns only the new I2P addresses.
ACKs for top commit:
laanwj:
ACK 142e2da440
vasild:
ACK 142e2da440
Tree-SHA512: 040576012d5f1f034e2bd566ad654a6fdfd8ff7f6b12fa40c9fda1e948ebf8417fcea64cfc14938a41439370aa4669bab3e97274f9d4f9a6906fa9520afa9cf8
575792e6ff fuzz: Add -fsanitize=integer suppression needed for RPC fuzzer (practicalswift)
Pull request description:
Add `-fsanitize=integer` suppression needed for RPC fuzzer (`generateblock`).
Context: https://github.com/bitcoin-core/qa-assets/pull/59/checks?check_run_id=2494624259
```
miner.cpp:130:21: runtime error: implicit conversion from type 'int64_t' (aka 'long') of value 244763573890 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4245405314 (32-bit, unsigned)
#0 0x56143974eaf3 in BlockAssembler::CreateNewBlock(CScript const&) miner.cpp:130:21
#1 0x56143993690d in generateblock()::$_4::operator()(RPCHelpMan const&, JSONRPCRequest const&) const rpc/mining.cpp:370:127
```
ACKs for top commit:
practicalswift:
> review ACK [575792e](575792e6ff), but this function shouldn't be called by the rpc fuzzer, at least not without sanitizing num_blocks
MarcoFalke:
review ACK 575792e6ff
Tree-SHA512: c2133d1064bf17df0e7749ef4a0f7664b5c8082040491a1035d39f0c6e5d96997b347ef2354411e285c7f1f973e34515f1c3c88eb3de60fab64ca4d2adf6dd74
5252f86eb6 fuzz: Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of #ifdef forests (practicalswift)
54549dda31 fuzz: RPC fuzzer post-merge follow-ups. Remove unused includes. Update list of fuzzed RPC commands. (practicalswift)
Pull request description:
Various RPC fuzzer follow-ups:
* Remove unused includes.
* Update list of fuzzed RPC commands.
* Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of `#ifdef` forests.
Context: https://github.com/bitcoin/bitcoin/pull/21169#pullrequestreview-646723483
ACKs for top commit:
MarcoFalke:
Concept ACK 5252f86eb6
Tree-SHA512: 286d70798131706ffb157758e1c73f7f00ed96ce120c7d9dc849e672b283f1362df47b206cfec9da44d5debb5869225e721761dcd5c38a7d5d1019dc6c912ab2
48bd9ebc5d script: update REVIEWERS (Adam Jonas)
Pull request description:
Meta: `git show a06eb03` indicates the commit was first made one year ago and the PR was merged in September 2020.
It might be time for an update, if automated review requests via DrahtBot are operational.
"Regular contributors are free to add their names to specific directories or files provided that they are willing to provide a review."
Perhaps we can compile and squash suggested updates here, per that guideline.
ACKs for top commit:
practicalswift:
ACK 48bd9ebc5d: patch looks correct :)
adamjonas:
ACK 48bd9eb. I ran through the new files added with `*`s using `git ls-files`.
hebasto:
ACK 48bd9ebc5d, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 770fd60d78bd64100814c72211e4bd97718c87a55a4c91e136fababa0149d12cdc1b3e89462dee3a55e5f51d3263420cc6d856868469eb9a72ff8c930859c54f
It does not matter if the tests fail due to a BOOST_CHECK failure or
due to a thrown exception. Prefer the exception because it is less
code.
Example fail with the throwing accessor:
unknown location(0): fatal error: in "script_standard_tests/script_standard_ExtractDestinations": std::bad_variant_access: std::get: wrong index for variant
test/script_standard_tests.cpp(314): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
fac96d0265 p2p: Limit m_block_inv_mutex (MarcoFalke)
Pull request description:
Keeping the lock longer than needed is confusing to reviewers and thread analysis. For example, keeping the lock while appending tx-invs, which requires the mempool lock, will tell thread analysis tools an incorrect lock order of `(1) m_block_inv_mutex, (2) pool.cs`.
ACKs for top commit:
Crypt-iQ:
crACK fac96d0265
jnewbery:
utACK fac96d0265
theStack:
Code-Review ACK fac96d0265
Tree-SHA512: fcfac0f1f8b16df7522513abf716b2eed3d2fc9153f231c8cb61f451e342f29c984a5c872deca6bab3e601e5d651874cc229146c9370e46811b4520747a21f2b
9096b13a47 net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov)
Pull request description:
It is not possible to have a node in `CConnman::vNodesDisconnected` and
its reference count to be incremented - all `CNode::AddRef()` are done
either before the node is added to `CConnman::vNodes` or while holding
`CConnman::cs_vNodes` and the object being in `CConnman::vNodes`.
So, the object being in `CConnman::vNodesDisconnected` and its reference
count being zero means that it is not and will not start to be used by
other threads.
So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will
always succeed and is not necessary.
Indeed all locks of `CNode::cs_vSend` are done either when the reference
count is >0 or under the protection of `CConnman::cs_vNodes` and the
node being in `CConnman::vNodes`.
ACKs for top commit:
MarcoFalke:
review ACK 9096b13a47🏧
jnewbery:
utACK 9096b13a47
Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
a5550f877a build: use -stdlib++-isystem with Clang 10 (fanquake)
51d9d1607f guix: use Clang 10 for the macOS cross compile (fanquake)
b80a6af9e5 build: no longer patch threading out of ld64 (fanquake)
c29cba44b3 build: Xcode 12.1, macOS SDK 10.15.6 (fanquake)
9ed2f19d38 build: native cctools 973.0.1, ld64 609 (fanquake)
f48f187cce build: Clang 10.0.1 (Hennadii Stepanov)
9b193cd2a3 build: libtapi 1100.0.11 (fanquake)
Pull request description:
Bumps our macOS toolchain to be using the following:
* Clang 10.0.1 (gitian) & Clang 10.0.0 (Guix)
* ld64 609
* libtapi 1100.0.11
* cctools 973.0.1
* Xcode 12.1
* macOS SDK 10.15.6
which are currently the most recent releases available as open source. See upstream [`cctools`](https://github.com/tpoechtrager/cctools-port/commits/973.0.1-ld64-609) and [`libtapi`](https://github.com/tpoechtrager/apple-libtapi/tree/1100.0.11).
This should improve the possibility of Apple ARM cross-compilation in depends.
This also removes our [patching out of pthreads usage](https://github.com/bitcoin/bitcoin/blob/master/depends/patches/native_cctools/ld64_disable_threading.patch) in `ld64`. There have been multiple changes since `ld64 450.3`, which have likely fixed the non-determinism we were working around. i.e from [InputFiles.cpp](https://opensource.apple.com/source/ld64/ld64-609/src/ld/InputFiles.cpp.auto.html):
```cpp
// <rdar://problem/15002251> make implicit dylib order be deterministic by sorting by install_name
std::sort(implicitDylibs.begin(), implicitDylibs.end(), DylibByInstallNameSorter());
```
```cpp
// <rdar://problem/42675402> ld64 output is not deterministic due to dylib processing order
std::sort(unprocessedDylibs.begin(), unprocessedDylibs.end(), [](const ld::dylib::File* lhs, const ld::dylib::File* rhs) {
return strcmp(lhs->path(), rhs->path()) < 0;
});
```
Guix Build:
```bash
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
f6c3817b8fe5f7370299d1ae2533e4a3acd313ba9f9aa8d423a8956117e52dd5 guix-build-a5550f877a2c/output/dist-archive/bitcoin-a5550f877a2c.tar.gz
4954dcf563c2d496b8d9fecd48f8e3f7fba2f319ffa254a5bc8ee12cfee6acf0 guix-build-a5550f877a2c/output/x86_64-apple-darwin18/bitcoin-a5550f877a2c-osx-unsigned.dmg
8f6095b445c7f1a8e6accd86bb7f0696d5849402084927d2b726b7d557831c3a guix-build-a5550f877a2c/output/x86_64-apple-darwin18/bitcoin-a5550f877a2c-osx-unsigned.tar.gz
cc40f25477b4defc1617ae694313d80f307ddf6742fe6cc85c6bc0e215ef8be0 guix-build-a5550f877a2c/output/x86_64-apple-darwin18/bitcoin-a5550f877a2c-osx64.tar.gz
```
Gitian Build:
```bash
Generating report
506a8abdefe559999b43dd9f14905b9b2b5a3363b1cd013d45ae47acc1f7ef6c bitcoin-a5550f877a2c-osx-unsigned.dmg
f606997f74026dd12d110d683c6f116b40df324836904ef507dd7ac787e6ebe2 bitcoin-a5550f877a2c-osx-unsigned.tar.gz
5b495ef15f2c3260c2950921b61326912a9bf533cccd51e13818809fd225489e bitcoin-a5550f877a2c-osx64.tar.gz
f6c3817b8fe5f7370299d1ae2533e4a3acd313ba9f9aa8d423a8956117e52dd5 src/bitcoin-a5550f877a2c.tar.gz
9eb0221e962d2839770963bd03c6c9e98e8bf3078566bee2ae42f06233a710fa bitcoin-core-osx-22-res.yml
Done.
```
ACKs for top commit:
hebasto:
ACK a5550f877a
Tree-SHA512: 504c4b0f9cd3b939714a322298320c5bde07e9356a48a9a000060b36f8dce4d6134ed60c3a5188810476a28ec5b108733eabbc6fb8053231b9ea8a494cc91b12
ebd4be43cc doc: add release notes for 20867 (Antoine Poinsot)
5aa50ab9cc rpc/util: multisig: only check redeemScript size is <= 520 for P2SH (Antoine Poinsot)
063df9e897 test/functional: standardness sanity checks for P2(W)SH multisig (Antoine Poinsot)
ae0429d3af script: allow up to 20 keys in wsh() descriptors (Antoine Poinsot)
9fc68faf35 script: match multisigs with up to MAX_PUBKEYS_PER_MULTISIG keys (Antoine Poinsot)
Pull request description:
As described in https://github.com/bitcoin/bitcoin/issues/20620 multisigs are currently limited to 16 keys in descriptors and RPC helpers, even for P2WSH and P2SH-P2WSH.
This adds support for multisig with up to 20 keys (which are already standard) for Segwit v0 context for descriptors (`wsh()`, `sh(wsh())`) and RPC helpers.
Fixes https://github.com/bitcoin/bitcoin/issues/20620
ACKs for top commit:
meshcollider:
re-utACK ebd4be43cc
instagibbs:
re-ACK ebd4be43cc
Tree-SHA512: 36141f10a8288010d17d5c4fe8d24878bcd4533b88a8aba3a44fa8f74ceb3182d70fee01427e0ab7f53ce7fab46c88c1cd3ac3b18ab8a10bd4a6b8b74ed79e46
0a331456e4 Remove Visual Studio 2017 reference from readme (Aaron Clauson)
Pull request description:
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017.
When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used.
It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious).
ACKs for top commit:
jarolrod:
ACK 0a331456e4
hebasto:
ACK 0a331456e4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 17e2326cd55a5cc3fc13622ba72bb44f9b9d55070cb77941df5fd943cd3f1baf02b9ac9504dfae5941c522748ad7b584c3c8da03fb323a87b3639eb926ce1699
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017.
When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used.
It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious).
Added advisory note about build not working with earlier Visual Studio versions.
Fixed grammar.
c5bb142817 test: resolve bug in test/functional/interface_bitcoin_cli.py - Test -getinfo with -rpcwallet=unloaded wallet returns no balances (klementtan)
Pull request description:
I think there is a bug in this test case where the new value of `cli_get_info` is not asserted.
ACKs for top commit:
jonatack:
ACK c5bb142817
Tree-SHA512: 50c0c2c8fe63c95f951dee892fbacedf92208f47efe5ed481fbb255f15137c799d9200fa3ff31a442df0691248d7ff04d899842722c3032cd7f35553622ba38c
cf971c9ea0 build: use -isysroot over --sysroot on macOS (fanquake)
Pull request description:
Not only does this seem to be the more correct behaviour when targeting Darwin, but if you use `-isysroot`, Clangs Darwin driver will [infer the deployment target](3e037f8f0e/clang/lib/Driver/ToolChains/Darwin.cpp (L1652)) from the SDK and use other SDK info when parsing arguments to the linker. In the case of [`-platform_version`](3e037f8f0e/clang/lib/Driver/ToolChains/Darwin.cpp (L2656)), which is added if the linker is [new enough](3e037f8f0e/clang/lib/Driver/ToolChains/Darwin.cpp (L342)), the version tuple is constructed from the SDKInfo, and SDKInfo, as far as I can tell, only exists when `-isysroot` has been passed, see [parseSDKSettings](3e037f8f0e/clang/lib/Driver/ToolChains/Darwin.cpp (L1785))
As a result, the SDK version field in the `LC_BUILD_VERSION` command is filled out. i.e when building master:
```bash
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk n/a
ntools 1
tool 3
version 650.9
```
vs this PR:
```bash
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk 11.3
ntools 1
tool 3
version 650.9
```
This, from what I understand, will fix the issue we are having with Qt deciding wether or not to enable features like "Dark mode" on macOS, see #21771, however I have not tested that. Thus this is an alternative to #21782.
Our usage of `--sysroot` was added in #17118.
```bash
-isysroot <dir> Set the system root directory (usually /)
```
ACKs for top commit:
Sjors:
tACK cf971c9ea0
hebasto:
re-ACK cf971c9ea0, only rebased and addressed comments since my [previous](https://github.com/bitcoin/bitcoin/pull/21793#pullrequestreview-647321518) review.
Tree-SHA512: f01138179fb85083b5505bbaa48810451098ffa4da5d3c9b673785448790aa76f2e64b2aab6e698f6ee378a21f70626445a3fabee7c61dbfc44e96f3e3964656
e94920a0bb qt: peertableview alternating row colors (randymcmillan)
Pull request description:
peers-tab: enable alternating row colors for peer table and banned table
ACKs for top commit:
Bosch-0:
tACK e94920a0bb on Windows 10 - works as intended. Before / after below:
jarolrod:
tACK e94920a0bb
Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d
jonatack:
Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
