9784818442 mining: add getTransactionsByWitnessID() IPC method (Sjors Provoost)
d282ae6883 mining: add getTransactionsByTxID() IPC method (Sjors Provoost)
0d5e4d4712 test: restart node after IPC option override test (Sjors Provoost)
f16b3613cd ipc: Serialize null CTransactionRef as empty Data (Sjors Provoost)
0f466e1094 mempool: add lookup by witness hash (Sjors Provoost)
Pull request description:
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is inefficient, made worse by the need to make multiple calls. It also doesn't support queuing by witness id (yet, see #34013).
This PR introduces two new IPC methods:
- `getTransactionsById()`: takes a list of `Txid`'s
- `getTransactionsByWitnessID()`: : takes a list of `Wtxid`'s
Both return a list of serialised transactions. An empty element is returned for transactions that were not found.
Unlike the RPC counterpart, the IPC methods do not take advantage of `-txindex`. This could be done in a followup. For `Wtxid` that would involve adding a `-witnesstxindex`.
I thought about having a single (or overloaded) `getTransactions()` that works with both `Txid` and `Wtxid`, but I prefer that clients are intentional about which one they want.
A unit and functional test cover the new functionality.
Sv2 probably only needs `getTransactionsByWitnessID()`, but it's easy enough to just add both.
To rest with Rust use:
- https://github.com/2140-dev/bitcoin-capnp-types/pull/11
[^0]: there's two reasons the pool requests these transactions: to approve the template and to broadcast the block if a solution is found (the miner will also broadcast via their template provider). See also https://github.com/stratum-mining/sv2-spec/issues/170
ACKs for top commit:
achow101:
ACK 9784818442
sedited:
Re-ACK 9784818442
ViniciusCestarii:
Re-ACK 9784818442
ismaelsadeeq:
Code review ACK 9784818442
Tree-SHA512: 3c6ceb572ab7d8bd090a8f31b5e331304a7a19a3d1f1551c9c2e1ee41339d76f96ca6c41bd634c87fca0a969e7d9bfa6a16c26fb06c0dd2315f6ca1c76a16a31
31abaa264c doc: add an AI contribution policy (will)
Pull request description:
This policy, adapted from ripgrep, f0cec341ab/AI_POLICY.md who in turn adapted it from uv c5187e200d/AI_POLICY.md, works as a reasonable and pragmatic AI contribution policy at this point in time.
It codifies roughly how the project is currently operating, it's expectations when Ai is being used, and what we don't wish to see.
Link to the document directly from the new PR and issue helptext.
ACKs for top commit:
Sjors:
re-ACK 31abaa264c
achow101:
ACK 31abaa264c
sedited:
Re-ACK 31abaa264c
l0rinc:
ACK 31abaa264c
Tree-SHA512: 667bda2d02717889ee6878438b4e4c7155025ae6933ac748b49f7ca2a04c94515bdd04d522a778828995c3e328780521c1734dd0d8cca4711a702fc9f242756f
68cb7840d2 doc: improve offline-signing-tutorial after 32489 (Pablo Martin)
Pull request description:
General improvements noted in the #32489 review and deferred by the author:
- Remove [a stale NOTE](https://github.com/bitcoin/bitcoin/pull/32489#discussion_r3484961398) referencing `walletcreatefundedpsbt`; the tutorial was updated to use the send RPC instead.
- [Fix](https://github.com/bitcoin/bitcoin/pull/32489#discussion_r3484961536) `listtransactions` example output from `{...}` to `[...]`; the RPC returns a JSON array, not an object.
ACKs for top commit:
polespinasa:
ACK 68cb7840d2
Tree-SHA512: 0615f042a98f68d1a3bd71bf04ad0f66aa88b7011572b58e2b349623da0ac334d5d388caeee16e1bd922650e5aa6a9fa7d9d00da9a70d227dad22a78dd9e6b76
4e29de719e private broadcast: add release note for limited cap (Gregory Sanders)
cbf8c107c1 Release cs_main between individual private tx re-attempts (Greg Sanders)
5aea3d0373 private broadcast: limit outstanding txs to count of 10,000 (Gregory Sanders)
Pull request description:
Add a belt-and-suspenders feature, limit the amount of memory and cpu possible when unlucky or simply misconfigured. The worst case limit is roughly 400kB * 10,000 = 4GB, regardless of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in standardness rules, or simply fee mismatches may result in unbounded growth of memory usage. As the feature may be expanded in the future, explicit bounds helps reasoning going forward.
ACKs for top commit:
frankomosh:
tACK 4e29de719e. Ran private_broadcast_tests and p2p_private_broadcast_cap.py. Great to have an explicit bound as the belt-and-suspenders against unbounded queue growth.
vasild:
ACK 4e29de719e
andrewtoth:
ACK 4e29de719e
stickies-v:
ACK 4e29de719e
Tree-SHA512: 18161755f37d07cca185a09e782dbe2fd0025b8befd4f6660e988865cc3a9b705d41769b816161e8142fe6ce31a56e0288bd78efc25135cedfc47fc855011799
a99148d576 test kernel: Don't log on warnings change (sedited)
5b4fd284f4 kernel: Generate a signet with a challenge (sedited)
Pull request description:
Adds a function for creating a chainparams with a signet challenge to the kernel API. This was requested in issue https://github.com/bitcoin/bitcoin/issues/35362.
Also takes this opportunity to de-noise the test kernel binary log output a bit.
ACKs for top commit:
stickies-v:
ACK a99148d576
yuvicc:
lgtm! ACK a99148d576
musaHaruna:
reACK [a99148d](a99148d576)
Tree-SHA512: 44b414e1d43af59080c03940579307c205aa0533138bc94b67a1bfb72d347f3d3c5d7c349f0a4b3983456ee1face42794c1ed46d08154fc6f5b71d3d6915ae98
eccb04a321 refactor: Use `NetworkErrorString` for macOS code in `netif.cpp` (Hennadii Stepanov)
Pull request description:
Although these `sysctl` calls report generic system errors rather than network errors, `NetworkErrorString()` is identical to `SysErrorString()` on POSIX systems, so this does not change behavior. It allows removing `#include <util/syserror.h>`, which was only needed by `__APPLE__` code and would otherwise require an `#ifdef` guard to silence an IWYU unused-include warning on other platforms.
Related to https://github.com/bitcoin/bitcoin/pull/34995.
ACKs for top commit:
sedited:
ACK eccb04a321
Tree-SHA512: 25838dcf206f728fb0968a9ff336863870b69f857436a47b9916269e624d0f157bd566b461911dfea69716de482bbde6903d51bb97e6f70631dfcd274390ac3d
55e3a57f22 qa: Avoid UTXO reuse between test functions (Hodlinator)
9c5dd2926a p2p: Ignore CMPCTBLOCK from peer that hasn't sent SENDCMPCT (David Gumberg)
bf9884f4e5 p2p: make blocksonly nodes ignore CMPCTBLOCK messages (David Gumberg)
92cea63c71 test: (Un)solicited invalid cb -> get disconnected. (David Gumberg)
e845e26344 test: p2p: Nodes ignore unsolicited CMPCTBLOCK's (David Gumberg)
8313591715 p2p: Drop unsolicited CMPCTBLOCK from non-HB peer (David Gumberg)
44f377a71f refactor: test: Static assert_highbandwidth_states (David Gumberg)
25457a3272 test: Tighten getblocktxn checks in parallel cb reconstruction test. (David Gumberg)
51dd90fb50 refactor: Merge announce_cmpct_block() defs into one (Hodlinator)
Pull request description:
Processing unsolicited `CMPCTBLOCK`'s from a peer that has not been marked high bandwidth is not well-specified behavior in BIP-0152, in fact the BIP seems to imply that it is not permitted:
> "[...] method is not useful for compact blocks because `cmpctblock` blocks can be sent unsolicitedly in high-bandwidth mode"
See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness
This PR disables processing of CMPCTBLOCK messages in three cases:
$1$. When the block is unsolicited and from a non-HB peer.
$2$. When this node is running in `-blocksonly` mode.
$3$. When the peer has not advertised `CMPCTBLOCK` support with a `SENDCMPCT` message.
Not processing unsolicited blocks slightly raises the cost of discovering a peer's mempool via `CMPCTBLOCK` as described in #28272. As pointed out there, getting an HB slot is relatively easy, so this does not prevent an attacker from doing this, it just slightly raises the bar.
Probably more important is not processing `CMPCTBLOCK` messages as a `-blocksonly` node. A blocksonly node has a lot less surface area for leaking its mempool since it does no transaction relay, and leaking a blocksonly node's mempool is pretty dangerous since it is very likely to be the origin for all of the transactions in its mempool.
ACKs for top commit:
achow101:
ACK 55e3a57f22
w0xlt:
reACK 55e3a57f22
hodlinator:
re-ACK 55e3a57f22
polespinasa:
lgtm ACK 55e3a57f22
Tree-SHA512: 118bea55adca01dbd6467ba5ae3adf420d960794a6a2c40dd30fcc7d79aa944e01af0f6dd6bd6ff6d33dc9155171f6f4f497cbd7e8eb6d3c4c89e12740b51c05
cddbad325d doc: Add release notes for 32489 (exportwatchonlywallet RPC) (Pablo Martin)
Pull request description:
This is a follow-up to #32489.
ACKs for top commit:
polespinasa:
ACK cddbad325d
Tree-SHA512: bfc14c1d8576395caea9636fcc57182c7fe027e397b8529d60901f7310e30acfe9157fbc0e11f6ad8c16c96cb96df9d7d70d0965c5e8a06d6fa3e9e7b5a65533
c1313b199f init: wake genesis wait after ImportBlocks() returns (ismaelsadeeq)
Pull request description:
During startup with `-reindex`, the block index and chainstate are wiped, and
the normal `LoadGenesisBlock()` startup path is skipped while block files are
being indexed. The init thread can then wait for genesis to be processed via the
tip-block condition variable.
If shutdown is requested after the init thread enters that wait but before the
import thread activates genesis, the reindex scan can return early without
calling the later `LoadGenesisBlock()` fallback or `ActivateBestChains()`.
Because no block tip is connected, the block-tip notification never fires. The
wait predicate would accept `ShutdownRequested(node)`, but it is not evaluated
again unless the condition variable is notified.
Notify the tip-block condition variable after `ImportBlocks()` returns so the
genesis wait can observe shutdown and exit cleanly.
Described in detail https://github.com/bitcoin/bitcoin/pull/35621#issuecomment-4876059120
The current test covers the path, but won't fail deterministically. You can use L0rinc suggested patch for a deterministic patch that demonstrate the deadlock on master https://github.com/bitcoin/bitcoin/pull/35652#pullrequestreview-4627988553
ACKs for top commit:
maflcko:
review ACK c1313b199f🦁
mzumsande:
Code Review ACK [c1313b1](c1313b199f)
sedited:
tACK c1313b199f
Tree-SHA512: b17faaa2042882f2acebf688f4a28fd6bb11cc5d7f978991fe2bcba7a9df6ed82a0a2811f2e71a929189570a7926f148afdbeef5c1ca57a081e3bb914c2f6fe2
If shutdown interrupts ImportBlocks() before genesis activation,
no blockTip notification is sent. The wait predicate allows
shutdown to occur, but the condition variable is never
notified, so init can remain stuck waiting for genesis activation.
Notify the tip condition variable after ImportBlocks() returns so
interrupted imports wake the wait and let it observe the shutdown request.
Add test coverage for interrupting startup after reindex block files import
begins, which exercises the path where ImportBlocks() can return before genesis
activation.
Although these `sysctl` calls report generic system errors rather
than network errors, `NetworkErrorString()` is identical to
`SysErrorString()` on POSIX systems, so this does not change behavior.
It allows removing `#include <util/syserror.h>`, which was only needed
by `__APPLE__` code and would otherwise require an `#ifdef` guard to
silence an IWYU unused-include warning on other platforms.
256482ab56 validation: In AcceptBlock, ignore flush result (optout)
Pull request description:
Shortcut: see https://github.com/bitcoin/bitcoin/pull/35570#issuecomment-4827437071 and https://github.com/bitcoin/bitcoin/pull/29700#discussion_r3488550312 .
**Summary.** At the end of `ChainstateManager::AcceptBlock`, the flushing of the chain state is initiated, but any potential failure from `FlushStateToDisk` is ignored. However, the error message might get propagated upwards. This change makes sure that both the error status and the error message are ignored, and adds an explanatory comment.
**Related/background.** The potential inconsistency between the return value and state returned from `AcceptBlock` was found during refactoring of the return of `BlockValidationState` in #35570. PR #29700 also touches this part by adding an explanatory comment, see https://github.com/bitcoin/bitcoin/pull/29700#discussion_r3488550312 . Splitting this minor behavior change out of #35570 makes that PR a clean (no-bahavior-change) refactor (this option was mentioned from the start, and also proposed by reviewers; see: https://github.com/bitcoin/bitcoin/pull/35570#issuecomment-4827437071 ).
**Motivation**
- Get rid of the potential inconsistency between the return value (`true`) and state returned (`Error`). This facilitates refactoring of the error returns in #35570.
- Make the ignoring clear and explicit, so there is no doubt about it being intentional or not.
**Details.** `ChainstateManager::AcceptBlock` is called for new block candidates. After checks and inclusion, it triggers flushing of the updated chain state to disk, by calling `Chainstate::FlushStateToDisk`. This method returns a bool and also takes & returns a `BlockValidationState` state. In case of error it returns `false`, and sets the state (to `Error`) and the reject reason string. At the call site, the return value is ignored, but for the state the state variable of `AcceptBlock` is provided, so an Error state may get propagated upwards in the call chain. A caller may react to this Error state.
**Rationale for ignoring the flush error**
The validation code flushes internally in several places, and mostly doesn't treat flush failures as errors returned to callers. Disk errors should never be mistreated as block validation failure. Note that the fatal error notification inside `FlushStateToDisk` still fires, so the node will shut down on unrecoverable flush errors regardless.
**Relevant use case.** `AcceptBlock` is called from the following methods (excluding test code):
- `ChainstateManager::ProcessNewBlock`
- Twice from `ChainstateManager::LoadExternalBlockFile`
Of these, in two places the `state` returned is not used at all. Only at `validation.cpp:5056` is the state used.
```
BlockValidationState state;
if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr, true)) {
nLoaded++;
}
if (state.IsError()) {
break;
}
```
Here a flush error *may* trigger the `break`, causing an early exit from the loop over blocks in a block file. Post-change the returned state will not be `Error` in a disk error case, so the `break` will not be hit here. However:
- This branch is only called for blocks that are not known yet
- In case of disk error case the fatal error handler will cause the node to shut down in any case.
Based on the above, the change in error return behavior is acceptable.
ACKs for top commit:
maflcko:
lgtm ACK 256482ab56
l0rinc:
lightly tested ACK 256482ab56
dergoegge:
Code review ACK 256482ab56
sedited:
ACK 256482ab56
ismaelsadeeq:
ACK 256482ab56
Tree-SHA512: c72747b5725739117d6061c3b3d5e6910bd06325a072b20708475c8e178a7ad8a6509b30cc8b547c92011a1267e4ad2feef31a992110072e5f37a4ac1f5c79d8
Pure `getifaddrs()`/`freeifaddrs()` calls do not need any type
definitions beyond those provided by `<ifaddrs.h>` itself.
The subsequent `struct ifaddrs` processing in `netif.cpp` does not
involve any symbols from `<sys/types.h>`.
9a8ef9b0a3 test: SOCKS5 proxy: expect that connection may be reset during handshake (Vasil Dimov)
eb3208364a test: SOCKS5 proxy: expect that connection may be reset when forwarding (Vasil Dimov)
Pull request description:
The `forward_sockets()` function used by the SOCKS5 proxy forwards data between two connected sockets. It might happen that one of those sockets gets closed/reset abruptly, without sending EOF first. This is to be expected if e.g. `bitcoind` is shutdown and shouldn't result in noisy harmless messages like:
```
2026-06-03T13:23:56.966859Z TestFramework.socks5 (ERROR): socks5 request handling failed (running True)
Traceback (most recent call last):
File ".../socks5.py", line 199, in handle
forward_sockets(self.conn, conn_to, self.wakeup_socket_pair[1], self.serv)
~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File ".../socks5.py", line 76, in forward_sockets
data = s.recv(4096)
ConnectionResetError: [Errno 104] Connection reset by peer
```
Instead turn this into a debug log message with a nice prefix containing enough information to identify the two forwarded sockets.
---
Also expect that the connection might be closed during the SOCKS5 handshake and only log a debug message if that happens.
ACKs for top commit:
optout21:
crACK 9a8ef9b0a3
danielabrozzoni:
reACK 9a8ef9b0a3
sedited:
ACK 9a8ef9b0a3
Tree-SHA512: 24e25a30529eda3536ebf472f63a93fd80fff46273054a7075490c88737f8870c0141b2bc99d9ef39e6b4f592af2801350fdfbc71927f573738b4a14f5fd7ce0
b2de59d486 wallet, bdbro: Validate btree page levels (Ava Chow)
dc3a2b9c3b wallet, bdbro: Enforce overflow data lengths (Ava Chow)
Pull request description:
Alternative to #34946
BDB's overflow records include the total length of the data to be read from the overflow pages. If this length is impossible (larger than max page * page size), or if the data that we are reading exceeds the stated length, then throw an exception as this is an invalid BDB file. This prevents infinite looping if an overflow page makes a circular reference.
BDB BTrees also include the level in the tree that the page is supposed to be at. Leaf pages are always at level 1. Starting from the root page, we can validate that each child has a level one less than the parent, until we reach a leaf page with a level of 1. This also ensures that we cannot have circular internal page references.
ACKs for top commit:
sedited:
ACK b2de59d486
rkrux:
code review ACK b2de59d486
Tree-SHA512: b740c349f68c78150ee57b77e8e81605bb45a7db4fbca3c3a0d87318178dfa197f37faf0cbfd8bef791ab20f4a9afc3d2571842960055a70b6c081d605f59f94
095596ddf7 log: expose -logratelimit option (Lőrinc)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-4804334291
**Problem:** `-logratelimit` is currently registered as `DEBUG_ONLY`, so users who are not reading `-help-debug` are unlikely to discover the supported escape hatch when rate limiting interferes with diagnosis or log processing.
**Fix:** Expose the existing option in normal help while keeping rate limiting enabled by default.
Add argsman coverage to assert that `-logratelimit` is not `DEBUG_ONLY` and appears in the default help text.
**Reproducer:** you can check manually by grepping the help page or by adding a temporary unit test - before and after the change and see the difference.
<details><summary>bitcoind -help(-debug)</summary>
```bash
cmake -B build && cmake --build build -j -t bitcoind
build/bin/bitcoind -help-debug | grep -A2 logratelimit
build/bin/bitcoind -help | grep -A2 logratelimit
```
</details>
<details><summary>util_LogRateLimitHelp</summary>
```patch
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index da0d684050..0e918afd6b 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -3,6 +3,7 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <common/args.h>
+#include <init/common.h>
#include <sync.h>
#include <test/util/logging.h>
#include <test/util/setup_common.h>
@@ -710,6 +711,14 @@ BOOST_AUTO_TEST_CASE(util_AddCommand_clearargs_replaces_command_options)
BOOST_CHECK(details.empty());
}
+BOOST_AUTO_TEST_CASE(util_LogRateLimitHelp)
+{
+ ArgsManager args;
+ init::AddLoggingArgs(args);
+ BOOST_CHECK_EQUAL(*Assert(args.GetArgFlags("-logratelimit")) & ArgsManager::DEBUG_ONLY, 0U);
+ BOOST_CHECK(args.GetHelpMessage().find("-logratelimit") != std::string::npos);
+}
+
BOOST_AUTO_TEST_CASE(util_GetChainTypeString)
{
TestArgsManager test_args;
```
</details>
ACKs for top commit:
Crypt-iQ:
crACK 095596ddf7
stickies-v:
ACK 095596ddf7
janb84:
ACK 095596ddf7
sedited:
ACK 095596ddf7
Tree-SHA512: ff5bad5ee72616a3fbe0438c675d9c3c0f60a0999433e7e0cee259f3b7ad1edd05e5baeeb6e02c649bc9780006bbc907333b4ce962c11588a1d633e0bae19272
9f3e427228 fuzz: Remove ConsumeUniValue (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3514230802.
`ConsumeUniValue` isn't that useful as a shared fuzz helper, as it just returns the same hard-coded fields. This PR removes it and updates the `ipc` target to read a UniValue directly from fuzzer input instead. This is similar to what the `parse_univalue` target already does, and lets `ipc` test with different JSON values instead of one fixed value.
ACKs for top commit:
sedited:
ACK 9f3e427228
brunoerg:
code review ACK 9f3e427228
Tree-SHA512: 5bc0957c426b185392aabf5c74411758e93bc8e75e9896b08774b33d5ec8ba9c223499a139b03a94dcf907b73552962df9a9b059e875f40048707a87a69bf7c5
The helper created a UniValue with hard-coded constants, which
isn't ideal for fuzz tests. Replace it in the ipc fuzz test with
parsing a UniValue directly from the fuzzed data provider.
General improvements noted in the #32489 review and deferred by the
author:
- Remove a stale NOTE referencing walletcreatefundedpsbt; the tutorial
was updated to use the send RPC instead.
- Fix listtransactions example output from {...} to [...]; the RPC
returns a JSON array, not an object.
9b2ef81757 doc: add release notes for #33671 (getbalances nonmempool field) (Pablo Martin)
Pull request description:
This is a follow-up to #33671.
Top commit has no ACKs.
Tree-SHA512: 0572b121ff74b3a455355f03d3297f5c779313d7d7e1ad258d46f2d44980d6140280cbd8c367828e6db7b8243d7cc485bde7367edae4415afef0db7b70c23713
47bbed052e ci: use true|false over "true|false" (fanquake)
9a25bc3989 ci: use a 8x instance over 16x (fanquake)
Pull request description:
Follows up to https://github.com/bitcoin/bitcoin/pull/31425#discussion_r3453058552.
ACKs for top commit:
sedited:
ACK 47bbed052e
Tree-SHA512: 518794115550d744e2bae048858299ebb83f2d3678aa7ba088928fc54a497aa1bc9a55ccaf31cdd615eaac13e22c037ac1df663ce581acec9392eff4414f4987
f595daf1dd test: ensure HTTPServer race condition is fixed (Matthew Zipkin)
b98b10c072 test: introduce a worker thread in http socket error test (Matthew Zipkin)
922b08d375 test: socket error handling in HTTPServer using ErrorSock mock socket (Matthew Zipkin)
73da2a8a52 http: prevent race condition between worker thread and I/O thread (Matthew Zipkin)
Pull request description:
This prevents a losing race condition that could prevent the server from reading any more requests from an HTTP client.
Found and reported by the fuzzing department: 7fe5f54497
The Race:
A connected socket can either be written to or read from based on the result of `GenerateWaitSockets()`. That method checks the `HTTPRemoteClient` flag `m_send_ready`. If it's `true` the implication is that there is data in the client's send buffer ready to go. Once that data is sent and the buffer is empty, `MaybeSendBytesFromBuffer()` sets it `false` again.
The sad case was when a worker thread calling `WriteReply()` adds data to the send buffer, but before it sets `m_send_ready` to `true`, the I/O thread sends that data and empties the buffer. With the buffer unexpectedly empty, `WriteReply()` sets `m_send_ready` to `true`.
The effect of this is that the socket will stay in "write" mode with nothing to write. With nothing to write, `MaybeSendBytesFromBuffer()` never sets it back to `false` and the socket is stuck forever.
The Fix:
Simply move `m_send_ready = true` inside the block of `WriteReply()` where `m_send_mutex` is still held. This prevents the I/O thread from emptying the send buffer while the worker thread is setting the flag.
Testing:
To observe the race condition, revert the first commit `"http: prevent race condition between worker thread and I/O thread"` and run the unit test from the remainder of the branch. I like to see the logs:
`test_bitcoin --log_level=all --run_test=httpserver_tests -- --printtoconsole --debug=http --debug=lock'
The test will fail with a small probability. The socket will get stuck and the test will abort after a 60 second timeout. To garuntee the race condition loses and fail the test every time, slow down `WriteReply()` in the worker thread:
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 99e30ff663..b0c7b516d8 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -614,6 +614,7 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> r
} else {
// Inform HTTPServer I/O that data is ready to be sent to this client
// in the next loop iteration.
+ std::this_thread::sleep_for(500ms);
m_client->m_send_ready = true;
}
```
With the first commit (the fix) back in place, slowing down the worker thread like this won't fail the test.
Bonus:
The unit test is spread over three commits. First, a method of the socket testing setup is templated so a mock socket that intentionally raises an error can be inserted. The unit test added in that commit covers a race condition that was fixed in #35182 in response to https://github.com/bitcoin/bitcoin/pull/35182/changes#r3358889539 so we get the added benefit of covering an error path, and guaranteeing coverage of both "optimistic send" (directly from worker thread) and regular send (from a tick in the I/O loop thread).
The next commit adds a worker thread to the unit test, at which point a race condition is possible but very unlikely because all requests are sent at once. Finally, we spread out the requests in the top commit and make the race condition much easier to catch.
ACKs for top commit:
janb84:
crACK f595daf1dd
dergoegge:
utACK f595daf1dd
theStack:
Code-review ACK f595daf1dd
Tree-SHA512: 451982fd72724c4115e371fc6392605693d6c3207f00ffebcf027aae9253f7974b5b1165b9f46c91b5436d7fe60c7d27316fb0b79f729ab0bf8f32db2530075f
a15bdc0598 doc: update offline-signing-tutorial to use exportwatchonlywallet rpc (Pol Espinasa)
a388076401 test: Test for exportwatchonlywallet (Ava Chow)
d053e3e5c8 wallet, rpc: Add exportwatchonlywallet RPC (Ava Chow)
444878efef wallet: Add CWallet::ExportWatchOnly (Ava Chow)
f9273f01db wallet: Move listdescriptors retrieving from RPC to CWallet (Ava Chow)
a1c83789a7 wallet: Write new descriptor's cache in AddWalletDescriptor (Ava Chow)
1e996640e6 wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses() (Ava Chow)
d2ee9227da descriptor: Add CanSelfExpand() (Ava Chow)
Pull request description:
Currently, if a user wants to use an airgapped setup, they need to manually create the watchonly wallet that will live on the online node by importing the public descriptors. This PR introduces `exportwatchonlywallet` which will create a wallet file with the public descriptors to avoid exposing the specific internals to the user. Additionally, this RPC will copy any existing labels, transactions, and wallet flags. This ensures that the exported watchonly wallet is almost entirely a copy of the original wallet but without private keys.
ACKs for top commit:
polespinasa:
lgtm ACK a15bdc0598
Sjors:
re-utACK a15bdc0598
pablomartin4btc:
re-ACK [a15bdc0](a15bdc0598)
w0xlt:
lgtm reACK a15bdc0598
Tree-SHA512: cfc59415ad9aa13d1445cf2a85db1c051215496b6edcf5a8db463499b2b51b92ee7bf840b709035dff7635f9d0c533423bceb58c851f220500e1ea254d12f3b8
fb1d152c24 depends: Boost 1.91.0-1 (fanquake)
Pull request description:
https://www.boost.org/releases/1.91.0/.
> MultiIndex
> Breaking change: All type lists accepted or provided by the library (indexed_by, tag, nested typedefs index_specifier_type_list, index_type_list, iterator_type_list and const_iterator_type_list) are no longer based on [Boost.MPL](https://www.boost.org/libs/mpl) but instead they are now [Boost.Mp11 lists](https://www.boost.org/libs/mp11/doc/html/mp11.html#definitions). As a result, Boost.MultiIndex doesn’t depend anymore on Boost.MPL. It is expected that most user code won’t be impacted by this change, but the previous behavior may be restored, however, by globally defining the macro BOOST_MULTI_INDEX_ENABLE_MPL_SUPPORT. Legacy support for Boost.MPL may be eventually deprecated and removed in the future.
> Breaking change: composite_key and associated class templates (composite_key_equal_to, composite_key_compare, composite_key_hash) have been made variadic (previously the maximum number of template arguments was limited by BOOST_MULTI_INDEX_LIMIT_COMPOSITE_KEY_SIZE). This change should be transparent to users, with the exception that composite_key::key_extractors returns a std::tuple instead of a boost::tuple (and similarly for the rest of affected class templates).
> Removed internal workarounds and fallbacks to support pre-C++11 compilers.
ACKs for top commit:
hebasto:
re-ACK fb1d152c24.
willcl-ark:
ACK fb1d152c24
Tree-SHA512: 08a12d5f2b184faea8a0c68a276e6a11ebeb0dbedfa9e0976d80d49d5cbcee20fec07d77ee90399e0d35a18068755973e30d8451e22e7a151efaded30c7bfcb1
Document project expectations for AI-assisted contributions so contributors
understand when AI use is acceptable and when it creates review or moderation
burden.
Link to the document directly from the new PR and issues helptext.
ExportWatchOnly produces a watchonly wallet file from a CWallet. This
can be restored onto another instance of Bitcoin Core to allow that
instance to watch the same descriptors, and also have all of the same
initial address book and transactions.
When listdescriptors retrieves the descriptors from the wallet, instead
of having this logic in the RPC, move it into CWallet itself. This
will enable other functions to get the descriptors in an exportable
form.
037ad77071 fuzz: add IPC round-trip target (Enoch Azariah)
8a739a5510 build: allow ipc fuzz builds (Enoch Azariah)
Pull request description:
As discussed in #23015, this PR adds an IPC fuzz target to exercise the Cap'n Proto/libmultiprocess serialization bridge using an in-process two-way pipe and a reflected test interface.
It covers round-trip serialization for `COutPoint`, `CScript`, `std::vector<uint8_t>`, `UniValue`, and transactions, and exercises libmultiprocess proxy/server interaction. The target guarantees at least one IPC operation per input and is included by default when IPC is enabled in fuzz builds
Coverage [report](https://marcofleon.github.io/coverage/ipc/) provided by marcofleon
ACKs for top commit:
marcofleon:
ACK 037ad77071
sedited:
Re-ACK 037ad77071
Tree-SHA512: aae8835c8a378886b36420c422136c743b235877f6fd6f0c681910903c68e18753598c7f0e0279ce2e64d627241ce3a4e08f7edecc2e38765569c9faa61bf45e
91586f701e test: introduce NodeSigner, run feature_taproot.py without wallet compiled (Sebastian Falbesoner)
771200ca43 test: return full keypair from `getnewdestination` helper (Sebastian Falbesoner)
Pull request description:
This PR introduces a simple `NodeSigner` wallet replacement class that delegates signing to a specified node via the `signrawtransactionwithkey` RPC. Note that this is fundamentally different to `MiniWallet`, as it allows spending standard output types (P2PKH, P2WPKH, P2SH-P2WPKH, P2TR) and operates on already existing raw transactions, rather than allowing to create them from scratch (though support for that could still be added later).
A `NodeSigner` instance is plugged into the taproot functional test (`feature_taproot.py`) in order to allow running without having the Bitcoin Core wallet compiled. This was done with the intention to change the nature of this functional test as little as possible, as it seems that the variety of additional (pre-taproot) output script types and a rather sophisticated scheme for deriving amounts (including change) is desired -- if this is not considered relevant, a more invasive replacement using MiniWallet might also be a possible alternative.
ACKs for top commit:
maflcko:
review ACK 91586f701e 🏗
brunoerg:
code review ACK 91586f701e
Tree-SHA512: 20c047268261ce5c9517bae5e3d71e9313de149841273451df8639bd81009b933b7061c4881651a345c12e11b311ff746feb2dee84ee9b19dd57e18bb7fe3fee
b36730a3ef Add CI job for riscv bare metal (TheCharlatan)
bfdbf513f6 Add CI job for producing a static bare metal binary (TheCharlatan)
a9a1d92a1d build: Add option for building for bare metal envs (TheCharlatan)
Pull request description:
This adds a CI job for building the static consensus library and linking it to an executable. It uses newlib-cygwin as a C library for the final linking step. This ensure compatibility with this target going forward and can serve as a starting point for enabling bare metal builds for the entire kernel library. This would have also caught the error fixed in #31365.
ACKs for top commit:
fanquake:
ACK b36730a3ef
willcl-ark:
reACK b36730a3ef
Tree-SHA512: c199260f243e20df7f6a537e6c1eaf3d32e23f8fc78b9a8e2b75d9feff3830ef61279e93093dacdceb99e8eb010321b4c1c644e2ad9e266f0ca1ae736baa20ae
Document that newly indexed `txospenderindex` entries use less disk space and that existing indexes remain readable.
Users only need to rebuild the index if they want previously indexed entries rewritten with the smaller marker.
`TxoSpenderIndex` uses LevelDB values only as presence markers. `FindSpender` iterates over keys and loads the spending transaction from the disk position stored in each key.
Write new entries with a zero-byte span instead of a serialized empty string, so rebuilt indexes avoid one extra byte per spender entry.
Existing indexes remain readable because the lookup path does not deserialize the marker value.
