21b4b790e4 test: Move cluster_linearize.h contents into cluster_linearize namespace (Hennadii Stepanov)
Pull request description:
Clang recently enabled `-Wunused-template` under `-Wall` (see https://github.com/llvm/llvm-project/pull/206123, https://github.com/llvm/llvm-project/pull/207848, https://github.com/llvm/llvm-project/pull/208001). Our codebase [triggers](https://my.cdash.org/builds/3714664/build) some of these warnings.
This PR:
1. Avoids Clang's `-Wunused-template` warnings in `src/test/util/cluster_linearize.h` when building the `bench_bitcoin` and `fuzz` targets.
2. Follows the C++ Core Guidelines: "[SF.21: Don't use an unnamed (anonymous) namespace in a header](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf21-dont-use-an-unnamed-anonymous-namespace-in-a-header)".
3. Drops `[[maybe_unused]]` annotations, which are no longer needed after changing the linkage from internal to external.
Along with https://github.com/bitcoin/bitcoin/pull/35679, this resolves all instances of this warning in the test/bench/fuzz code.
Another related change: https://github.com/bitcoin-core/minisketch/pull/102.
---
Steps to reproduce on the master branch @ 70d9ec7f3d:
```console
$ CXXFLAGS="-Wunused-template" cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_BENCH=ON
$ cmake --build build -t test_bitcoin
$ cmake --build build -t bench_bitcoin
[19/62] Building CXX object src/bench/CMakeFiles/bench_bitcoin.dir/cluster_linearize.cpp.o
In file included from /home/hebasto/dev/bitcoin/src/bench/cluster_linearize.cpp:9:
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:122:17: warning: unused function template 'Ser' [-Wunused-template]
122 | static void Ser(Stream& s, const DepGraph<SetType>& depgraph)
| ^~~
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:286:6: warning: unused function template 'SanityCheck' [-Wunused-template]
286 | void SanityCheck(const DepGraph<SetType>& depgraph)
| ^~~~~~~~~~~
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:383:6: warning: unused function template 'SanityCheck' [-Wunused-template]
383 | void SanityCheck(const DepGraph<SetType>& depgraph, std::span<const DepGraphIndex> linearization)
| ^~~~~~~~~~~
3 warnings generated.
[62/62] Linking CXX executable bin/bench_bitcoin
```
ACKs for top commit:
maflcko:
lgtm ACK 21b4b790e4
sedited:
ACK 21b4b790e4
Tree-SHA512: 893082c2dc3295b68738c438094656b3568f85b95e0e2f9b0b9dd178d0613e4cb50cd2300a9595e390b9d0a4c650ca78d2772dd1010c178756cefbeef6e2ed94
ddddffda3a doc: Add doc/release-notes-35836.md (MarcoFalke)
fa7fe798c6 wallet: Remove meaningless bool fallback in FundTransaction (MarcoFalke)
Pull request description:
This mostly removes a no-op and meaningless bool fallback in the `fundrawtransaction` RPC.
This allows to remove a `skip_type_check`. This makes validating the JSON schema from https://github.com/bitcoin/bitcoin/pull/34683 more consistent.
Adding the type check here is useful, because:
* The fallback was added in af4fe7fd12 (more than a decade ago). Retaining backwards compat with more than 10-year old clients seems purely theoretical. There were other breaking RPC changes on shorter notice in the meantime. If someone really forgot to update this over the last 10 years, I don't see a downside of notifying them.
* The compat only works for positional args, which seems another small reason to drop it.
* The compat is now fully irrelevant and a no-op, given that watch-only wallet is not a concept
anymore after commit 1337c72198.
* Keeping the compat means that openrpc spec users do not get any type checks at all here.
ACKs for top commit:
polespinasa:
ACK ddddffda3a
sedited:
ACK ddddffda3a
Tree-SHA512: aa5113bd74159ae5a3bf189edc48d011761db98beaf701cd144ba94eac9f525bec2b8eeb53e588b2f20dc9965ebabef2c5ce55a732b8cef2874a95f398ecae8f
13b53f8bf6 iwyu: Fix warnings in `src/consensus` and treat them as errors (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
maflcko:
review ACK 13b53f8bf6 🖐
sedited:
ACK 13b53f8bf6
Tree-SHA512: c5ec24aec2b618abe9b2046b8ffafa4f3eb61edf8fc9c73a2b6f4e6858797ec214d9cd28c546fbfae1a8f3324d7d6f1c9b6afd4050a1e7fd6075217f7abdfd32
fa5cbb8909 uint256: Workaround GCC-14 stringop-overread bug in Compare (Ava Chow)
6c9d76d589 doc: release note for alternate_wtxids in gettransaction (Ava Chow)
99bdcb064c test: compat, ensure downgrade preserves tx witness variants (furszy)
ef2afc6a0a test: Test for wallet txs with alternate wtxids (Ava Chow)
2d55c7a74d wallet: Show alternate wtxids in gettransaction (Ava Chow)
0b1af01bd4 wallet: Replace CWalletTx::SetTx with Update (Ava Chow)
56cf27db4d wallet: Store all witness variants of a transaction (furszy)
798ba6d04f wallet: Make CWalletTx::tx private and use CWalletTx::GetTx to access (Ava Chow)
72ebdd6364 wallet: Remove unused CWalletTx CopyFrom and copy constructor (Ava Chow)
19af439bdf wallet: Deserialize directly in CWalletTx's ctor (Ava Chow)
Pull request description:
When the wallet is presented with a transaction that has the same txid as one already known to the wallet, but has a different witness, instead of ignoring the transaction, store it alongside the known tx. This enables the wallet to be aware of all wtxid variants of its transactions. This also allows for the wallet to be able to calculate fees for replacements better as txs with different witnesses may have different feerates.
Specifically, the wallet stores these alternates in `CWalletTx` and extends the existing `tx` record type to essentially have a vector of transactions appended to the record. In `CWalletTx`, the single transaction is replaced with a map of wtxid to transaction so that all witness variants can still be represented by a single `CWalletTx`. For all of the various things that need the tx from a `CWalletTx`, a single witness variant is chosen to be the canonical tx and returned by `GetTx()`. This canonical tx is written into the same place as the previous single tx was written to in the `tx` record so that wallets can be loaded into previous versions.
To choose the canonical transaction, if any of the variants is confirmed, then that is the canonical one. Otherwise, the witness variant with the least weight is chosen.
An additional change I've included is to make `CWalletTx` RAII. This simplifies some of the implementation and enforces the assumption that a `CWalletTx` always has a transaction.
Lastly, `gettransaction` and `listtransaction` have a new field `alternate_wtxids` to inform users of the wtxids of the witness variants for a transaction, and of course, a test.
Closes#11240
ACKs for top commit:
furszy:
ACK fa5cbb8909
ajtowns:
ACK fa5cbb8909
w0xlt:
ACK fa5cbb8909
Tree-SHA512: ee303b395ab7a0843969f9491f876f4472c6301e968d9db87312edf44f7447245e707dd544356371d5f32fe6a619ee6937c24f3b7899f7a8108090b425f22d8e
1eac6a728b fuzz: populate wallet TXO index in wallet_create_transaction (frankomosh)
Pull request description:
A refactor in #27286 seemingly caused a regression in this harness. Rewiring to `m_txos` from `mapWallet` makes the setup loop in `wallet_create_transaction`, which uses raw `mapWallet.emplace()`, to no longer see coins. Therefore, `GetTXO`/`GetTXOs` probably return empty. To make coin selection possible, this fix calls `RefreshTXOsFromTx` after each insertion.
<details>
<summary>Coverage diff on select functions</summary>
`diff spend_cov_before.txt spend_cov_after.txt` on `src/wallet/spend.cpp`:
```diff
- 748| 0| if (auto bnb_result{SelectCoinsBnB(...)}) {
+ 748| 2.14k| if (auto bnb_result{SelectCoinsBnB(...)}) {
- 761| 0| if (auto knapsack_result{KnapsackSolver(...)}) {
+ 761| 4.46k| if (auto knapsack_result{KnapsackSolver(...)}) {
- 766| 0| if (auto cg_result{CoinGrinder(...)}) {
+ 766| 2.88k| if (auto cg_result{CoinGrinder(...)}) {
- 774| 0| if (auto srd_result{SelectCoinsSRD(...)}) {
+ 774| 4.46k| if (auto srd_result{SelectCoinsSRD(...)}) {
- 845| 0| auto op_selection_result = AutomaticCoinSelection(...);
+ 845| 1.27k| auto op_selection_result = AutomaticCoinSelection(...);
- 960| 0| if (auto res{AttemptSelection(...)}) {
+ 960| 1.36k| if (auto res{AttemptSelection(...)}) {
- 1484| 0| auto txr_grouped = CreateTransactionInternal(...);
+ 1484| 380| auto txr_grouped = CreateTransactionInternal(...);
```
</details>
ACKs for top commit:
brunoerg:
code review ACK 1eac6a728b
sedited:
tACK 1eac6a728b
Tree-SHA512: 9680332e7fca634af8c1526efb7f63c188be25dd40d3a3fdc1c2a54d982267fc2a3ddfd7a081225ffbfe51ce0281272998e6616a64d23e4d98a8e9005d19457f
fa7304f3a5 refactor: Remove unused #include in common/system (MarcoFalke)
Pull request description:
(See commit msg)
This should also fix CI issues, such as https://github.com/bitcoin/bitcoin/pull/35885#issuecomment-5184108518
ACKs for top commit:
hebasto:
ACK fa7304f3a5.
Tree-SHA512: 726aad3b79224ffc22c7db1bb7247d7a669eff5b43335c0d33c835848c2e6525e68759242d4f7cfd3c42734ce571e453d071064c6ae7b6c1d047538d0495de58
Now that clamping to std::numeric_limits<size_t>::max() is removed, the limits include is no longer needed.
Similarly after changing size_t -> u64, the cstddef include needs to be replaced by cstdint.
fa7f553781 test: Suppress implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator() (MarcoFalke)
Pull request description:
The truncation of u64 to size_t is intentional here, but it would be nice to document that for ubsan.
Otherwise, ubsan will print warnings about this. E.g. on 32-bit platforms:
```
/ci_container_base/src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long long') of value 18400304222395891501 (64-bit, unsigned) to type 'size_t' (aka 'unsigned int') changed the value to 2265382701 (32-bit, unsigned)
```
This is a bit tedious to test on 64-bit platforms, but one can use a diff like:
```diff
diff --git a/src/coins.h b/src/coins.h
index c854893bcb..906be9efae 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -246,3 +246,3 @@ public:
/** Hash a transaction ID, itself a cryptographic hash, as one jumbo block. */
- size_t operator()(const Txid& id) const noexcept
+ uint32_t operator()(const Txid& id) const noexcept
{
@@ -252,3 +252,3 @@ public:
/** Hash an outpoint as its txid jumbo block followed by the zero-extended index as one normal block. */
- size_t operator()(const COutPoint& id) const noexcept
+ uint32_t operator()(const COutPoint& id) const noexcept
{
```
and:
```
$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/test_bitcoin
./src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 17092028281225243117 (64-bit, unsigned) to type 'uint32_t' (aka 'unsigned int') changed the value to 291269101 (32-bit, unsigned)
ACKs for top commit:
l0rinc:
ACK fa7f553781
sedited:
ACK fa7f553781
Tree-SHA512: 0798e09a1291c7e7e2799c586f416b9e92ddf3ad64af3888a01726f338935f735c4e36716a7c38426e75ddbbdc2854949c3f553ab67758a053a2e3ba6a3c3ec1
6a2de55a0d test: require `TryGetTotalRam()` detection (Lőrinc)
cd086c16dd node, qt: inline `DEFAULT_DB_CACHE` (Lőrinc)
8bd9f46082 kernel: allow setting chainstate `dbcache` (Lőrinc)
8aa21e119b kernel, node: colocate dbcache bounds (Lőrinc)
7cfa21d60a scripted-diff: use `MIN_DBCACHE_BYTES` (Lőrinc)
ab63432576 common: cache total RAM as `uint64_t` (Lőrinc)
031fa402c8 scripted-diff: use `TryGetTotalRam` (Lőrinc)
41c44f5588 node, qt: use `1_MiB` for dbcache conversions (Lőrinc)
Pull request description:
**Problem:** Since #34692, the node chooses a `450 MiB` or `1 GiB` database cache from detected RAM, while Kernel always uses `450 MiB`.
The shared names obscure the difference between the node's automatic policy and Kernel's fixed fallback, and Kernel callers cannot set their own cache budget.
**Fix:** Cache RAM detection as `uint64_t`, keep the node's two-tier default unchanged, and make the fixed Kernel fallback explicit.
Add a chainstate-manager option setter that accepts a total database cache budget and applies the shared bounds and cache split.
ACKs for top commit:
maflcko:
review ACK 6a2de55a0d🚵
stringintech:
re-ACK 6a2de55a
sedited:
ACK 6a2de55a0d
Tree-SHA512: 4c92267647a757efb79e8396015de89290eed56c6ff109d9e81495f33ea68ccf90b77cb4b65412cd7825c29b6bf5384ac4ec77fc6c50fbf3e31ee82cf0b552f0
f4a6d079c4 qa: Support `get_bind_addrs` and `feature_bind_extra` on illumos (Hennadii Stepanov)
5e96a8fd5a doc: Add `lsof` to Test Suite Dependencies on NetBSD (Hennadii Stepanov)
5d01aa4772 qa: Ignore `lsof` warnings on NetBSD (Hennadii Stepanov)
70352fda03 qa: Strip prefix length from NetBSD `ifconfig` output (Hennadii Stepanov)
1c1735567e doc: Add `lsof` to Test Suite Dependencies on FreeBSD (Hennadii Stepanov)
4cb7f39c2c qa: Drop OpenBSD from supported platforms in `get_bind_addrs` function (Hennadii Stepanov)
8a982eea85 qa: Add `skip_if_no_lsof_on_nonlinux` helper and use it where needed (Hennadii Stepanov)
Pull request description:
This PR is a follow-up to #34256. It extends functional test support to illumos-based OSes and fixes several related issues on the *BSDs.
Changes:
- Make `lsof` an optional functional test dependency via a new `skip_if_no_lsof` helper, consistent with other optional test deps.
- Strip the CIDR prefix length from NetBSD `ifconfig` output (no-op on other platforms).
- Suppress spurious `lsof` warnings on NetBSD.
- Drop OpenBSD from the platforms supported by `get_bind_addrs`.
- Document the `lsof` Test Suite Dependency for FreeBSD and NetBSD.
- Add support for `get_bind_addrs` and `feature_bind_extra` on illumos.
CI runs: https://github.com/hebasto/bitcoin-core-nightly/pull/280.
Addresses https://github.com/bitcoin/bitcoin/pull/34256#issuecomment-4361855749.
ACKs for top commit:
l0rinc:
Lightly tested code review ACK f4a6d079c4
sedited:
utACK f4a6d079c4
Tree-SHA512: 24d943d059f5fa3f5626017eff744836177a41724544355f34b3a31fdf287bd1916bc6e903b598c1c55b61da2ff0f931b4455542d9cff6cf399ef7963096dff4
756afe14b5 test: give each ValidateInputsStandardness case its own scope (JP)
5559fa464b test: fix wrong transaction in GetP2SHSigOpCount assertion (JP)
Pull request description:
While reading through `script_p2sh_tests.cpp` I noticed one of the assertions in `ValidateInputsStandardness` checks the wrong transaction.
The test builds `txToNonStd2_no_scriptSig` (which spends a P2SH prevout with an empty scriptSig) and checks its standardness result ("input 0 P2SH redeemscript missing"), but the `GetP2SHSigOpCount` assertion right after it re-checks the previous transaction: line 433 is byte-identical to line 419. Looks like a copy-paste slip from 248c175e3d, which added a `GetP2SHSigOpCount` check after each constructed transaction.
This PR points the assertion at `txToNonStd2_no_scriptSig` and expects 0 sigops. With an empty scriptSig there's no redeemScript push, so `GetSigOpCount(scriptSig)` ends up counting an empty subscript and returns 0. This case wasn't asserted anywhere before. The line above covers the other side, where the same prevout spent with the actual redeemScript counts 20.
To make sure the fix isn't vacuous I also ran the assertion expecting 20, and it fails with `[0 != 20]`.
Tested with:
```
cmake --build build --target test_bitcoin
build/bin/test_bitcoin --run_test=script_p2sh_tests
```
ACKs for top commit:
l0rinc:
ACK 756afe14b5
sedited:
ACK 756afe14b5
Tree-SHA512: 463eda7bb8790fb55619b36a6bedcd437f5c3d753e8c0abaa57dde3154421cde8c36f04293eb712ddf2745d525b5e254e8a1ab898f4f68538520dd80d873dba3
28641fd195 p2p: reject empty getblocktxn requests (furszy)
9871fb726c p2p: reject filtered block inv early when bloom is disabled (furszy)
aaf9412026 refactor: split p2p_getdata.py in sub-cases (furszy)
Pull request description:
Reject requests that make the node read blocks from disk unnecessarily:
* `getblocktxn` is meant to request the txs a peer is missing. When a
peer sends a `getblocktxn` with an empty index vector, it isn't missing
anything, so it shouldn't have sent the message in the first place.
* The peer should not request a filtered block when the node does not
advertise the `NODE_BLOOM` service. Filtered blocks are built from
the bloom filter, which can be loaded only when the `NODE_BLOOM`
service is offered.
Both are disconnected now.
Note: can be split in two PRs if preferred.
ACKs for top commit:
151henry151:
ACK 28641fd195
l0rinc:
lightly tested ACK 28641fd195
mzumsande:
Code Review ACK 28641fd195
winterrdog:
tested ACK 28641fd195
sedited:
ACK 28641fd195
Tree-SHA512: 787ee0741fb797ea0898daab1bf3d7b3a21d91c9d940fc833e893806e95843e9d5a7a79a55ba67ac0f1550c08f2b6cee5ab2b625c082f56885ae795958cc608c
fa06ea4244 ci: Temporarily remove riscv32 config from GHA matrix (MarcoFalke)
873550bea3 ci: verify cross-build SDK archives (Lőrinc)
2c87337efe ci: update NetBSD cross-build SDK (Lőrinc)
Pull request description:
Address a few CI failures in three commits:
* The NetBSD SDK release candidate was removed, so the CI consistently fails. Fix it by bumping to a release.
* Also, include SDK download hashes to catch stuff like rare bit flips due to cosmic rays.
* The riscv32 config consistently fails with download errors as well. Temporarily remove it until issue https://github.com/bitcoin/bitcoin/issues/35853 is fixed.
ACKs for top commit:
hebasto:
ACK fa06ea4244.
sedited:
ACK fa06ea4244
Tree-SHA512: 27c728f2a94386e4ef882757e1cc30ca8336c89775965d1a567dcac17681d9dd49547246f700969293ac8798e882489eb5c1b2b5fed4759c83d0f4463a592579
The macOS and BSD cross-build jobs extract SDK archives fetched at runtime without checking their content.
Keep each expected digest beside the corresponding SDK version and verify every archive before extraction.
Create the OpenBSD library symlinks once after both archives are extracted, when all link targets are present.
Add `btck_chainstate_manager_options_set_database_cache_bytes()` so Kernel callers can set the total database cache budget.
Use `uint64_t` for a fixed-width C API, reject values outside the architecture-specific range, and keep `DEFAULT_KERNEL_CACHE` as the fallback.
Apply the selected split to the block tree database and `LoadChainstate()`.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: stringintech <stringintech@gmail.com>
Detect total RAM once so automatic cache selection and the oversized-cache warning use the same value.
Database cache arithmetic now uses `uint64_t`, so return the byte count directly and remove the 32-bit `size_t` clamp.
Co-authored-by: stringintech <stringintech@gmail.com>
This is a follow-up to bitcoin/bitcoin#35221. The `MakeAndPushFeature`
member function template has no callers yet, which triggers
`-Wunused-template` now that Clang 23 enables it as part of `-Wall`.
6573196e63 doc: Release note for export watchonly wallet gui action (Ava Chow)
cb51f97f6c gui: Menu action for exporting a watchonly wallet (Ava Chow)
5907a5c7dc gui: Add ExceptionSafeConnect that takes a lambda (Ava Chow)
Pull request description:
Allows a user to export a watchonly version of their wallet to be used in an airgapped setup.
Built on https://github.com/bitcoin/bitcoin/pull/32489
ACKs for top commit:
polespinasa:
lgtm ACK 6573196e63
pablomartin4btc:
ACK 6573196e63
hebasto:
ACK 6573196e63.
Tree-SHA512: 30732ecf2ff40dbbd62a8a9974a907fd60f0da89afacce618fb706a02349135dc06d7dfcc11009caba0e020609ab7e586a0ebbeb7cd65940ee2df229d23f0605
Otherwise the build will fail with warnings about missing Python (removed
in the next commit). However the non-gui build does not need Python.
Note that #25573 contains the same change, because the use of -static-pie
causes warnings when combined with our/CMakes own PIE handling.
8221d714c7 lint: document CI lief version requirement (fanquake)
594a02c3ae lint: re-add guix scripts to mypy linting (fanquake)
Pull request description:
These were no-longer being linted after https://github.com/bitcoin/bitcoin/pull/32458.
suppress `[union-attr]` warning. i.e:
```bash
contrib/guix/symbol-check.py:309: error: Item "None" of "lief.PE.Binary | lief.ELF.Binary | lief.MachO.Binary | lief.COFF.Binary | None" has no attribute "format" [union-attr]
contrib/guix/security-check.py:284: error: Item "lief.COFF.Binary" of "lief.PE.Binary | lief.ELF.Binary | lief.MachO.Binary | lief.COFF.Binary | None" has no attribute "abstract" [union-attr]
```
Add the comment suggested in [#35855.](https://github.com/bitcoin/bitcoin/pull/35855#discussion_r3694954625).
ACKs for top commit:
maflcko:
lgtm ACK 8221d714c7 in any case.
hebasto:
ACK 8221d714c7, I have reviewed the code and it looks OK.
Tree-SHA512: ec404a8235fd40b212aad71ee3fe3473a3ce6f1ebaed46661d36ef02862b08528e3b6b829e05c5b943c8543380f3876e33da725154ce31d4022ad39ae5ef5ab3
Fix all IWYU warnings in `src/common` and treat them as errors in CI.
Additionally, ensure that our drop-in header replacements are used
instead of system headers:
- `<util/check.h>` instead of `<cassert>`
- `<util/time.h>` instead of `<chrono>`
fa895bb77a fuzz: Rework rpc fuzz target (MarcoFalke)
Pull request description:
The `rpc` fuzz target constructs a vector of string args and passes that to `RPCConvertValues`.
This has many issues:
* Each of those strings could represent an array itself. E.g. via `range argument` or via `ConsumeArrayRPCArgument`. However, those strings may not be converted to an array via `RPCConvertValues` and just be passed on as string argument. Having a call to `ConsumeArrayRPCArgument` that ends up with a plain json string is confusing.
* The strings could only represent an object or json null, when a raw string represented such a serialized json and was also converted to one via `RPCConvertValues`. Having a call to `ConsumeScalarRPCArgument` that was intended to give a raw string but ends up with a arbitrary json object is confusing.
Fix those "stringly-typed" issues by making the fuzz target "type safe":
* Rename `ConsumeScalarRPCArgument` to `ConsumeBasicRPCArgument` and return a proper `UniValue` from it.
* The "consume string" case inside that function, which had a "double meaning" is turned into two type-safe cases: One that returns a json string and one that reads an arbitrary json from a string.
* A new case for json null is added.
* `ConsumeRPCArgument` is changed to cover both json arrays and json dicts properly.
* Pass the resulting positional UniValue array directly to the RPC method, avoiding the need for `RPCConvertValues`.
Making the fuzz target "type safe" is also the first step in making it schema-aware.
ACKs for top commit:
dergoegge:
utACK fa895bb77a
Tree-SHA512: ee22310c981be802f4838454be0e7ef2be213704621c08ffe98dbeab2e3d7cc6ff6a37f7a129e1570185f13a740d34fd9e2c6de3a8281e8b8dc1277f673c4a48
dd2561003d fuzz: cover the mempool interface for transaction announcement (Antoine Poinsot)
Pull request description:
This adds coverage in the existing `tx_pool` harness for the `ExtractBestByMiningScoreWithTopology` method recently added in #34628.
ACKs for top commit:
dergoegge:
utACK dd2561003d
Tree-SHA512: 3e2ea6afff080f48f5b519c49cb6ef51a76eb20186bcfcb5d8cf194f67aa99bf5f5683271b58b97c9bc4e521ea3e08a46ee019e0ffc028222db9dbcb383c48fd
55d3cd51a4 doc: add release note describing change for forbidden clients (Matthew Zipkin)
d1ed2a6e25 http: check rpcallowip immediately after accepting connection (Matthew Zipkin)
Pull request description:
This is a follow-up to #35182 addressing a review comment from that PR: https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068
This update to HTTPServer checks the IP subnet allowlist as soon as possible (immediately after receiving a connection from a client) before any data is received. This does not entirely protect the server from the "slow loris" attack or [CWE-400](https://cwe.mitre.org/data/definitions/400.html) but does restrict the attack surface to localhost and clients explicitly allowed by the user.
If a client is not allowed by the list, we disconnect as soon as possible. This is a behavior change from master branch (and previous release with libevent) where `403 Forbidden` was returned (after a potentially large amount request data was written to memory by the server).
To facilitate existing unit tests, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of static file scope. This is needed because otherwise the allow list would be empty when the unit tests run.
There is still plenty of refactoring to do in order to modernize `HTTPServer` and de-globalize it, but since this specific issue has a resource allocation guard, I wanted to open it quickly on its own.
ACKs for top commit:
janb84:
ACK 55d3cd51a4
winterrdog:
ACK 55d3cd51a4
w0xlt:
ACK 55d3cd51a4
fjahr:
Code review ACK 55d3cd51a4
Tree-SHA512: 545911f2e4d2f97ab8bc854e9e57c39eb896428f8c349d34c8e8025a1f6bfb8cfd436f381e36af8b87592c07df3e16210819f3eef7943e23c6626030e615fdf5
A getblocktxn msg is only needed when at least one tx is
missing from a compact block. If no txs are missing, the
block can be reconstructed without sending the request.
This avoids reading the requested block from disk
unnecessarily and also alerts the peer operator about
their node's buggy behavior.
A peer should not request filtered blocks from a node that
does not advertise NODE_BLOOM. Perform this check before
looking up the block to avoid an unnecessary disk read.
Note: currently, the request is ignored only after the
block has been read from disk, in the bloom filter
existence check.
