fa52e13ee8 test: Remove struct.pack from almost all places (MarcoFalke)
fa826db477 scripted-diff: test: Use int.to_bytes over struct packing (MarcoFalke)
faf2a975ad test: Use int.to_bytes over struct packing (MarcoFalke)
faf3cd659a test: Normalize struct.pack format (MarcoFalke)
Pull request description:
`struct.pack` has many issues:
* The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
This lead to many test bugs, which weren't hit, which is fine, but still confusing. Ref: https://github.com/bitcoin/bitcoin/pull/29400, https://github.com/bitcoin/bitcoin/pull/29399, https://github.com/bitcoin/bitcoin/pull/29363, fa3886b7c6, ...
Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff.
Review notes:
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
* Two `struct.pack` remain. One for float serialization in a C++ code comment, and one for native serialization.
ACKs for top commit:
achow101:
ACK fa52e13ee8
rkrux:
tACK [fa52e13](fa52e13ee8)
theStack:
Code-review ACK fa52e13ee8
Tree-SHA512: ee80d935b68ae43d1654b047e84ceb80abbd20306df35cca848b3f7874634b518560ddcbc7e714e2e7a19241e153dee64556dc4701287ae811e26e4f8c57fe3e
232928b58a build: no-longer allow GCC-10 in C++20 check (fanquake)
Pull request description:
Reverts part of fa67f096bd, now that we require a minimum of GCC 11.
See also:
https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1745143612.
ACKs for top commit:
maflcko:
utACK 232928b58a
theuni:
utACK 232928b58a
Tree-SHA512: 10e0adac2dd6e455aaf97ebfe73c7586430349fc27ac435bc6c0d99a4934a380398d14467aacd9cedf371345da291366b3ab2c3be7db5d97e21ad6212b2c7890
5f2c1d84e3 guix: show *_FLAGS variables in pre-build output (fanquake)
Pull request description:
For example:
```bash
# ADDITIONAL_GUIX_COMMON_FLAGS set in the ENV
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS="--emulate-fhs" ./contrib/guix/guix-build
<snip>
INFO: Building f751991 for platform triple x86_64-linux-gnu:
...using reference timestamp: 1716905119
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-f75199182133/distsrc-f75199182133-x86_64-linux-gnu'
...bind-mounted in container to: '/distsrc-base/distsrc-f75199182133-x86_64-linux-gnu'
...outputting in: '/bitcoin/guix-build-f75199182133/output/x86_64-linux-gnu'
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
ADDITIONAL FLAGS (if set)
ADDITIONAL_GUIX_COMMON_FLAGS: --no-substitutes
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS: --emulate-fhs
ADDITIONAL_GUIX_TIMEMACHINE_FLAGS:
```
ACKs for top commit:
hebasto:
ACK 5f2c1d84e3.
Tree-SHA512: 85a6d508499b4ec1d6166343a1707b682d327b2fcfb2fb438571894478aac0062d21e1239b5092091ff98711c5c747151973c4f325a7a7c447d0e807166fcb07
4444de152f test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure (MarcoFalke)
fa6aa4027c test: Fix typos and use names args (MarcoFalke)
Pull request description:
Otherwise, the test may fail on slow hardware when running in valgrind.
Also, use named args for the absolute timepoint, while touching this file.
ACKs for top commit:
tdb3:
ACK for 4444de152f
AngusP:
re-ACK 4444de152f
Tree-SHA512: 660269c8dd18887d69b284f38656899caf028159ce83ddf921f3e9c080a5d0e663989f0e42b4baf4c4939f20f34da0e7e844dff9b7c91d0cab570c60958bd0e1
8801e319d5 refactor: remove unused `CKey::Negate` method (Sebastian Falbesoner)
Pull request description:
This method was introduced as a pre-requirement for the v2 transport protocol back then (see PR #14047, commit 463921bb), when it was still BIP151. With the replacement BIP324, this is not needed anymore, and it's also unlikely that for any other proposal we'd ever need to negate private keys at this abstraction level. I'd argue that this operation is usually something that should happen within a secp256k1 module (like e.g. done in MuSig2, Silent Payments...).
(If there is really demand in the future, it's also trivial to reintroduce the method.)
ACKs for top commit:
laanwj:
ACK 8801e319d5
sipa:
ACK 8801e319d5
achow101:
ACK 8801e319d5
Tree-SHA512: 7bc1566399635c5c6e4940a2724c865d5443eb190024379099330c023c516f1e4f423ed9e8c42bc93413b723a5464ec79d3f879f58c0e598fe24f495238df4ec
07aba8dd21 functional test: ensure confirmed utxo being sourced for 2nd chain (Greg Sanders)
Pull request description:
The test could fail/stop testing what we want if non-confirmed utxos become sourced through some internal change to `MiniWallet`; better to just fetch confirmed explicitly.
ACKs for top commit:
achow101:
ACK 07aba8dd21
ismaelsadeeq:
utACK 07aba8dd21
theStack:
ACK 07aba8dd21
Tree-SHA512: 66795fdf881139ed91bde0f8239a46bd9bc70bb311fa97c0e2b5537e1fd2a1fd36bf3a225fc77b9695deb835a9d6d29879aa1e05ea5054b9a33a400e199da014
2451a217dd test: addmultisigaddress, coverage for script size limits (furszy)
53302a0981 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc0 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45 fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b578 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d3 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a3289433 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d43268 test: rpc_createmultisig, remove manual wallet initialization (furszy)
Pull request description:
Fixing https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104 and more.
Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:
1) The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
2) The `signrawtransactionwithkey` RPC command fail to sign them.
3) The legacy wallet `addmultisigaddress` wrongly discards them.
The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes
the [p2sh max redeem script size rule](ded6873340/src/script/signingprovider.cpp (L160)) on all scripts. Which blocks segwit redeem scripts longer than
the max element size in all the previously mentioned processes (`createmultisig`, `addmultisigaddress`, and
`signrawtransactionwithkey`).
This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and
allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte
p2sh limit.
Important note:
Instead of adding support for these longer redeem scripts in the legacy wallet, an "unsupported operation"
error has been added. The reasons behind this decision are:
1) The introduction of this feature brings about a compatibility-breaking change that requires downgrade
protection; older wallets would be unable to interact with these "new" legacy wallets.
2) Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling
reason to transition towards descriptors.
Testing notes:
To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be
cherry-picked on top of master. Where `rpc_createmultisig.py` (with and without the `--legacy-wallet`
arg) will fail without the bugs fixes commits.
Extra note:
The initial commits improves the `rpc_createmultisig.py` test in many ways. I found this test very
antiquated, screaming for an update and cleanup.
ACKs for top commit:
pinheadmz:
ACK 2451a217dd
theStack:
Code-review ACK 2451a217dd
achow101:
ACK 2451a217dd
Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f
d7290d662f fuzz: wallet, add target for Crypter (Ayush Singh)
Pull request description:
This PR adds fuzz coverage for `wallet/crypter`.
Motivation: Issue [27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)
I ran this for a long time with Sanitizers on and had no crashes; the average `exec/sec` also looks good to me. However, I would really appreciate it if some of the reviewers could try it on their machines too, and give their feedback.
ACKs for top commit:
maflcko:
utACK d7290d662f
achow101:
ACK d7290d662f
brunoerg:
utACK d7290d662f
Tree-SHA512: f5c496cabdd3263a7e1ad49eeff702725336f76bf19a82e5dbbead082e990889dd43c851d0d2d6ab740f44b8ec2aa06defd9ff6b02be68b5f8b4eaf963f88599
7f3f6c6dc8 refactor: replace hardcoded numbers (Lőrinc)
5676aec1e1 refactor: Model the bech32 charlimit as an Enum (josibake)
Pull request description:
Broken out from #28122
---
Bech32(m) was defined with a 90 character limit so that certain guarantees for error detection could be made for segwit addresses (see https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#checksum-design).
However, there is nothing about the encoding scheme itself that requires a limit of 90 and in practice bech32(m) is being used without the 90 char limit (e.g. lightning invoices, silent payments). Further, increasing the character limit doesn't do away with error detection, it simply changes the guarantee.
The primary motivation for this change is for being able to parse BIP352 v0 silent payment addresses (see 622c7a98b9), which require up to 118 characters. In addition to BIP352, modeling the character limit as an enum allows us to easily support new address types that use bech32m and specify their own character limit.
ACKs for top commit:
paplorinc:
re-ACK 7f3f6c6dc8
achow101:
ACK 7f3f6c6dc8
theuni:
utACK 7f3f6c6dc8
Tree-SHA512: 9c793d657448c1f795093b9f7d4d6dfa431598f48d54e1c899a69fb2f43aeb68b40ca2ff08864eefeeb6627d4171877234b5df0056ff2a2b84415bc3558bd280
fa3169b073 rpc: Remove index-based Arg accessor (MarcoFalke)
Pull request description:
The index-based Arg accessor is redundant with the name-based one. It does not provide any benefit to the code reader, or otherwise, so remove it.
ACKs for top commit:
stickies-v:
re-ACK fa3169b073, addressed doc nits
achow101:
ACK fa3169b073
ryanofsky:
Code review ACK fa3169b073. One changes since last review are some documentation improvements
Tree-SHA512: f9da1c049dbf38c3b47a8caf8d24d195c2d4b88c7ec45a9ccfb78f1e39f29cb86869f84b308f6e49856b074c06604ab634c90eb89c9c93d2a8169e070aa1bd40
df6dc2aaae test: Assumeutxo: snapshots with less work should not be loaded (Hernan Marino)
Pull request description:
This PR adds a test which checks that snapshots with less accumulated work than the node's active chain, should not be loaded and return with an error. Although in a different context of discussion the missing test was detect in a thread in https://github.com/bitcoin/bitcoin/pull/29394 (see https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484122214)
ACKs for top commit:
maflcko:
utACK df6dc2aaae
kevkevinpal:
utACK [df6dc2a](df6dc2aaae)
achow101:
ACK df6dc2aaae
alfonsoromanz:
Re ACK df6dc2aaae. Make is successful and the test passes.
Tree-SHA512: 07a394b4b288cc8ad3f66ed4e70dcda468db18113e9442eb7215cf491768432d55efaaa5b79d633094917e05475a30f0c5e4f64f8f2da293ba306891b4485560
9013e2b97e Link to gen-bitcoin-conf.sh instead of bitcoin.conf placeholder (Epic Curious)
Pull request description:
Closes#30153.
This PR updates `doc/init.md` to mention generating an example bitcoin.conf instead of referencing the placeholder `share/examples/bitcoin.conf`. Also changes the code-formatted text to a markdown link.
## Background
- Two years ago, `share/examples/bitcoin.conf` was replaced with [a placeholder file](b483084d86). To see an example `bitcoin.conf`, the user now runs the `contrib/devtools/gen-bitcoin-conf.sh` script, which replaces the placeholder file with the parsed contents of `bitcoind --help`.
- The instructions in `init.md` about an example `bitcoin.conf` haven't changed significantly since they were [added almost 10 years ago](234bfbf6a5/doc/init.md (L39)). They should be updated to improve clarity.
ACKs for top commit:
edilmedeiros:
ACK 9013e2b97e
kevkevinpal:
reACK [9013e2b](9013e2b97e)
achow101:
ACK 9013e2b97e
stickies-v:
ACK 9013e2b97e
tdb3:
ACK for 9013e2b97e
Tree-SHA512: 5ac5ad672ad181d574e19e29c3727fb9e5373282444fae09b42d113d5c8915cb2829d496212638cdc4b70540b7e1794a751fcdc9539f956a594cddd70c8fd747
71aae72e1f test: test sendall does ancestor aware funding (ishaanam)
36757941a0 wallet, rpc: implement ancestor aware funding for sendall (ishaanam)
544131f3fb rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified (ishaanam)
Pull request description:
This PR:
- Adds a functional test that `sendall` spends unconfirmed change
- Adds a functional test that `sendall` spends regular unconfirmed inputs when specified by user
- Adds ancestor aware funding to `sendall` by using `calculateCombinedBumpFee` and adjusting the effective value accordingly
- Adds a functional test for ancestor aware funding in `sendall`
ACKs for top commit:
S3RK:
ACK 71aae72e1f
achow101:
ACK 71aae72e1f
furszy:
ACK 71aae72e1f
Tree-SHA512: acaeb7c65166ce53123a1d6cb5012197202246acc02ef9f37a28154cc93afdbd77c25e840ab79bdc7e0b88904014a43ab1ddea79d5337dc310ea210634ab61f0
bd34dd85e7 Use `exact_target` shorthand in coinselector_tests (Murch)
7aa7e30441 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch)
Pull request description:
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0).
ACKs for top commit:
achow101:
ACK bd34dd85e7
furszy:
Code ACK bd34dd85e7
ismaelsadeeq:
Code Review ACK bd34dd85e7
Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
22d0f1a27e [fuzz] Avoid endless waiting in FuzzedSock::{Wait,WaitMany} (marcofleon)
a7fceda68b [fuzz] Make peeking through FuzzedSock::Recv fuzzer friendly (dergoegge)
865cdf3692 [fuzz] Use fuzzer friendly ConsumeRandomLengthByteVector in FuzzedSock::Recv (dergoegge)
Pull request description:
`FuzzedSock` has a few issues that block a fuzzer from making progress. See commit messages for details.
ACKs for top commit:
marcofleon:
Tested ACK 22d0f1a27e
brunoerg:
utACK 22d0f1a27e
Tree-SHA512: 2d66fc94ba58b6652ae234bd1dcd33b7d685b5054fe83e0cd624b053dd51519c23148f43a865ab8c8bc5fc2dc25e701952831b99159687474978a90348faa4c5
ffbc173ca1 depends: Update Boost download link (Hennadii Stepanov)
Pull request description:
The Boost has [migrated](https://github.com/boostorg/boost-tasks/pull/3) their downloads from from boostorg.jfrog.io to archives.boost.io.
So do we.
FWIW, the download speed at my location is much better :)
ACKs for top commit:
maflcko:
ACK ffbc173ca1
Tree-SHA512: 3f66675c390510ecfdacf2ac8af4a0a6bdbdf6fbfe01ed66fbe1b11bc9e935709e5492a754711cd788352c5853738fce755afd0c0480f36d0f8af31fcc108263
This method was introduced as a pre-requirement for the v2 transport
protocol back then (see PR #14047, commit 463921bb), when it was still
BIP151. With the replacement BIP324, this is not needed anymore, and
it's also unlikely that any other proposal would need to negate private
keys at this abstraction level.
(If there is really demand, it's trivial to reintroduce the method.)
3c08e11c3e doc: JSON-RPC request Content-Type is application/json (Luke Dashjr)
Pull request description:
Specify json content type in RPC examples.
Picks up #29946. Which needed rebasing and the commit message fixing,
ACKs for top commit:
laanwj:
ACK 3c08e11c3e
tdb3:
ACK for 3c08e11c3e
Tree-SHA512: 770bbbc0fb324cb63628980b13583cabf02e75079851850170587fb6eca41a70b01dcedaf1926bb6488eb9816a3cc6616fe8cee8c4b7e09aa39b7df5834ca0ec
e3249f2111 fuzz: add more coverage for `ScriptPubKeyMan` (brunoerg)
Pull request description:
This PR adds more coverage for `ScriptPubKeyMan`:
- Check `GetKey` and `HasPrivKey` after adding descriptor key.
- Cover `GetEndRange` and `GetKeyPoolSize`.
- Cover `MarkUnusedAddresses` with the scripts from ScriptPubKeys and `GetMetadata` with the destinations from them.
ACKs for top commit:
marcofleon:
Tested ACK e3249f2111. I ran the updated harness for ~9 hours on an empty corpus, generated a coverage report, and checked that the new functions mentioned were hit. Coverage of `scriptpubkeyman.cpp` increased.
murchandamus:
Tested ACK e3249f2111
Tree-SHA512: cfab91f6c8401174842e79209c0e9225c08f011fe9b41d0a58bcec716ae4545eaf803867f899ed7b5fbcefea45711f91894e36df082ba19732dd310cd9e61a79
For example:
```bash
ADDITIONAL_GUIX_COMMON_FLAGS set in the ENV
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS="--emulate-fhs" ./contrib/guix/guix-build
<snip>
INFO: Building f75199182133 for platform triple x86_64-linux-gnu:
...using reference timestamp: 1716905119
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-f75199182133/distsrc-f75199182133-x86_64-linux-gnu'
...bind-mounted in container to: '/distsrc-base/distsrc-f75199182133-x86_64-linux-gnu'
...outputting in: '/bitcoin/guix-build-f75199182133/output/x86_64-linux-gnu'
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
ADDITIONAL FLAGS (if set)
ADDITIONAL_GUIX_COMMON_FLAGS: --no-substitutes
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS: --emulate-fhs
ADDITIONAL_GUIX_TIMEMACHINE_FLAGS:
```
Currently, when the FuzzedDataProvider of a FuzzedSock runs out of data,
FuzzedSock::Wait and WaitMany will simulate endless waiting as the
requested events are never simulated as occured.
Fix this by simulating event occurence when ConsumeBool() returns false
(e.g. when the data provider runs out).
Co-authored-by: dergoegge <n.goeggi@gmail.com>
FuzzedSock only supports peeking at one byte at a time, which is not
fuzzer friendly when trying to receive long data.
Fix this by supporting peek data of arbitrary length instead of only one
byte.
cbd4640ede build: remove --enable-lcov-branch-coverage (fanquake)
Pull request description:
This supports lcov `2.x` in the sense that we are no-longer hardcoding version specific options, and instead will use the `LCOV_OPTS` variable (which is the more flexible thing to do in any case). It's also quite likely that devs are already having to pass extra options to lcov `2.x`, given it's more stringent in terms of coverage generation and error checking. See this thread for an example: https://github.com/linux-test-project/lcov/issues/238.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
Related to #28468.
ACKs for top commit:
theuni:
utACK cbd4640ede
hebasto:
ACK cbd4640ede, tested on Ubuntu 22.04.
Tree-SHA512: 94eb01e0e236a480052749f6107b1d0d2e4f6f70a8eefd55fa9ba3d2f72996c9e8a0f28340698b7ac82e7a71e9cf799b7a53ddb6e435e5e9795f5f98a18820f7
8defc182a3 scripted-diff: Replace nNextSweep with m_next_sweep (marcofleon)
0048680467 increase txorphan harness stability (marcofleon)
Pull request description:
This moves `nNextSweep` from being a static variable in `LimitOrphans` to being a member of the `TxOrphanage` class. This improves the stability of the `txorphan` fuzz harness, as each orphanage (created every iteration) now has its own value for `nNextSweep`.
ACKs for top commit:
maflcko:
utACK 8defc182a3
dergoegge:
Code review ACK 8defc182a3
glozow:
utACK 8defc182a3, I can rebase on this pretty easily
Tree-SHA512: 54d4a5074def764f6c895308b94e417662d2f21f157925421131745f22743907df59971f4ce545063658cd74ec133792cdd8df96ae3e69af8314e9b0ff899d48
a27e1ceb9f depends: consolidate dependency docs (fanquake)
Pull request description:
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```
`xz-utils` was also missing (but generally already installed), and is needed for the `.tar.xz` tarballs.
Remove `bsdmainutils`, as this is only needed by the main build system (for tests), and isn't needed to complete a depends build.
ACKs for top commit:
maflcko:
ACK a27e1ceb9f
Tree-SHA512: 720c31d4d4c9b86fda4aace405d528193714dd3e526f38d5b8a83e4b676a433b9c891f01d86d673be9ac848458eda8a89b0981003a42eaa6d97bacc2e914396a
0a3631fc35 depends: fix Qt macOS build with Clang 18 (fanquake)
b018bd779d depends: qt 5.15.14 (fanquake)
Pull request description:
Also adds a patch to Qts internal libpng, to fix compilation using Clang 18, when targetting macOS. I'd like to get this patched, so we can continue working on removing `FORCE_USE_SYSTEM_CLANG` (#30201); otherwise builds will be broken using the default Clang (`18`) on the current Ubuntu LTS (`24.04`).
With this PR, anyone using Ubuntu 24.04 should be able to `apt install clang llvm lld`, and then cross-compile for macOS using:
```bash
# clang --version
Ubuntu clang version 18.1.3 (1)
make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1
./autogen.sh
CONFIG_SITE=/path/to/depends/arm64-apple-darwin/share/config.site ./configure
make
# file src/qt/bitcoin-qt
src/qt/bitcoin-qt: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|WEAK_DEFINES|BINDS_TO_WEAK|PIE|HAS_TLV_DESCRIPTORS>
```
ACKs for top commit:
TheCharlatan:
ACK 0a3631fc35
theuni:
utACK 0a3631fc35
hebasto:
ACK 0a3631fc35, a new patch indeed fixes cross-compiling on Ubuntu 24.04 with `FORCE_USE_SYSTEM_CLANG=1`.
Tree-SHA512: 711d321b1efbb1aeef802d1d7e72fff8f4e28aa2420d19df9db6f4449fc7d281e1d08ba242ce20122dfe21129e713bd59e7e6ade0b67d7271eea18b39ceb9283
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```
`xz-utils` was also missing (but generally already installed), and is
needed for the `.tar.xz` tarballs.
Remove bsdmainutils, as this is only needed by the main build system
(for tests), and isn't needed to complete a depends build.
4b7d984269 lint: add markdown hyperlink checker (willcl-ark)
Pull request description:
Potential followup to: #30025
This should prevent us reintroducing broken markdown links.
It does not test "online" (external) links, only those within this repo. Both relative and absolute links are parsed successfully if they resolve.
ACKs for top commit:
maflcko:
re-utACK 4b7d984269
davidgumberg:
reACK 4b7d984269
Tree-SHA512: 9bc40d700b73499c046bb76157bc139f32ec3850f64ef813bbf7f18f9c01a253abe6a857d6f559890165f2bd26e7742c05d86232cd9b8efb33ff85d735f4f095
Patch Qts internal libpng to resolve the failure.
I would like to have this patched, so we can continue working on the
removal of `FORCE_USE_SYSTEM_CLANG`. Otherwise builds will be broken using
the default clang (18) on the current Ubuntu LTS (24.04).
This supports lcov 2.x in the sense that we are no-longer hardcoding
version specific options, and instead will use the `LCOV_OPTS` variable
(which is the more correct/flexible thing to do in any case). It's also
quite likely that devs are already having to pass extra options to lcov
2.x, given it's more stringent in terms of coverage generation and error
checking. See this thread for an example:
https://github.com/linux-test-project/lcov/issues/238.
Added an example to the developer notes.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
5deb0b024e build, test, doc: Temporarily remove Android-related stuff (Hennadii Stepanov)
Pull request description:
Previously, our Android builds were geared towards generating APKs, which relied on Qt. However, after migrating to C++20, compiling for Android became unfeasible due to Qt 5.15's compatibility limitations with NDK only up to r25, which includes an outdated embedded libc++ (see https://github.com/bitcoin/bitcoin/issues/29360).
All removed stuff will be reinstated after migrating the build system to CMake and upgrading Qt to version 6.x.
This PR makes possible a clean migration to the CMake-based build system as it removes code, which is not used at this moment.
ACKs for top commit:
vasild:
ACK 5deb0b024e
fanquake:
ACK 5deb0b024e - given none of this is currently tested/wont compile. Can be revisted in future.
Tree-SHA512: 3bc2ccfe881e11cc1d78c27acd6f1d86cfba86821ef3bb5eca2e80d978fdfa13659ec82284dcaadc507e2394524dea91d4b8f81d0030c1cef9708df8be76bf07
7c8abf3c20 bench: bugfix, properly release wallet before erasing directory (furszy)
Pull request description:
Simple fix for #29816.
Since the wallet is appended to the global `WalletContext` during
creation, merely calling `reset()` on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet object results in keeping the db connection open, which
was causes the `fs::remove_all()` failure on Windows.
ACKs for top commit:
maflcko:
utACK 7c8abf3c20
kevkevinpal:
utACK [7c8abf3](7c8abf3c20)
hebasto:
re-ACK 7c8abf3c20, I agree with changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061694682).
Tree-SHA512: 279df65bea8f7aa02af0a2efed62dca9bf9b29cb748eb369c602d223e08a8a907dea7b1bffbd3dab91b1656c1d91b18a9a0534bc3f153bd751414b0e6230b3a4
9ddf39dd87 fuzz: Handle missing BDBRO errors (Ava Chow)
Pull request description:
Adds error messages that were not being handled. Also removes error messages that no longer exist.
Fixes#30166
ACKs for top commit:
dergoegge:
reACK 9ddf39dd87
TheCharlatan:
ACK 9ddf39dd87
Tree-SHA512: 2597536a1e5d030653dfcb02fd892f7492f5a091def787f6cbd421b8bca9544847684a498e9458ea99ae7de5a8a6d91532ff904d1e39222d324939d31d2eb3f0
