e646fa89f0 fix(ci): yet another 6356 follow-up (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
```
From https://github.com/dashpay/dash
* branch refs/pull/6348/head -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint: git config pull.rebase false # merge
hint: git config pull.rebase true # rebase
hint: git config pull.ff only # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.
```
https://github.com/dashpay/dash/actions/runs/11506422850/job/32030325600
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
Gosh, are you done yet? :D utACK e646fa89f0
Tree-SHA512: 533a2aaf73400fcb12b848d1496496667a1e7a89c17563b14f1c45c1624cff7d320563d6531d2b6b0e4b3cd638e2ff070bbd810bd88e75ce628de940b025fc12
9a79217756 Merge bitcoin/bitcoin#28227: test: check for specific bip157 disconnect reasons, add test coverage (fanquake)
a7a4603b8e Merge bitcoin-core/gui#755: Silence `-Wcast-function-type` warning (Hennadii Stepanov)
e216d0851d Merge bitcoin/bitcoin#27934: test: added coverage to estimatefee (fanquake)
34f0f56582 Merge bitcoin/bitcoin#28506: fuzz: Add missing PROVIDE_FUZZ_MAIN_FUNCTION guard to __AFL_FUZZ_INIT (fanquake)
ca0225c0fd Merge bitcoin/bitcoin#28480: fuzz: Don't use afl++ deferred forkserver mode (fanquake)
2b236ad07b Merge bitcoin/bitcoin#28460: fuzz: Use afl++ shared-memory fuzzing (fanquake)
52f036b316 Merge bitcoin/bitcoin#28427: index: coinstats reorg, fail when block cannot be reversed (fanquake)
6ad6f2f28d Merge bitcoin/bitcoin#28412: test: remove unused variables in `p2p_invalid_block` (fanquake)
43b88315e1 Merge bitcoin/bitcoin#28426: doc: s/--no-substitute/--no-substitutes in guix/INSTALL (fanquake)
1730a267ba Merge bitcoin/bitcoin#28386: test: remove fixed timeouts from feature_config_args (fanquake)
7f83db0d0c Merge bitcoin/bitcoin#28332: test: previous releases: speed up fetching sources with shallow clone (fanquake)
8490bf4b03 Merge bitcoin/bitcoin#28288: test: fix 'unknown named parameter' test in `wallet_basic` (fanquake)
8b8ff1c7d5 Merge bitcoin/bitcoin#28215: fuzz: fix a couple incorrect assertions in the `coins_view` target (fanquake)
c36f7d93fa Merge bitcoin/bitcoin#27401: tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings (fanquake)
163020ef92 Merge bitcoin/bitcoin#28203: refactor: serialization simplifications (fanquake)
24e57da770 Merge bitcoin/bitcoin#28181: qa, doc: Fix comment (fanquake)
933a63e8fc Merge bitcoin/bitcoin#28145: valgrind: add suppression for bug 472219 (fanquake)
33766805eb Merge bitcoin/bitcoin#28124: fuzz: Re-enable symbolize=1 in ASAN_OPTIONS (fanquake)
621061459a Merge bitcoin/bitcoin#28099: contrib: move user32.dll from bitcoind.exe libs (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 9a79217756
UdjinM6:
utACK 9a79217756
Tree-SHA512: 12d18abf28e3140bbb488fe912cf8e5c18a38aebaf09caad3120dc556116f348c4bbb40131f72c89edfbf48eb980c6947dde155e58e47c783f1d5b125aa6524b
7064eb9104 fix: 6356 follow-up (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
🙈
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
Happens :) utACK 7064eb9104
Tree-SHA512: 5c1a5f154ba5698eb89d57bffa369f106ed9d6248c02ff975bdde995dfbe4276140360ec7d2b5d45c32e82e9451aa8db99d80d21f680298311772e27caa410b3
BACKPORT NOTE:
Prior work is done here: c338cd69d4 (diff-060e8fd790fc1c3e18c64327a7395bb5b2d6d57db9792cc666bd8d7354a40c0b)
Missing changes in tx_verify.h included in this PR, but changes in
src/test/sigopcount_tests.cpp and src/test/transaction_tests.cpp are
irrelevant because they are segwit-related
fa621ededdfe31a200b77a8787de7e3d2e667aec refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes#22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
kristapsk:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
jonatack:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
c76e398caa ci: check ff after merge into base branch for PRs (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Check ff against the current state of PR's base branch instead of the one it had on PR creation.
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK; this makes sense to me c76e398caa
Tree-SHA512: ba68bc6454db81851e9356f27194e9f67a00935b51a0dba028902c8bed736e23021771220dcf45d2e1c350fa0705ecf3f0a4a78404dcab1152db837b611ac4e1
f8ca1357c8205ceff732dcfb0d2bad79b40b611b build: Fix check whether `-latomic` needed (Hennadii Stepanov)
Pull request description:
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
ACKs for top commit:
maflcko:
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
fanquake:
ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
Tree-SHA512: ba8b6a88fd3471a206d068e8a000a053c99cb46d26bd04624418ddb066b3b9664a569ec8a1569af67c96b3e27f13dccbd5e24f985290ac072b6d74c92524e35d
05aca093819be276ac7d648472c6ed5c7d235cc5 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov)
Pull request description:
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
/* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#endif
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/
```
In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#if defined(__has_builtin) && __has_builtin(__is_target_os)
#if __is_target_os(macos)
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif
#elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */
```
The latter macro is not provided by LLVM Clang until c8e2dd8c6f, which is available in Clang 17.
The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header.
ACKs for top commit:
maflcko:
lgtm ACK 05aca093819be276ac7d648472c6ed5c7d235cc5
Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743
821a8a11256ccf26fe8b0255cea4ec04dddd8f18 doc: remove x86_64 build assumption from depends doc (fanquake)
Pull request description:
This dates from the introduction of depends, and has not been the case for some time now.
ACKs for top commit:
maflcko:
lgtm ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18
hebasto:
ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18.
theuni:
ACK 821a8a11256ccf26fe8b0255cea4ec04dddd8f18
Tree-SHA512: 640967a3e6dfab495fd733d3379aa916ac7f67e89a92ef6a94c3bea0494dc7921a9d7485e1b90a1beab00548b575cdab8fb08eb9267dcc5e890cc796ae1b6875
30bd4b1e4aee00edbe77da7c20bf80e28f0561cc doc: remove mention of missing bdb being a configure error (fanquake)
Pull request description:
This is no-longer the case, unless you're passing additional flags, which is not the case in this example.
ACKs for top commit:
maflcko:
lgtm ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc
TheCharlatan:
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc
hebasto:
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
Tree-SHA512: b3730546d7ff1f49854b88e710c72c4f6e4b6d238147599d4c4e4adeeb256424c2096635f6c51dcfe2e5a9c1155c1c9915fe03a09c5c38605bee2722756c8f6e
ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7 doc: update docs for CHECK_ATOMIC macro (fanquake)
Pull request description:
Clarify that supported versions of GCC are not affected, and that Clang
prior to version 15 still requires the explicit `-latomic` linking, when
compiling for 32-bit.
ACKs for top commit:
hebasto:
ACK ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7.
Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
aee5404e02e203a256c1a97b629b9b107cc8bb07 Add support for RNDR/RNDRRS for aarch64 on Linux (John Moffett)
Pull request description:
This checks whether the ARMv8.5-A optional TRNG extensions [RNDR](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDR--Random-Number) and [RNDRRS](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDRRS--Reseeded-Random-Number) are available and, if they are, uses them for random entropy purposes.
They are nearly functionally identical to the x86 RDRAND/RDSEED extensions and are used in a similar manner.
Currently, there [appears to be](https://marcin.juszkiewicz.com.pl/download/tables/arm-socs.html) only one actual hardware implementation -- the Amazon Graviton 3. (See the `rnd` column in the link.) However, future hardware implementations may become available.
It's not possible to directly query for the capability in userspace, but the Linux kernel [added support](1a50ec0b3b) for querying the extension via `getauxval` in version 5.6 (in 2020), so this is limited to Linux-only for now.
Reviewers may want to launch any of the `c7g` instances from AWS to test the Graviton 3 hardware. Alternatively, QEMU emulates these opcodes for `aarch64` with CPU setting `max`.
Output from Graviton 3 hardware:
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:01:48Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:01:48Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:01:48Z Using RNDR and RNDRRS as additional entropy sources
2023-01-06T20:01:48Z Default data directory /home/ubuntu/.bitcoin
```
Graviton 2 (doesn't support extensions):
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:05:04Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:05:04Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:05:04Z Default data directory /home/ubuntu/.bitcoin
```
This partially closes#26796. As noted in that issue, OpenSSL [added support](https://github.com/openssl/openssl/pull/15361) for these extensions a little over a year ago.
ACKs for top commit:
achow101:
ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
laanwj:
Tested ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
Tree-SHA512: 1c1eb345d6690f5307a87e9bac8f06a0d1fdc7ca35db38fa22192510a44289a03252e4677dc7cbf731a27e6e3a9a4e42b6eb4149fe063bc1c905eb2536cdb1d3
a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Fee estimation: extend bucket ranges consistently (Anthony Towns)
Pull request description:
When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target.
Fixes#20725
ACKs for top commit:
ismaelsadeeq:
Code review ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
glozow:
btw what I meant by [this](https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1350258467) was ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
jonatack:
Initial ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
Tree-SHA512: 0edf4e56717c4ab8d4ab0bc0f1d7ab36a13b99de12f689e55c9142c6b81691367ffd8df2e8260c5e14335310b1a51770c6c22995db31109976239befcb558ef8
6bdff429ec17eae4138c3af1e21de3ec46f4ab13 build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (Hennadii Stepanov)
Pull request description:
The `ENABLE_TRACING` macro is expected to be defined in the `config/bitcoin-config.h` header.
Therefore, the current code is error-prone as it depends on whether the `config/bitcoin-config.h` header was included before or not.
This bug was noticed while working on CMake [stuff](https://github.com/hebasto/bitcoin/pull/37).
ACKs for top commit:
fanquake:
ACK 6bdff429ec17eae4138c3af1e21de3ec46f4ab13
Tree-SHA512: 22c4fdeb51628814050eb99a83db4268a4f3106207eeef918a07214bbc52f2b22490f6b05fcb96216f147afa4197c51102503738131e2583e750b6d195747a49
fac36b94ef32567c0f10b605a3a441d11559e56e refactor: Remove CBlockFileInfo::SetNull (MarcoFalke)
Pull request description:
Seems better to use C++11 member initializers and then let the compiler figure out how to construct objects of this class.
ACKs for top commit:
stickies-v:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
pablomartin4btc:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
theStack:
LGTM ACK fac36b94ef32567c0f10b605a3a441d11559e56e
Tree-SHA512: aee741c8f668f0e5b658fc83f4ebd196b43fead3dd437afdb0a2dafe092ae3d559332b3d9d61985c92e1a59982d8f24942606e6a98598c6ef7ff43697e858725
fa21535551e300eaa988d209ad64cdc17fd7f66b fuzz: Increase merge -rss_limit_mb (MarcoFalke)
Pull request description:
For some reason, the limit is hit. (Presumably due to `-set_cover_merge=1` eating more memory, or by simply having more fuzz inputs).
Fix it by increasing it for the merge operation.
ACKs for top commit:
dergoegge:
ACK fa21535551e300eaa988d209ad64cdc17fd7f66b
hebasto:
ACK fa21535551e300eaa988d209ad64cdc17fd7f66b, considering the discussion in https://github.com/bitcoin-core/qa-assets/pull/155.
Tree-SHA512: 4fed0f254eccc6fe0b53656bc345ff898b13811dc39387387317d34b521ab77cee03d82b0896dd92d253b7546b6a7e4bdcd478749f47064374ab44ad759ab9ff
c96f9e0285 feat: only require reindexing when the index was off going to off (pasta)
Pull request description:
## What was done?
It does not seem reasonable that we need to reindex when turning indexes off. this logic was introduced in e0781095f0
## How Has This Been Tested?
Hasn't
## Breaking Changes
None, basically
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK c96f9e0285
Tree-SHA512: a5b634b9fbb6632b35286055a8e5b8db340738b7327a7d860e645ec1f6ef5b5cddc140a3e1c51b09c2f47db375c83e7c53fe5eaf221ab8df1d173caf50fa862d
fa1eb0ecaef14d428812f956082d29ab134fc728 test: Make the unlikely race in p2p_invalid_messages impossible (MarcoFalke)
Pull request description:
After `add_p2p_connection` both sides have the verack processed.
However the pong from conn in reply to the ping from the node has not
been processed and recorded in totalbytesrecv.
Flush the pong from conn by sending a ping from conn.
This should make the unlikely race impossible.
ACKs for top commit:
mzumsande:
ACK fa1eb0ecaef14d428812f956082d29ab134fc728
pinheadmz:
ACK fa1eb0ecaef14d428812f956082d29ab134fc728
Tree-SHA512: 44166587572e8c0c758cac460fcfd5cf403b2883880128b13dc62e7f74ca5cb8f145bb68a903df177ff0e62faa360f913fd409b009d4cd1360f1f4403ade39ae
8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3 util: fix argsman dupe key error (willcl-ark)
Pull request description:
fixes#22638
Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when `settings.json` contains duplicate keys. This change has no effect on `bitcoind` because it treats a corrupt `settings.json` file as a hard error and doesn't attempt to modify it.
If we find a duplicate key and error, clear `values` before returning so that `WriteSettings()` will write an empty file, therefore clearing it.
This aligns with GUI behaviour added in 1ee6d0b.
The test added only checks that `values` is empty after a duplicate key is detected. This paves the way for the `abort` option in the GUI to properly clear `settings.json`, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents).
ACKs for top commit:
TheCharlatan:
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3
ryanofsky:
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR:
Tree-SHA512: a5fd49b30ede0a24188623192825bccb952e427cc35f96ff9bfdc737361dcc35ac6480589ddf7f0ddeaebd34361bdaee31e7a91f2c0d857e4ff682614bb6bc04
54c4d03578c5842f19bf8bc68aca5faf8beed5c3 doc: Show how less noisy clang-tidy output can be achieved (TheCharlatan)
Pull request description:
Adds a paragraph to the clang-tidy section explaining how to de-noise its output. By default clang-tidy will print errors arrising from included headers in leveldb and other dependencies. By passing `--enable-suppress-external-warnings` flag to configure, errors arising from external dependencies are suppressed. Additional errors arrising from internal dependencies such as leveldb are suppressed by passing the `src/.bear-tidy-config` configuration file to bear. This file includes exclusionary rules for leveldb.
ACKs for top commit:
MarcoFalke:
utACK 54c4d03578c5842f19bf8bc68aca5faf8beed5c3
Tree-SHA512: c3dd8fb0600157582a38365a587e02e1d249fb246d6b8b4949a800fd05d3473dee49e2a4a556c60e51d6508feff810024e55fe09f5a0875f560fde30f3b6817c
5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2 Use string interpolation for default value of -listen (ekzyis)
Pull request description:
This is a refactoring change. So I have read the following and will try to answer why this change should be accepted
> * Refactoring changes are only accepted if they are required for a feature or
bug fix or **_otherwise improve developer experience significantly_**. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
I have noticed in https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1086731856 that the helper message for `-listen` does not use string interpolation.
That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change.
However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be).
Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future.
ACKs for top commit:
stratospher:
ACK 5c938e7.
vasild:
ACK 5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2
kristapsk:
cr utACK 5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2
Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f
faa671591f9c83ef0fb5afea151a1907c28f024b test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f9c83ef0fb5afea151a1907c28f024b
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
8847ce44e0713350a6d3524f62eaeb10ba548bae util: add missing include and fix function signature (Cory Fields)
Pull request description:
ping hebasto
Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it.
Similar to the fix in #27144.
The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along.
ACKs for top commit:
fanquake:
ACK 8847ce44e0713350a6d3524f62eaeb10ba548bae
Tree-SHA512: 5eb9a6691ee37cbc5033a48aedcbf5c93af3b234614ae14c3fcc858f1ee505f630ad68f8bbb69ffa280080c8d0f91451362cb3819290b741ce906b2b3224a622
29b62c01c8d211475ea9dd1a1093820f0a86c06d valgrind: remove libsecp256k1 suppression (fanquake)
Pull request description:
I am no-longer been able to recreate this issue, atleast after the most recent libsecp256k1 changes. Can someone else confirm?
ACKs for top commit:
MarcoFalke:
lgtm ACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
sipa:
utACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
Tree-SHA512: e61e5b88ac2af3c779f23d999938bec4497f6433a029c07dd57a9481fe9cc104d8d8f63586910b29f41e66bbf0ed094bc7539c0df84754a1783c4b1b15af072e
84ca5b349ecc2ad083bb39352e5d5ae731fb1622 doc: mention sanitizer suppressions in developer docs (fanquake)
Pull request description:
Should be enough to close#17834.
ACKs for top commit:
MarcoFalke:
lgtm ACK 84ca5b349ecc2ad083bb39352e5d5ae731fb1622
Tree-SHA512: 233c688a3cef1006c9a00f7b7a52fd6ee0ec150367e5e56904b6f1bbdca21b9217c69f8fcf653a4943613d12c3178a39f761b25eb24fc1954a563cfb1f832f5e
b70e091374 Merge bitcoin/bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake)
6d7aa3d978 Merge bitcoin/bitcoin#29497: test: simplify test_runner.py (fanquake)
d0e15d59f8 Merge bitcoin/bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow)
045fa5f57e Merge bitcoin/bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow)
bd607f049d Merge bitcoin/bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow)
c9617558e3 Merge bitcoin/bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake)
8d6e5e7d67 Merge bitcoin/bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake)
4dce690a5e Merge bitcoin/bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake)
910a7d6cbf Merge bitcoin/bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake)
fdac2b3286 Merge bitcoin/bitcoin#29493: subtree: update crc32c subtree (fanquake)
a23b342d7d Merge bitcoin/bitcoin#29475: doc: Fix Broken Links (fanquake)
92bad90e6c Merge bitcoin/bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake)
9b6a05df66 Merge bitcoin/bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake)
9963e6bc28 Merge bitcoin/bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake)
3914745a10 Merge bitcoin/bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake)
b719883081 Merge bitcoin/bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake)
f0968807bc Merge bitcoin/bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow)
03e0bd3347 Merge bitcoin/bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
See commits
## How Has This Been Tested?
built locally; large combined merge passed tests locally
## Breaking Changes
Should be none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK b70e091374
knst:
utACK b70e091374
Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6
fae5bd920007c33de0b794bf2b2d698bfccc12ee test: Fix wallet_balance intermittent issue (MacroFake)
Pull request description:
Diff to reproduce:
```diff
index d2ed97ca76..25cc2d5734 100755
--- a/test/functional/wallet_balance.py
+++ b/test/functional/wallet_balance.py
@@ -265,7 +265,7 @@ class WalletTest(BitcoinTestFramework):
self.nodes[0].invalidateblock(block_reorg)
self.nodes[1].invalidateblock(block_reorg)
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
- self.generatetoaddress(self.nodes[0], 1, ADDRESS_WATCHONLY, sync_fun=self.no_op)
+ self.generatetoaddress(self.nodes[0], 1, ADDRESS_WATCHONLY)
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
# Now confirm tx_orig
```
Example in CI:
```
test 2022-08-24T10:09:22.486000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_balance.py", line 269, in run_test
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(98.85983340 == 0)
```
https://cirrus-ci.com/task/4981266251513856?logs=ci#L3269
ACKs for top commit:
achow101:
ACK fae5bd920007c33de0b794bf2b2d698bfccc12ee
w0xlt:
ACK fae5bd9200
Tree-SHA512: 470f366720615c4a9326ec4c581fff569ecce9877f9134bb1975ec3d6f1d13a6403051418a91a80b2a86de617f43e539ec11bbf4f1713d0354d5b0ab98d22437
f8e228476f54b36beacc3fd09038dfeebdac6b3c tracing: do not use `coin` after move in `CCoinsViewCache::AddCoin` (Seibart Nedor)
Pull request description:
This is fix for https://github.com/bitcoin/bitcoin/issues/25640.
ACKs for top commit:
0xB10C:
ACK f8e228476f54b36beacc3fd09038dfeebdac6b3c
Tree-SHA512: e7643ac8e6b6247aaf250f44572c4b458da4aea030ac0268227564e6857200e9c23efe325cfc535f46498cbeccaf46301551efeeb54b062f71d2dcf1ffe71fb8
2ab7952bda8d15e91b03f8307839030cbb55614e test: add bip157 coverage for (start height > stop height) disconnect (Sebastian Falbesoner)
63e90e1d3f5ed08f9871f07667d389ec66aa621c test: check for specific disconnect reasons in p2p_blockfilters.py (Sebastian Falbesoner)
Pull request description:
This PR checks for specific disconnect reasons using `assert_debug_log` in the functional test `p2p_blockfilters.py`. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message.
Also, based on that, missing coverage for the (start height > stop height) disconnect case is added:
b7138252ac/src/net_processing.cpp (L3050-L3056)
ACKs for top commit:
MarcoFalke:
lgtm ACK 2ab7952bda8d15e91b03f8307839030cbb55614e
furszy:
Looks good, code ACK 2ab7952b
Tree-SHA512: 0581cb569d5935aaa004a95a6f16eeafe628b9d816ebb89232f2832e377049df878a1e74c369fb46931b94e1a3a5e3f4aaa21a007c0a488f4ad2cda0919c605d
d05be124dbc0b24fb69d0c28ba2d6b297d243751 test: added coverage to estimatefee (kevkevin)
Pull request description:
Added a assert for an rpc error when we try to estimate fee for the max conf_target
Line I am adding coverage to https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#LL71C52-L71C52
ACKs for top commit:
MarcoFalke:
lgtm ACK d05be124dbc0b24fb69d0c28ba2d6b297d243751
Tree-SHA512: dfab075989446e33d1a5ff1a308f1ba1b9f80cce3848fbe4231f69212ceef456a3f2b19365a42123e0397c31893fd9f1fd9973cc00cfbb324386e12ed0e6bccc
508d05f8a7b511dd53f543df8899813487eb03e5 [fuzz] Don't use afl++ deferred forkserver mode (dergoegge)
Pull request description:
Fixes#28469
This makes our afl++ harness essentially behave like libFuzzer, with the exception that the whole program does fully reset every 100000 iterations. 100000 is somewhat arbitrary and we could also go with `std::numeric_limits<unsigned in>::max()` but a smaller limit does allow for the occasional reset to counter act some amount of instability in the fuzzing loop (e.g. non-determinism, statefulness).
It's a bit of a shame to do this just for the targets whose initial state can't be forked (e.g. threads) because other targets do benefit from not having to redo the state setup. An alternative would be https://github.com/bitcoin/bitcoin/issues/28469#issuecomment-1717526774:
```
If the goal is to be maximally performant, the fork would need to happen for each fuzz target specifically.
I guess it can be achieved by wrapping __AFL_INIT(); into a helper function and then require all fuzz
target initialize() to call it?
```
ACKs for top commit:
MarcoFalke:
lgtm ACK 508d05f8a7b511dd53f543df8899813487eb03e5
Tree-SHA512: d9fe94e2e3198795f8fb58f67eb383531a534bcd4ec75a1f0ae6ccb5531863dbc09800bb7d77536417745c4c8bc49a4f84dcc959918b27d4997a270eeacb0e7e
97e2e1d641016cd7b74848b9560e3771f092c1ea [fuzz] Use afl++ shared-memory fuzzing (dergoegge)
Pull request description:
Using shared-memory is faster than reading from stdin, see 7d2122e059/instrumentation/README.persistent_mode.md
ACKs for top commit:
MarcoFalke:
review ACK 97e2e1d641016cd7b74848b9560e3771f092c1ea
Tree-SHA512: 7e71b5f84835e41531c19ee959be2426da245869757de8e5dd1c730ae83ead650e2ef75f4d594d7965f661821a4ffbd27be84d3ce623702991501b34a8d02fc3
c0bf667912064960df194ea94150976b34f7c267 index: add [nodiscard] attribute to functions writing to the db (furszy)
eef595560e9ecf3a0d1db4d8ea7ecc33a49d839f index: coinstats reorg, fail when block cannot be reversed (furszy)
Pull request description:
Found it while reviewing https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1310863359.
During a reorg, continuing execution when a block cannot be reversed leaves the
coinstats index in an inconsistent state.
This was surely overlooked when 'CustomRewind' was implemented.
ACKs for top commit:
ryanofsky:
Code review ACK c0bf667912064960df194ea94150976b34f7c267. Only change since last review is new commit adding [[nodiscard]]
Tree-SHA512: f4fc8522508d23e4fff09a29c935971819b1bd3b2a260e08e2e2b72f9340980d74fbec742a58fe216baf61d27de057c7c8300e8fa075f8507cd1227f128af909
fbcacd4cf0dbe54e51f89cda05ff3db9e378ea12 test: remove fixed timeouts from feature_config_args (Martin Zumsande)
Pull request description:
Fixes#28290
These fixed timeouts aren't affected by the `timeout_factor` option and can therefore cause timeouts in slow environments.
They are also unnecessary for the test because they measure the wrong thing:
While there is an internal waiting time of 60s within `ThreadOpenConnections` (beginning only when that thread is started) for fixed seeds querying, the timeouts here don't measure that but the time from startup until a debug log message is encountered, during which many other things happen in init, so they don't make much sense to me in the first place.
ACKs for top commit:
MarcoFalke:
lgtm ACK fbcacd4cf0dbe54e51f89cda05ff3db9e378ea12
Tree-SHA512: 7bb3b7db2f9666b1929ffb7773c838ee98b0845569428e5d00ecf5234973d534c4f474e213896c71baabd6096a79347bd21b41a17b130053049714eb8a447c79
360ac64b90ee16cc24bd4c574ec7e11760515a79 test: previous releases: speed up fetching sources with shallow clone (Sebastian Falbesoner)
Pull request description:
For the sake of building previous releases, fetching the whole history of the repository for each version seems to be overkill as it takes much more time, bandwidth and disk space than necessary. Create a shallow clone instead with history truncated to the one commit of the version tag, which is directly checked out in the same command. This has the nice side-effect that we can remove the extra `git checkout` step after as it's not needed anymore.
Note that it might look confusing to pass a _tag_ to a parameter named `--branch`, but the git-clone manpage explicitly states that this is supported.
ACKs for top commit:
MarcoFalke:
lgtm ACK 360ac64b90ee16cc24bd4c574ec7e11760515a79
Tree-SHA512: c885a695c1ea90895cf7a785540c24e8ef8d1d9ea78db28143837240586beb6dfb985b8b0b542d2f64e2f0ffdca7c65fc3d55f44b5e1b22cc5535bc044566f86
452c094449de00f3640e6e0763366e7603375825 test: fix 'unknown named parameter' test in `wallet_basic` (brunoerg)
Pull request description:
This PR removes loop when testing an unknown named parameter. They don't have any effect.
ACKs for top commit:
jonatack:
ACK 452c094449de00f3640e6e0763366e7603375825
theStack:
re-ACK 452c094449de00f3640e6e0763366e7603375825
Tree-SHA512: cf1a37d738bb6fdf9817e7b1d33bc69643dae61e3dbfae5c1e9f26220c55db6f134018dd9a1c65c13869ee58bcb6f3337c5999aabf2614d3126fbc01270705e8
e417c988f61bf9d3948d5c8e169626922fe6e24c fuzz: coins_view: remove an incorrect assertion (Antoine Poinsot)
c5f6b1db56f67f529377bfb61f58c0a8c17b0127 fuzz: coins_view: correct an incorrect assertion (Antoine Poinsot)
Pull request description:
The `coins_view` fuzz target would assert in two places that the cache is consistent with the backend. But it's never the case (that's the whole point of using a cache).
The only reason this didn't result in a crash was that we would never actually hit these assertions. I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view (see https://github.com/bitcoin/bitcoin/pull/28216) which made the code paths with those assertions actually reachable.
ACKs for top commit:
dergoegge:
Code review ACK e417c988f61bf9d3948d5c8e169626922fe6e24c
Tree-SHA512: 5847bb2744a2f2831dace62d32b79cc491bf54e2af4ce425411d245d566622d9aff816d9be5ec8e830d10851c13f2500bf4f0c004d88b4d7cca1d483ef8960a6
5197660e947435e510ef3ef72be8be8dee3ffa41 tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings (Martin Leitner-Ankerl)
Pull request description:
Fixes#26916 by disabling the warning `-Wgnu-zero-variadic-macro-arguments` when clang is used as the compiler.
Also see the comments
* Proposed changes in the bug https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1480997053
* Proposed changes when moving to a variadic maro: https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768
ACKs for top commit:
hebasto:
ACK 5197660e947435e510ef3ef72be8be8dee3ffa41, I've reconsidered my [comment](https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1507142439) and I think the current localized approach is optimal.
fanquake:
ACK 5197660e947435e510ef3ef72be8be8dee3ffa41 - checked that this fixes the warnings under Clang.
Tree-SHA512: c3dda3bcbb2540af6283ffff65885a9937bfdaaef3b00dc7d60b9f9740031d5c36ac9cb3d3d8756dbadce4812201a9754f5b8770df0d5e0d5ee690ba8a7135d2
f054bd072afb72d8dae7adc521ce15c13b236700 refactor: use "if constexpr" in std::vector's Unserialize() (Martin Leitner-Ankerl)
088caa68fb8efd8624709d643913b8a7e1218f8a refactor: use "if constexpr" in std::vector's Serialize() (Martin Leitner-Ankerl)
0fafaca4d3bbf0c0b5bfe1ec617ab15252ea51e6 refactor: use "if constexpr" in prevector's Unserialize() (Martin Leitner-Ankerl)
c8839ec5cd81ba9ae88081747c49ecc758973dd1 refactor: use "if constexpr" in prevector's Serialize() (Martin Leitner-Ankerl)
1403d181c106bc271ad2522adebde07c7850069b refactor: use fold expressions instead of recursive calls in UnserializeMany() (Martin Leitner-Ankerl)
bd08a008b42dac921bd9c031637e378899c1cd1d refactor: use fold expressions instead of recursive calls in SerializeMany() (Martin Leitner-Ankerl)
Pull request description:
This simplifies the serialization code a bit and should also make it a bit faster.
* use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
* use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization.
ACKs for top commit:
MarcoFalke:
only change is to add a missing `&`. lgtm, re-ACK f054bd072afb72d8dae7adc521ce15c13b236700 📦
jonatack:
ACK f054bd072afb72d8dae7adc521ce15c13b236700
sipa:
utACK f054bd072afb72d8dae7adc521ce15c13b236700
john-moffett:
ACK f054bd072afb72d8dae7adc521ce15c13b236700
Tree-SHA512: 0417bf2d6be486c581732297945449211fc3481bac82964e27628b38ef55a47dfa58d730148aeaf1b19fa8eb1076489cc646ceebb178162a9afa59034601501d
50f7214e0915a88dd81c1ac1d292e049a398cda2 valgrind: add suppression for bug 472219 (fanquake)
Pull request description:
Now that https://bugs.kde.org/show_bug.cgi?id=472219 has been fixed upstream in:
https://sourceware.org/git/?p=valgrind.git;a=commit;h=6ce0979884a8f246c80a098333ceef1a7b7f694d
Add a supression to ignore the bug until we are using a fixed version of Valgrind.
Related to #28072.
ACKs for top commit:
MarcoFalke:
lgtm ACK 50f7214e0915a88dd81c1ac1d292e049a398cda2
Tree-SHA512: 1030f3709195250350fd9c558420a9b1773fb54fdb323e0452a46eeb69ec6d60b5df50bde617c12d917e16dde07db64dee1b0101ddd4eda6161261fc7f6d4474
