fab300b378941a233119805c0d62198596a57790 test: Enable ruff E713 lint (MarcoFalke)
Pull request description:
Membership tests of the form `not item in stuff` may be confusing, because they could be read as `(not item) in stuff`, which is different.
So enable the ruff E713 lint, which should also help to avoid having to go through review cycles for this.
ACKs for top commit:
bensig:
ACK fab300b378941a233119805c0d62198596a57790
l0rinc:
ACK fab300b378941a233119805c0d62198596a57790
rkrux:
lgtm crACK fab300b378941a233119805c0d62198596a57790
Tree-SHA512: c3eaf0fbe0dd22d8e04b896e98adaf28162fb748e6f7f5ebfd73b2020da66046bf8f0c1a27db5da05250366b98ded8c4a55d53edd8fa050e80521aee42ba3c5a
56750c4f87d089c6a3f093eb2bf2edd07170d4a8 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e182853ce44d9fd63d24ee6cab5223e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7cf4b0fef9a28d3771e73f1dc9fb0528 cmake: Fix target name (Hennadii Stepanov)
0f81e005197fa4201a38e635ddf8c5dcc12a3878 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb1e225099223a355d88da0522d7d69b iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23706e4820392dc456c3acccbf196dd6 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄
sedited:
ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
fa4cb13b52030c2e55c6bea170649ab69d75f758 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f29774872d18febc0df38831a6e45f3de69cc scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a9921235d1ecf8c3c7dc1836ec68131) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
rkrux:
re-ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
janb84:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
e7e51952dc24531932b6c06e4599be3a3d6bede8 contrib: Avoid outputting binary data to TTY (Hodlinator)
Pull request description:
Verify that we wouldn't be writing encoded asmap binary data directly to the TTY since it is the default but makes no sense. (Having stdout as default does make sense when piping to other applications however).
Found while exploring the ASMap data pipeline (https://github.com/asmap/asmap-data/pull/38#pullrequestreview-3547352533) from Kartograf into Bitcoin Core.
ACKs for top commit:
fjahr:
tACK e7e51952dc24531932b6c06e4599be3a3d6bede8
sipa:
ACK e7e51952dc24531932b6c06e4599be3a3d6bede8
Tree-SHA512: e1ae1ee129715471cbb824268e68cec267d159d4073297af35c06eadfb6b98eeae040beaafeb6489c2853ea9b83cd04471bcd0b27f0ae8fcb377e6e10b4ae6c5
41e657aacfa605ac2de08ef26f2b536810f3e55a guix: add bitcoin-qt runtime libs doc in symbol-check (fanquake)
ef4ce19a1545d81dcf95b07e04628557a57451e2 depends: freetype 2.11.1 (fanquake)
Pull request description:
Update freetype to `2.11.1`.
Updating fontconfig (currently `2.12.6`) to `2.13.1` requires what looks like a hard dep on gperf; leaving that as-is for now.
Document expectations in `symbol-check.py`.
Closes#29977 (changes are based on discussion there).
ACKs for top commit:
sedited:
ACK 41e657aacfa6
Tree-SHA512: 71c4ccc442df0b90bebc475003eb325564111b8312c42bc7d7a9c81a2fc166fdc0814c9ddde3cfe562c3c835556e7f97107458b02a07b981b1a199bf65d5ac1d
7b90b4f5bb10e2156709b07e3996f867e2421232 guix: reduce allowed exported symbols (fanquake)
Pull request description:
Need to double-check, but pretty sure this is atleast partly from #33181.
ACKs for top commit:
sedited:
Nice, ACK 7b90b4f5bb10e2156709b07e3996f867e2421232
Tree-SHA512: 538c03dc32aab9b3e18100e8ffa0d664aea5ceba6aafee9e8e0894c2d02eea3b3fb09733cf7b5bd0aefb6b56d0ac3b92f28da932e135b23f55404efd8f43664a
710031ebef838d2f0a1effa19170bef7b130bbeb Revert "guix: sqlite wants tcl" (Hennadii Stepanov)
4cf5ea6c3d2a5990aafd59ea5137d99f050840f4 depends: Propagate native C compiler to `sqlite` package (Hennadii Stepanov)
Pull request description:
This PR:
1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system.
2. Removes the `tcl` package from the Guix manifest.
This is an alternative to https://github.com/bitcoin/bitcoin/pull/33975.
ACKs for top commit:
fanquake:
ACK 710031ebef838d2f0a1effa19170bef7b130bbeb
sedited:
ACK 710031ebef838d2f0a1effa19170bef7b130bbeb
Tree-SHA512: bdaa29af977799669bfc2aa3a8d0a4a688263b99c5f06b1582fbefb71ef77be0ee6223903e8357e51a9e0a7744807174b94262c2f4a3afd9f39737b61b00863e
fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8 contrib: Count entry differences in asmap-tool diff summary (Fabian Jahr)
Pull request description:
Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list.
Example output currently:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed
```
This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugging of certain issues also the more relevant value. This PR adds the count of changed entries to this summary output at the end. There as also a bit more structure so it's easier to parse as well.
Example new output:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# Summary
IPv4: 0 entries with 0 addresses changed
IPv6: 12 entries with 79552154633921058212365205504 (2^96.01) addresses changed
```
ACKs for top commit:
jurraca:
utACK [`fd4ce55121`](fd4ce55121)
janb84:
utACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8
hodlinator:
ACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8
Tree-SHA512: 97cc543eaba80a33f0291b20630411bda869d3b8d1b35ed7f36792064cb1edccc8fe4740b7229b5451a88b7bd8d68c42f96829ce4255ecac3e29d70b68061608
b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e Remove unreliable seed from chainparams.cpp, and the associated README (SatsAndSports)
Pull request description:
The DNS seed `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` is not returning a representative sample of bitcoin nodes. It currently returns nothing later than 28.1.0, breaching the policy.
This PR removes that seed from the list of DNS seeds
### Rationale
The [policy for seeds](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) includes this:
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network
A number of comments below, in response to this PR, include apparent breaches of this policy: [1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231) [2](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457655364), [3](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457712557), in particular the first linked comment ([1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231)) comparing the distribution at this seed to other seeds. This seed is not including anything later than 28.2.0, breaching this policy.
To ensure the policy is followed, and the seeds include a representative sample of Bitcoin nodes, this PR removes this seed from the list
### Data
I ran this:
```
# Get some ip address from that seed:
# Repeated multiple times, to get many different IPs:
dig +short dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us >> dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us
# For each distinct ip gathered from the seed, get basic info about the node, including it's User Agent string:
cat dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us | sort -u | while read ip; do echo ===; echo $ip; nmap -p 8333 --script bitcoin-info "$ip"; done > seed_versions.txt
```
and then summarized the agents with `egrep 'User Agent' seed_versions.txt | sort | uniq -c` and got:
```
1 User Agent: /Satoshi:22.0.0/
1 User Agent: /Satoshi:22.1.0/
5 User Agent: /Satoshi:24.0.1/
1 User Agent: /Satoshi:25.1.0/
30 User Agent: /Satoshi:27.0.0/
1 User Agent: /Satoshi:27.1.0/
1 User Agent: /Satoshi:27.1.0/Knots:20240801/
1 User Agent: /Satoshi:28.0.0/
7 User Agent: /Satoshi:28.1.0/
2 User Agent: /Satoshi:28.1.0/Knots:20250305/
```
ACKs for top commit:
l0rinc:
reACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
delta1:
reACK b0c706795c
Crypt-iQ:
crACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
laanwj:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
murchandamus:
ACK b0c706795c
RandyMcMillan:
ACK b0c7067
wiz:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
dergoegge:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
stickies-v:
re-ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
mzumsande:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
instagibbs:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage
generation, because the assumption that the last word in the line
containing the version number, was the version number, no-longer holds
for some binaries. i.e bitcoind.
fad61185861a6a9ed806c387aa63d2b31262b1db test: Fix "typo" in written invalid content (MarcoFalke)
fab085c15f7221986f73af7e05e799edf3eadaf0 contrib: Use text=True in subprocess over manual encoding handling (MarcoFalke)
fa71c15f8610816a6ee0426cd396315da3d27c30 scripted-diff: Bump copyright headers after encoding changes (MarcoFalke)
fae612424b3e70acd6011a4459518174463b3424 contrib: Remove confusing and redundant encoding from IO (MarcoFalke)
fa7d72bd1be9a45e8c09525aee68caad1e57963e lint: Drop check to enforce encoding to be specified in Python scripts (MarcoFalke)
faf39d8539c9d563f68071054bbd533157f586ef test: Clarify that Python UTF-8 mode is the default today for most systems (MarcoFalke)
fa83e3a81ddb2170a0d7b0d86b94641a80d026ee lint: Do not allow locale dependent shell scripts (MarcoFalke)
Pull request description:
Historically, there was an attempt via `test/lint/lint-python-utf8-encoding.py` to enforce explicit UTF8 in every Python IO statement (`open`, `subprocess`, ...). However, the lint check has many problems:
* The check is incomplete and many IO statements lack the explicit UTF8 specification.
* It was added at a time when some systems were not UTF8 by default.
* The check is brittle, as it depends on a fragile regex.
In theory, now that the minimum Python version is 3.10 (since commit 2123c94448ed142e78942421c597a1f264859c48), the check could be replaced by `PYTHONWARNDEFAULTENCODING=1` from https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option. However, this comes with many other problems:
* All our Python scripts already assume and require UTF8 to be set externally. On almost all modern systems, this is already the default. Some Windows versions do not have UTF8 by default and require `PYTHONUTF8=1` to be set for the tests to run already today (with or without the changes in this pull). Also, the CI and many other Bash scripts force UTF8 via `LC_ALL`. Finally, Python 3.15 will likely enable UTF8 on *all* systems by default, per https://peps.python.org/pep-0686/#abstract.
* So adding UTF8 to every single IO call is redundant, verbose, and confusing, given that it is the expected default.
So fix all issues, by:
* Removing the `test/lint/lint-python-utf8-encoding.py` check.
* Removing the encoding on the individual IO calls.
* Clarifying the existing docs around the existing UTF8 requirement and assumption.
Obviously, every IO call is still free to specify UTF8 or any other encoding explicitly, if there is a documented need for it in the future.
ACKs for top commit:
theStack:
re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db
laanwj:
Re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db
Tree-SHA512: 78025ea3508597d2299490347614f0ee3e4c66e3ba559ff50e498045a9c8bbd92f3a5ced18719d8fcebbd1e47bdbb56a0c85a5b73b425adb0ea4f02fe69c3149
libfreetype and libfontconfig are our two remaining runtime libs for
bitcoin-qt. According to #29977 Ubuntu 22.04 should be considered the
baseline for what is supported. Document that.
Closes#29977.
Starting with Python 3.11, Pythons gzip might delegate to zlib.
Depending on the OS, i.e Ubuntu vs Fedora, the underlying zlib
implementation might differ, resulting in different output.
For now, or until a better solution exists, disable compression. This
results in the SDK increasing in size to ~157mb. Which is not
unreasonable, to regain determinism (and would be significantly worse
without the previous commit).
See: https://docs.python.org/3/library/gzip.html#gzip.compress
Co-authored-by: stickies-v <stickies-v@protonmail.com>
All touched Python scripts already assume and require UTF8, so manually
specifying encoding or decoding for functions in the subprocess module
is redundant to just using text=True, which exists since Python 3.7
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
The encoding arg is confusing, because it is not applied consistently
for all IO.
Also, it is useless, as the majority of files are ASCII encoded, which
are fine to encode and decode with any mode.
Moreover, UTF-8 is already required for most scripts to work properly,
so setting the encoding twice is redundant.
So remove the encoding from most IO. It would be fine to remove from all
IO, however I kept it for two files:
* contrib/asmap/asmap-tool.py: This specifically looks for utf-8
encoding errors, so it makes sense to sepecify the utf-8 encoding
explicitly.
* test/functional/test_framework/test_node.py: Reading the debug log in
text mode specifically counts the utf-8 characters (not bytes), so it
makes sense to specify the utf-8 encoding explicitly.
2594d5a189e52052c2019faccaa47f2affdc48e1 build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings (Henry Romp)
Pull request description:
Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings that are no longer needed after reordering the Guix build script to perform binary checks after installation.
This PR also removes the unused CMake maintenance targets (`check-security` and `check-symbols`) and updates the Guix security checks to include binaries in the `libexec/` directory (added in PR #31679).
ACKs for top commit:
purpleKarrot:
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1
hebasto:
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1.
Tree-SHA512: ed451a298f5aae05c177b0033b092faaa7536caeaa3d84da9b8b611e2aa905e1dd337e57aef0efd69ce6ce6ac0cf77dc57adf175079b95bf53dd96d5d0c8118b
Remove CMake settings that are no longer needed after reordering Guix build script to perform binary checks after installation.
Also removes unused CMake maintenance targets (check-security and check-symbols) and updates security checks to include libexec/ directory binaries (see PR #31679).
This causes issues when building against newer glibcs (i.e 2.42), and isn't needed
in any case.
```bash
../../../../gcc-14.3.0/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:483:31: error: invalid application of ‘sizeof’ to incomplete type ‘__sanitizer::termio’
483 | unsigned struct_termio_sz = sizeof(struct termio);
| ^~~~~~~~~~~~~~~~~~~~~
```
Extracted from #25573.
b4d0288c467f82a94041b51d10d38e66bb5c33ae doc: update Guix INSTALL.md (fanquake)
Pull request description:
It's somewhat annoying that Guix is falling out of being packaged by distros. For some more context, see https://lwn.net/Articles/1035491/.
> However, it is likely that the [Guix](https://guix.gnu.org/en/) package manager will soon be removed from the repositories for Debian 13 and Debian 12 ("bookworm", also called oldstable).
This seems to be happening. You can't `apt install guix` using the current release of Debian. https://packages.debian.org/search?keywords=guix. Guix is not going to be included in next release of Ubuntu (`25.10`): https://packages.ubuntu.com/search?keywords=guix.
Looking at https://aur.archlinux.org/packages/guix, comments over the last few months seem to indicate that the build is broken.
A 1.5.0 release is planned for sometime in January 2026: https://codeberg.org/guix/release-planning/wiki/release-1.5.0-project/. So hopefully the situation is going to improve in future.
ACKs for top commit:
willcl-ark:
ACK b4d0288c467f82a94041b51d10d38e66bb5c33ae
hebasto:
ACK b4d0288c467f82a94041b51d10d38e66bb5c33ae.
Tree-SHA512: 545f3529af82c18556ddfe104c01f77e28da31018a44047812450565a9b3bad3afa60f714b375c06c3a10aed722d54500846aa70a8069c5fe2d96b26d426b6c1
02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387bd26689aed7682b248b648dba42779 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc193f398ebad83911d3297b5257e721 refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2016ba2ae9cc40833447dff648af809 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)
Pull request description:
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
ACKs for top commit:
maflcko:
re-ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d 💮
ryanofsky:
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
willcl-ark:
ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d
Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6
59c4898994bde3d86168075f0031c9d5a9ac5c8f guix: remove python-pydantic-core input from LIEF (fanquake)
9f2a6927d3a9fc1ac536f8fb24a89582e39f24d6 guix: use Clang & LLVM 19 for macOS build (fanquake)
9570ddbec9cb20c268f78ff5e581a65e00864773 guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2 (fanquake)
7b5cc276aa0a7aeea7e535b0fd30a0b6811000d9 guix: patch around riscv issue with newer (2.40+) binutils (fanquake)
91b5cbaabbca49a8bd9df6da2506070b31482892 ci: use Debian Trixie for macOS cross job (fanquake)
Pull request description:
5cb84f2013 isn't super recent, but it's enough to get access to some newer packages, such as LLVM 19, and avoids having to add any further work arounds for things that we know are fixed later (i.e nsis). Once things upstream have stabilized a bit more (the `core-updates` branch was fairly recently merged), we could look at bumping to something newer.
Package updates:
(base) glibc 2.35 -> 2.39
binutils 2.38 -> 2.41
diffutils 3.8 -> 3.10
gawk 5.2.1 -> 5.3.0
git-minimal 2.45.2 -> 2.46.0
grep 3.8 -> 3.11
gzip 1.12 -> 1.13
linux-headers 6.1.106 -> 6.1.119
make 4.3 -> 4.4.1
xz 5.2.8 -> 5.4.5
CMake 3.30 becomes available.
Clang/LLVM 19 becomes available.
Could be used for #32764.
ACKs for top commit:
hebasto:
re-ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f.
willcl-ark:
ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f
Tree-SHA512: c44965d5a315e4c862f5e40d8e98c645713405fec72a61055f95b6c68b7d2dcc69a61a084e397a4556d4c1df18f1cfa7a905234643fe4a7df9c58d486e26c097
cc5dda1de333cf7aa10e2237ee2c9221f705dbd9 headerssync: Make HeadersSyncState more flexible and move constants (Hodlinator)
8fd1c2893e6768223069d8b2fdec033b026cb2eb test(headerssync): Test returning of pow_validated_headers behavior (Hodlinator)
7b00643ef5f932116ee303af9984312b27c040f1 test(headerssync): headers_sync_chainwork test improvements (Hodlinator)
04eeb9578c60ce5661f285f6bde996569fafdcc3 doc(test): Improve comments (Hodlinator)
fe896f8faa7883f33169fe3e6dddb91feaca23e1 refactor(test): Store HeadersSyncState on the stack (Hodlinator)
f03686892a9c07e87e6dd12027d988fe188b1f9e refactor(test): Break up headers_sync_state (Hodlinator)
e984618d0b9946dc11f1087adf22a4cfbf9c1a77 refactor(headerssync): Process spans of headers (Hodlinator)
a4ac9915a95eb865779cf4627dd518d94c01032b refactor(headerssync): Extract test constants ahead of breakup into functions (Hodlinator)
Pull request description:
### Background
As part of the release process we often run *contrib/devtools/headerssync-params.py* and increase the values of the constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` in *src/headerssync.cpp* as per *doc/release-process.md* (example: 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b). This helps fine tune the memory consumption per `HeadersSyncState`-instance in the face of malicious peers.
(The `REDOWNLOAD_BUFFER_SIZE`/`HEADER_COMMITMENT_PERIOD` ratio determines how many Headers Sync commitment bits must match between PRESYNC & REDOWNLOAD phases before we start permanently storing headers from a peer. For more details see comments in *src/headerssync.h* and *contrib/devtools/headerssync-params.py*).
### Problem: Not feeding back headers until completing sync
During v30 release process #33274 made `REDOWNLOAD_BUFFER_SIZE` exceed the `target_blocks` constant used to control the length of chains generated for testing Headers Sync (`15000`, *headers_sync_chainwork_tests.cpp*).
The `HeadersSyncState::m_redownloaded_headers`-buffer now does not reach the `REDOWNLOAD_BUFFER_SIZE`-threshold during those unit tests. As a consequence `HeadersSyncState::PopHeadersReadyForAcceptance()` will not start feeding back headers until the PoW threshold has been met. While this will not cause the unit test to start failing on master, it means we have gone from testing behavior that resembles mainnet (way more than `REDOWNLOAD_BUFFER_SIZE` headers to reach the PoW limit), to behavior that is not possible/expected there.
### Solution
Avoid testing this unrealistic condition of completing Headers Sync before reaching `REDOWNLOAD_BUFFER_SIZE` by making tests able to define their own values through the new `HeadersSyncParams` instead of having them hard-coded for all chains & tests.
### Commits
* First 6 commits refactor and improve the unit tests in order to clarify latter changes.
* We then add checks for the behavior around the `REDOWNLOAD_BUFFER_SIZE` threshold.
* The main change: we extract the section from *headerssync.cpp* containing the constants to *kernel/chainparams.cpp*, making `HeadersSyncState` no longer hard-coded to mainnet.
### Notes
This PR used to be called "headerssync: Preempt unrealistic unit test behavior".
ACKs for top commit:
l0rinc:
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
marcofleon:
code review ACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
danielabrozzoni:
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
Tree-SHA512: ccc824dcbbb8ad5ae98c3bf5808b38467aac0230739898a758c9b939eecd74f982df088fa0ba81cc1c1732f19a607b135a6e9577bb9fcf7f8570567ce92f66e6
