31e59d94c6 iwyu: Drop backported mapping (Hennadii Stepanov)
fe9bc5abef ci: Update Clang in "tidy" job (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.
New "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been addressed.
ACKs for top commit:
maflcko:
lgtm ACK 31e59d94c6
l0rinc:
ACK 31e59d94c6
theuni:
ACK 31e59d94c6
Tree-SHA512: ae0ca150673e1bfa78664f2ef35dbc965094b32374cafeeae390c6d368c28169a7f7790debe9a6eeb5efc39c9a468f5032d92f30cc4032b09d8265f6a75de882
This change switches to the latest IWYU 0.23, which is compatible with
Clang 19.
Fixed new "modernize-use-starts-ends-with" warnings.
The new "bugprone-use-after-move" warning in `result_tests.cpp` is a
false positive caused by a bug in Boost.Test versions < 1.87. This has
been addressed by introducing a local variable.
See upstream references:
- Issue: https://github.com/boostorg/test/issues/343
- Fix: https://github.com/boostorg/test/pull/348
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Fixes: #31044
This MLC update includes a change which will ignore files being ignored
by git, and help avoid false-positives when linting in this repo.
fa5e706459 ci: Skip broken Wine64 tests by default (MarcoFalke)
Pull request description:
I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.
Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.
A follow-up could run them on real Windows, see https://github.com/bitcoin/bitcoin/pull/31176.
Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.
ACKs for top commit:
hebasto:
ACK fa5e706459, to avoid false-positives.
willcl-ark:
ACK fa5e706459
Tree-SHA512: 6bd54470e4d5ce18923c5d724aba0dbf475d053d7097d3f87e822a455cc537b6ce5f0dfcc8ccd0719c12c5d0c8fc7355a6c84185a6c9b5d484d98aee763d0c49
b031b7910d [ci] Split out native fuzz jobs for macOS and windows (dergoegge)
Pull request description:
Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.
In both jobs the fuzz binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.
ACKs for top commit:
maflcko:
re-lgtm ACK b031b7910d
achow101:
ACK b031b7910d
hebasto:
ACK b031b7910d.
Tree-SHA512: 7d0dc5a9cb299f6f4596dd9a5526b6aaf82efc6eba308bdc9d8b0a45f79dea87204fb6cd4b2ea2a1bd952466b2e958d64021999296d110d7a83c1667f4de51fe
This is mentioned in the developer notes, but isn't present in
`TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures
occur. Checked that adding does output more information.
fabe90c824 ci: Use clang-19 from apt.llvm.org (MarcoFalke)
Pull request description:
A new clang version generally comes with bugfixes, new sanitizer features, deprecations, as well as new features.
Upgrade the sanitizer tasks to use the new version.
ACKs for top commit:
TheCharlatan:
ACK fabe90c824
hebasto:
ACK fabe90c824, I have reviewed the code and it looks OK.
Tree-SHA512: 637452e3cbc8ad97a3650976e2dbf4bcd032f2c454e12b48514eb5d252b3e106448674fe2d3bf45d11c0782874250d7a79e34fbb0aaa990499a156fcafd50329
40e5f26a3f mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb mapport: drop outdated comments (Antoine Poinsot)
b7b2435290 doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da depends: drop miniupnpc (Antoine Poinsot)
953533d021 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482 ci: remove UPnP options (Antoine Poinsot)
a9598e5eaa build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385 interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20 daemon: remove UPnP support (Antoine Poinsot)
844770b05e qt: remove UPnP settings (Antoine Poinsot)
Pull request description:
This PR removes UPnP IGD support and drops our [miniupnp](https://github.com/miniupnp/miniupnp) dependency.
Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed [here](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=miniupnp)), some of which directly affected our software ([RCE in 2015](https://bitcoincore.org/en/2024/07/03/disclose_upnp_rce/), [OOM in 2020](https://bitcoincore.org/en/2024/07/31/disclose-upnp-oom/)).
The main purpose of this functionality is to have more (non-data-center) reachable nodes on the network. For a non-technical user running Bitcoin Core at home, the software would automatically open a port on their router to receive incoming connections. This way, users not able to manually open a port on their router would still provide the network with more resources and enhance its diversity.
However, due to past vulnerabilities (and a worry about unknown future ones) in miniupnpc this feature was disabled by default in https://github.com/bitcoin/bitcoin/pull/6795. Having it disabled by default kills (most of?) the purpose of having this functionality in the first place: someone technical enough to understand the `-upnp` startup option or the "enable UPnP" setting is most likely able to open a port on his box in the first place.
In addition, laanwj implemented PCP with a NAT-PMP fallback directly in Bitcoin Core in https://github.com/bitcoin/bitcoin/pull/30043. If we ever want to re-enable automatic NAT traversal by default in Bitcoin Core, this is the best option (and in my opinion the only sane one). The NAT-PMP fallback makes it so compatibility shouldn't be (much of) an issue.
On balance, i believe that keeping this functionality and this barely maintained C dependency has higher costs than benefits. Therefore i propose that we get rid of it.
ACKs for top commit:
jarolrod:
ACK 40e5f26a3f
1440000bytes:
Code Review ACK 40e5f26a3f
laanwj:
Code review ACK 40e5f26a3f
i-am-yuvi:
Tested ACK 40e5f26a3f
Tree-SHA512: 9ea48662775510f5ec6de7af65790f7c8d211603398e9d8c634a86387be81b28081419a95b4d6680d3d7fe6a9f16cec99f16516548201dc7e49781909899a657
fa71bedf86 ci: Approximate MAKEJOBS in image build phase (MarcoFalke)
Pull request description:
The `MAKEJOBS` env var is the default in image builds, which is fine, because it is only relevant when building msan (or iwyu) and only differs when setting MAKEJOBS to something other than `nproc` (currently used as an approximation).
So the normal workflow of `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` already works today.
However, `MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` does not.
This is hard to fix, because making the env var a build arg means that changing it (and only it) requires a new (expensive and redundant) build.
So add an option `HAVE_CGROUP_CPUSET`, which can be set to approximate `MAKEJOBS` a bit. Can be tested via:
`HAVE_CGROUP_CPUSET=yo MAKEJOBS="-j_something" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh`
ACKs for top commit:
fanquake:
ACK fa71bedf86
Tree-SHA512: 43ef194c71d726f4cfa3fe08a5894c7872150f37da7e4fa0c2d89e4572bc63acadb5dae3286a5e5cc14a8ce3e1ebcc14571f1a3541e8db2d18d2f7503764a2f3
fa1b139d17 Bump python minimum supported version to 3.10 (MarcoFalke)
Pull request description:
All supported operating systems ship with python 3.10 (or later), so bumping the minimum should not cause any issues. A bump will allow new code to use new python features.
For reference:
* https://packages.debian.org/bookworm/python3
* https://packages.ubuntu.com/jammy/python3
* FreeBSD 13/14 ships with 3.11
* CentOS-like 8/9 ships with 3.11/3.12 (via `appstream`)
* OpenSuse Tumbleweed ships with all python versions, e.g. https://software.opensuse.org/package/python312-base
This is for Bitcoin Core 29.0 in 2025 (next year), not the soon upcoming 28.0 this fall.
ACKs for top commit:
achow101:
ACK fa1b139d17
AngusP:
ACK fa1b139d17
l0rinc:
ACK fa1b139d17
stickies-v:
ACK fa1b139d17
Tree-SHA512: 910b202ff2374bb21c93e5249a151fd2c8f63759bed5659676b0e467afa6e8e977be797c3fccceca303c82575e11ec236a8d7c5880910e4314b3875b820e7e8a
fa1cffacae ci: Install missing nproc in macos task (MarcoFalke)
faf7a2bccc ci: Add missing -DWERROR=ON to test-each-commit (MarcoFalke)
Pull request description:
Found by in Sjors in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610 (Thanks!)
Also, includes an unrelated commit to simplify the ci scripts, by assuming nproc exists on macos as well. (Having more than one commit is also required to trigger the `test-each-commit` task)
ACKs for top commit:
theuni:
utACK fa1cffacae.
Tree-SHA512: 881df6964da31c3f7161b763bdf74029743567272c7a4db24ded10909a8cedad39b494a9f486f0b059bbeb0beb190dcd91369650d014e1fc3b5f7ae07cd54771
This should be long enough (with headroom) for our longest running tests,
which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` under TSAN:
```bash
tests ................................ Passed 795.20 sec
```
and MSAN:
```bash
tests ................................ Passed 658.48 sec
```
This will also prevent the current issue we are seeing of `ctest`
running until it reaches the CI timeout, see #30969.
However, we still need to figure out what underlying issue is causing
the tests to (sometimes) run for so long, but in the mean time, this
will stop `ctest` wasting our CI CPU.
5c7cacf649 ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da doc: Remove mention of natpmp build options (laanwj)
061c3e32a2 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa build: Drop libnatpmp from build system (laanwj)
7b04709862 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c6 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cd net: Add PCP and NATPMP implementation (laanwj)
d72df63d16 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b net: Add netif utility (laanwj)
754e425438 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649
achow101:
ACK 5c7cacf649
vasild:
ACK 5c7cacf649
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
fa99e4521b ci: Allow CCACHE_DIR bind mount (MarcoFalke)
fa252da0b9 ci: Remove hardcoded CCACHE_DIR in cirrus (MarcoFalke)
fa146904e1 ci: Bump default CCACHE_MAXSIZE to 500M (MarcoFalke)
aaaa7cf8ba cirrus: Drop CCACHE_NOHASHDIR (MarcoFalke)
fa7ca182a9 ci: Print inner env (MarcoFalke)
Pull request description:
The outer env is printed when the `/tmp/env-$USER-$CONTAINER_NAME` is created. There is also a separate env printed when building the container image (usually with default values).
To confirm that the inner container env is correctly derived from the outer env, and not from the default build env, print it a third time.
ACKs for top commit:
l0rinc:
utACK fa99e4521b
willcl-ark:
ACK fa99e4521b
Tree-SHA512: 59fd3262d551e09224866e31c14ca865461e81abbe00b83391fe3a9c7ada30fd2fd0272e4aa812df2712433ac7594d1a55cf674248b341359cec09c8d3f0c58b
ccccb67851 ci: Use clang-19 in msan tasks (MarcoFalke)
Pull request description:
A new clang version generally comes with bugfixes, new sanitizer features, deprecations, as well as new features.
Upgrade the memory sanitizer tasks to use the new version.
(Ref https://github.com/bitcoin/bitcoin/pull/30634)
ACKs for top commit:
fanquake:
ACK ccccb67851 Tested both jobs on aarch64, and one on x86_64 with `mmap_rnd_bits`.
Tree-SHA512: a42bf2da7c08aa54c0c5ab3811ff51b98b80b276be135eed32395a55ae93a42d41d7cd32c307062dcca711a892958ea141168c2a06025560074f8c5d20190946
This makes it easier to overwrite the value.
Also, drop the dot in the CCACHE_DIR fallback value, because the folder
in the scratch dir does not need and probably should not be hidden.
3ae35b427f ci: run check-deps.sh as part of clang-tidy job (Ryan Ofsky)
0aaa1298a0 contrib: fix check-deps.sh when libraries do not import symbols (Ryan Ofsky)
3c99f5a38a contrib: fix check-deps.sh to check for weak symbols (Ryan Ofsky)
86c80e9cf2 contrib: make check-deps.sh script work with cmake (Ryan Ofsky)
Pull request description:
Fix check-deps.sh to check for weak symbols so it can detect when an exported template function like is used from another library.
Also update the script to work with cmake and configure it to run as part of CI.
Problem was reported by hebasto in https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
ACKs for top commit:
TheCharlatan:
Re-ACK 3ae35b427f
hebasto:
ACK 3ae35b427f, I have reviewed the code and it looks OK. Also I've tested it locally.
Tree-SHA512: c3b58175450b675e6e848549b81bcfe42930ea9bcd693063867ce3f0ac3999c98cd2c3e961f163ff06641e8288f3a4e81530936a296a83d45d33364f27489521
8131bf7483 ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa0 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483
achow101:
ACK 8131bf7483
hebasto:
ACK 8131bf7483, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
fafdb7df34 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
7777047835 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f0 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69 test: [refactor] Fix E714 pycodestyle (MarcoFalke)
Pull request description:
The checks have many issues:
* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds
ACKs for top commit:
davidgumberg:
review and tested reACK fafdb7df34
kevkevinpal:
ACK [fafdb7d](fafdb7df34)
achow101:
ACK fafdb7df34
Tree-SHA512: a0488b722cfaf7071bd6848cd3be002e0b6c38af80d8b5cbb08613c0b174ef63277289f960db8ac31adb09fe563a4973203b8fb10b83cbcfdc6f0ef39bd04410
faa382ae76 ci, doc: Drop reference to `src/.bear-tidy-config` (Hennadii Stepanov)
d71ac76842 build: Remove Autotools-based build system (Hennadii Stepanov)
e268b48419 doc: Adjust `doc/design/libraries.md` (Hennadii Stepanov)
d209e4f156 doc: Drop mentions of `share/genbuild.sh` (Hennadii Stepanov)
Pull request description:
This PR deletes the Autotools-based build system.
The MSVC build system is deleted in https://github.com/bitcoin/bitcoin/pull/30731.
ACKs for top commit:
maflcko:
re-ACK faa382ae76🍦
TheCharlatan:
ACK faa382ae76
fanquake:
ACK faa382ae76
Tree-SHA512: 53df977b5b199a1c38f7f61a042a62b24831c559ba65a461b4ac1c96a1a56e2dfd676df79f1358fd1cc1749ff27e7b548086157f337d4f596c1054cb3d2d5739
