4101fea620 Merge bitcoin/bitcoin#28304: doc: Remove confusing assert linter (fanquake)
c59cb158e5 Merge bitcoin/bitcoin#26282: wallet: have prune error take precedence over assumedvalid (fanquake)
e2e8598c5a Merge bitcoin/bitcoin#23997: wallet: avoid rescans under assumed-valid blocks (Andrew Chow)
b66eebe64d Merge bitcoin/bitcoin#25599: build: Check for std::atomic::exchange rather than std::atomic_exchange (fanquake)
1204dc0f83 Merge bitcoin/bitcoin#25486: test: fix failing test `interface_usdt_utxocache.py` (MacroFake)
de17997621 Merge bitcoin/bitcoin#24062: refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (MacroFake)
c91f010e0e Merge bitcoin/bitcoin#25092: doc: various developer notes updates (MacroFake)
f39fcd1402 Merge bitcoin/bitcoin#24988: lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (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:
- [ ] 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 4101fea620
kwvg:
utACK 4101fea620
Tree-SHA512: e948ff58b256f2ecb9611681f773570d233985f1470e3eaa6899f3b7e53701c06f56ed5b965d250e22764938b0afebc8d85f92879ba111a0e20127cd63e99809
fa6e6a3f03a38f8b431bf694268ed344d1815b3b doc: Remove confusing assert linter (MarcoFalke)
Pull request description:
The `assert()` documentation and linter are redundant and confusing:
* The source code already refuses to compile with `assert()` disabled.
* They violate the assumptions about `Assert()`, which *requires* side effects.
* The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects.
Fix all issues by removing the docs and the linter. See also https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102
Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit.
ACKs for top commit:
hebasto:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b, I have reviewed the code and it looks OK.
theStack:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b
Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236
1c36bafc5f7db268546dcc86c793071a7e9d35e0 wallet: have prune error take precedence over assumedvalid (James O'Beirne)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739.
From Russ Yanofsky:
> Agree with all of Marco's points here and think this should be updated
>
> If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"
ACKs for top commit:
MarcoFalke:
ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0
aureleoules:
ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0
Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
817326a828d6148dc63d9ef08f641b9c0c522411 wallet: avoid rescans if under the snapshot (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks.
Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests.
ACKs for top commit:
achow101:
ACK 817326a828d6148dc63d9ef08f641b9c0c522411
ryanofsky:
Code review ACK 817326a828d6148dc63d9ef08f641b9c0c522411. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded.
Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596
4de4221ab4b645ff77503c777c9b195a962e9fa1 build: Check for std::atomic::exchange rather than std::atomic_exchange (Andrew Chow)
Pull request description:
Our usage of std::atomic is with it's own exchange function, not std::atomic_exchange. So we should be looking specifically for that function.
This removes the need for -latomic for riscv builds, which resolves a guix cross architecture reproducibility issue.
ACKs for top commit:
hebasto:
ACK 4de4221ab4b645ff77503c777c9b195a962e9fa1
fanquake:
ACK 4de4221ab4b645ff77503c777c9b195a962e9fa1
Tree-SHA512: dd8225fc9c6a335601f611700003d0249b9ef941efa502db39306129677929d013048e9221be1d6d7f0ea2d90313d4b87de239f441be21b25bea40a6c19a031e
f665c6ecda854adaaa629e961b1912847b42fd34 test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner)
Pull request description:
The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR #25435 (commit fa8421bc5bcd483a9501257073db17ff2e76eb46), leading to an error on master:
```
$ sudo ./test/functional/interface_usdt_utxocache.py
2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1
2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API
2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test
self.test_uncache()
File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache
invalid_tx = self.wallet.create_self_transfer(
TypeError: create_self_transfer() got an unexpected keyword argument 'from_node'
2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes
[...]
```
Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.)
ACKs for top commit:
MarcoFalke:
cr ACK f665c6ecda854adaaa629e961b1912847b42fd34
Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6
0dbafcee46 Merge bitcoin/bitcoin#27289: Refactor: Remove unused FlatFilePos::SetNull (fanquake)
dbe2e04d62 Merge bitcoin/bitcoin#27212: test: Make the unlikely race in p2p_invalid_messages impossible (fanquake)
6f6b718f78 Merge bitcoin/bitcoin#27236: util: fix argsman dupe key error (fanquake)
74c6e38530 Merge bitcoin/bitcoin#27205: doc: Show how less noisy clang-tidy output can be achieved (fanquake)
9e552f0293 Merge bitcoin/bitcoin#27232: Use string interpolation for default value of -listen (fanquake)
2a39b93233 Merge bitcoin/bitcoin#27226: test: Use self.wait_until over wait_until_helper (fanquake)
be2e16f33a Merge bitcoin/bitcoin#27192: util: add missing include and fix function signature (fanquake)
176a4a60d2 Merge bitcoin/bitcoin#27173: valgrind: remove libsecp256k1 suppression (fanquake)
d2fc8be331 Merge bitcoin/bitcoin#27154: doc: mention sanitizer suppressions in developer docs (glozow)
f5b4cc7e32 Merge bitcoin/bitcoin#16195: util: Use void* throughout support/lockedpool.h (Andrew Chow)
c66c0fdbf8 Merge bitcoin/bitcoin#27137: test: Raise PRNG seed log to INFO (fanquake)
bba215031b Merge bitcoin/bitcoin#25950: test: fix test abort for high timeout values (and `--timeout-factor 0`) (fanquake)
6751add2ea Merge bitcoin/bitcoin#27107: doc: remove mention of "proper signing key" (merge-script)
34c895a542 Merge bitcoin/bitcoin#26997: psbt: s/transcation/transaction/ (fanquake)
befdbeddf9 Merge bitcoin/bitcoin#27097: descriptors: fix docstring (param [in] vs [out]) (fanquake)
c98dd824b6 Merge bitcoin/bitcoin#27080: Wallet: Zero out wallet master key upon locking so it doesn't persist in memory (Andrew 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:
- [ ] 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 0dbafcee46
UdjinM6:
utACK 0dbafcee46
Tree-SHA512: e93a1136e848aa6c6f3d9fb0567b3e284975d35e82bbc1d9a8cd908067df4bf1257c939882abcaca6820360a4982b991f505a1165d95e1a8b52c9b181b7026b7
c75a0d4c57 Merge bitcoin/bitcoin#29177: build: Fix check whether `-latomic` needed (fanquake)
f670118cce Merge bitcoin/bitcoin#28851: build: Patch Qt to handle minimum macOS version properly (fanquake)
685ee8a46f Merge bitcoin/bitcoin#28884: doc: remove x86_64 build assumption from depends doc (fanquake)
47f6126504 Merge bitcoin/bitcoin#28881: doc: remove mention of missing bdb being a configure error (fanquake)
a9021db4ec Merge bitcoin/bitcoin#28777: doc: update docs for `CHECK_ATOMIC` macro (fanquake)
d5e15dfc5a Merge bitcoin/bitcoin#26839: Add support for RNDR/RNDRRS for AArch64 on Linux (Andrew Chow)
5aedcbfb43 Merge bitcoin/bitcoin#28778: depends: drop -O1 workaround from arm64 apple Qt build (fanquake)
95a8d8cfdc Merge bitcoin/bitcoin#21161: Fee estimation: extend bucket ranges consistently (glozow)
f4ea48e623 Merge bitcoin/bitcoin#28693: build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (fanquake)
f160e0dbb2 Merge bitcoin/bitcoin#28691: refactor: Remove CBlockFileInfo::SetNull (fanquake)
0278163aa3 Merge bitcoin/bitcoin#28697: fuzz: Increase merge -rss_limit_mb (fanquake)
90a1fb0e8d Merge bitcoin/bitcoin#28650: fuzz: Merge with -set_cover_merge=1 (fanquake)
f007abd19d Merge bitcoin/bitcoin#28459: build: add `-mbranch-protection=bti` (aarch64) to hardening flags (fanquake)
af8d12445a Merge bitcoin/bitcoin#28624: docs: fix typo (fanquake)
c740264da8 Merge bitcoin/bitcoin#28532: qt: enable` -ltcg` for windows under LTO (fanquake)
ccd3920d40 Merge bitcoin/bitcoin#28556: doc: fix link to developer-notes.md file in multiprocess.md (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:
UdjinM6:
utACK c75a0d4c57
Tree-SHA512: 035dc3fa9812c7f381946ae4798b8e729a58b38a090d94502a8d992e9cfaab3307173c602d7b782c637a79c5c41b62570dc73bb4bb367e4505a039964926181b
8bf1d06599 Merge bitcoin/bitcoin#29308: doc: update `BroadcastTransaction` comment (glozow)
2a77808596 Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header (Hennadii Stepanov)
da371b830d Merge bitcoin/bitcoin#28870: depends: Include `config.guess` and `config.sub` into `meta_depends` (fanquake)
2e41562d81 Merge bitcoin/bitcoin#29219: fuzz: Improve fuzzing stability for ellswift_roundtrip harness (fanquake)
b091329599 Merge bitcoin/bitcoin#29211: fuzz: fix `connman` initialization (Ava Chow)
df42d41060 Merge bitcoin/bitcoin#29200: net: create I2P sessions using both ECIES-X25519 and ElGamal encryption (fanquake)
4cdd1a8a5d Merge bitcoin/bitcoin#29172: fuzz: set `nMaxOutboundLimit` in connman target (fanquake)
97012ea522 Merge bitcoin/bitcoin#28962: doc: Rework guix docs after 1.4 release (fanquake)
c70ff5d702 Merge bitcoin/bitcoin#28844: contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake)
e6f19e7760 Merge bitcoin/bitcoin#29068: test: Actually fail when a python unit test fails (fanquake)
75e0334866 Merge bitcoin/bitcoin#28989: test: Fix test by checking the actual exception instance (Andrew Chow)
8cd85d311f Merge bitcoin/bitcoin#28852: script, assumeutxo: Enhance validations in utxo_snapshot.sh (Ryan Ofsky)
fd2e88d6f3 Merge bitcoin/bitcoin#26077: guix: switch from `guix environment` to `guix shell` (fanquake)
02741a7706 Merge bitcoin/bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment (fanquake)
dfd53dabed Merge bitcoin/bitcoin#28902: doc: Simplify guix install doc, after 1.4 release (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:
UdjinM6:
utACK 8bf1d06599
knst:
utACK 8bf1d06599
Tree-SHA512: 506273e5a188f9ca74edf656e3cd338992192e6e97f68c89fc43e34be20fb7f211b48e4dfa8693727839a7920da8284509413c722f55774a428939c296dad517
fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a Refactor: Remove unused FlatFilePos::SetNull (MarcoFalke)
Pull request description:
This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor.
Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method.
ACKs for top commit:
dergoegge:
Code review ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
TheCharlatan:
ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
john-moffett:
ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a
Tree-SHA512: 465c5e3eb4625405c445695d33e09a1fc5185c7dd1e766ba06034fb093880bfc65441d5334f7d9b20e2e417c2075557d86059f59d9648ca0e62a54c699c029b9
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
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
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
