These benchmark inputs are immutable fixture bytes, so `DataStream` adds an unnecessary owned buffer and the setup needed to recreate or preserve its state.
Use `SpanReader` for block deserialization in `checkblock` instead.
This keeps `DeserializeBlockTest` focused on deserialization work, while `CheckBlockTest` still uses untimed setup only to rebuild a fresh uncached `CBlock` for the timed `CheckBlock()` call.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
`PrevectorDeserialize` only needs a reusable read-only view over fixed serialized bytes.
Keeping a mutable `DataStream` around just to call `Rewind()` is unnecessary.
Rebuild a fresh `SpanReader` for each benchmark run and remove `DataStream::Rewind()`, whose remaining use was this benchmark-only reset path.
The benchmark can now serialize exactly the 1000 entries it deserializes, so drop the stale extra element that used to avoid full consumption.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
`TestBlockAndIndex` still deserialized its fixed block fixture through `DataStream` and appended a dummy byte to avoid compaction after full consumption.
Use `SpanReader` for that fixture instead.
This removes the leftover dummy-byte workaround and reads the immutable fixture through a read-only view.
When `DataStream` is fully consumed, both `read()` and `ignore()` reset it to an empty state by clearing the backing buffer and resetting the read position.
Call `clear()` in both places instead of open-coding the same state transition.
This keeps the behavior unchanged while documenting the fully-consumed reset in one place.
Remove the unused `Compact()` method as well - it has been unused for a long time and can be added back if it is ever needed.
c9d8582235 fuzz: remove redundant CScript method calls from script harness (Bruno Garcia)
Pull request description:
The script harness was calling `GetSigOpCount`, `HasValidOps`, `IsPayToAnchor`, `IsPayToScriptHash`, `IsPayToWitnessScriptHash`, and `IsPushOnly` on the fuzzed CScript. All of these are already covered by the `script_ops` harness, which is the dedicated harness for CScript member methods. Also, add the missing `IsPayToAnchor` call to `script_ops` so the move preserves coverage.
ACKs for top commit:
maflcko:
lgtm ACK c9d8582235
Tree-SHA512: f69c9f217b5422e24714f985a8c0f7fcfb2c1b1e9117a0e54e7d31139bedfec9279a756ceb9b8860f7574b7cf59fb17c1063596176de8de4800824dc7866f33c
The script harness was calling GetSigOpCount, HasValidOps,
IsPayToAnchor, IsPayToScriptHash, IsPayToWitnessScriptHash, and
IsPushOnly on the fuzzed CScript. All of these are already covered
by the script_ops harness, which is the dedicated harness for
CScript member methods.
Also add the missing IsPayToAnchor call to script_ops so the move
preserves coverage.
5f36e0ff1e rpc: fix getblockstats UTXO overhead accounting (Lőrinc)
76190489e6 coins: pack `Coin` height/coinbase consistently (Lőrinc)
1f309d1aa2 coins: make `Coin::fCoinBase` a bool (Lőrinc)
Pull request description:
The [`getblockstats` RPC](https://github.com/bitcoin/bitcoin/pull/10757) currently overestimates UTXO overhead by treating the `fCoinBase` bitfield as an additional `bool` in `PER_UTXO_OVERHEAD`.
However, `fCoinBase` and `nHeight` are stored as bitfields and effectively packed into a single 32-bit value; counting an extra bool in the overhead calculation is unnecessary.
This PR introduces the following changes across three commits:
* Store `fCoinBase` as a `bool` bitfield to reduce implicit conversions at call sites.
* Use a consistent height/coinbase packing style across `Coin` serialization, undo serialization, and coinstats hashing (casting `nHeight` to `uint32_t` before shifting to avoid signed-promotion UB).
* Adjust UTXO overhead estimation to match the actual `Coin` layout and update the related tests accordingly.
ACKs for top commit:
achow101:
ACK 5f36e0ff1e
sedited:
ACK 5f36e0ff1e
vasild:
ACK 5f36e0ff1e
optout21:
crACK 5f36e0ff1e
Tree-SHA512: f4a44debed358e9e130da9d6fae5f89289daa34f0bdb7155edc3e9b691c219451f4c80b1e16aca5b011f0fa2fa975484ef1af4ca4563b7c6ba4ca9dd133f30be
6ac49373aa test: Add clean shutdown to Socks5Server (optout)
Pull request description:
Add clean shutdown to `Socks5Server` test utility, fix a sporadic CI failure.
Closes#34849.
The `Socks5Server` utility handles multiple incoming connections, which are handled in separate background threads. Its `stop()` method unblocks and waits for the main background thread cleanly, but it doesn't attempt to wait for the completion of handler threads. There is no guarantee that the handler threads are finished after `stop()` returns, which can lead to IO errors.
In the reported sporadic test failures, the test in `p2p_private_broadcast.py` uses the Socks5 server, and makes a node connect to/through it. Then it stops the Socks5 server, and then it stops the node. However, if a connection handler is still active, that can lead to errors, as the socket is closed.
The change attempts to fix this by storing all handler threads and connections, and attempting to shut them down before `stop()` returns.
Notes:
- ~~I was not able to reliably reproduce the failure locally.~~
Update: I could reproduce the failure, see https://github.com/bitcoin/bitcoin/issues/34849#issuecomment-4126331780 .
Running the relevant test:
```sh
build/test/functional/test_runner.py p2p_private_broadcast.py
```
ACKs for top commit:
andrewtoth:
re-ACK 6ac49373aa
achow101:
ACK 6ac49373aa
vasild:
ACK 6ac49373aa
w0xlt:
ACK 6ac49373aa
Tree-SHA512: c63a62a516925252c450c9b7931539aedac2e44566bd4fe217aa54e6ca1438c8f50a100c714166b7cc67786b00f42c1a36e4916d63311842a77293cd2e102356
07b9b13b45 doc: add integer type conventions in btck api remarks (Alexander Wiederin)
ba6287a449 kernel: align height parameters to int32_t in btck API (Alexander Wiederin)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/34885#discussion_r3093370576
### Summary
Align height-related parameters and return values in the kernel API to use `int32_t`.
### Motivation
The convention in the API is to use fixed-width types like `int32_t` for data values (e.g. heights), and plain `int`/`unsigned int` for boolean-like values and flags. Two functions were not following this:
- `btck_chain_get_height`: header declared `int32_t` but implementation used `int`
- `btck_chain_get_by_height`: both header and implementation used plain `int`
### Changes
- `btck_chain_get_height`: align `.cpp` to match header (`int` -> `int32_t`)
- `btck_chain_get_by_height`: update both header and `.cpp` (`int` -> `int32_t`)
- `CountEntries` in `bitcoinkernel_wrapper.h`: align return type to `int32_t`
- `GetByHeight` in `bitcoinkernel_wrapper.h`: align parameter type to `int32_t`
- Document integer type conventions in the `@page remarks` section of `bitcoinkernel.h`
*Note: This is technically a breaking change for C consumers who have function pointer declarations matching the old signature. The Rust, Go, Java and Python bindings should not be affected.*
ACKs for top commit:
musaHaruna:
Code review ACK [07b9b13](07b9b13b45)
sedited:
ACK 07b9b13b45
stringintech:
ACK 07b9b13
Tree-SHA512: 1c6c5c3113c82f7c5f07871c1aa996b71191d0b190d000d48774a866fcc08514cdabcc13a9de2fed8b5d824da050258db73d73e6aa0b80d828af1533128fe1cd
c54f37c1ba cli, rpc: add -rpcid option for custom request IDs (Torkel Rogstad)
Pull request description:
Add a `-rpcid` CLI argument to `bitcoin-cli` that allows setting a custom string as the JSON-RPC request ID instead of the hardcoded default of 1. This enables correlating requests and responses when debugging or when multiple clients are making concurrent calls.
On the server side, include the request ID in the RPC debug log line.
Tests are added in `test/functional/interface_bitcoin_cli.py` for this.
ACKs for top commit:
maflcko:
lgtm ACK c54f37c1ba
stickies-v:
ACK c54f37c1ba
sedited:
ACK c54f37c1ba
Tree-SHA512: b693a50a2be5dc7797e5b61bddc4c10237888b52065d1b68029d7211542e0dd44f2b3dfb9b3bd7c5e4d9026766817de66a7ef4f29a8d97a268554ba775063e10
3e5dc61035 rpc, refactor: gettxoutsetinfo race condition fix follow-ups (rkrux)
Pull request description:
This patch addresses my own review comments from the review of #34451. If these are found helpful, it makes sense to do them now after the previous PR was merged and backported.
Pasting the comments below that also explains the changes:
- Move the pindex declaration below now that it is not used earlier.
- stats was being generated partially in both these ComputeUTXOStats functions, which reads oddly to me. Now that the pcursor is also moved and passed to this function, which reads oddly as well, I believe we can refactor this function to completely build the stats inside this function. A side benefit is that by removing the stats and pcursor arguments, the function signature becomes quite similar to its namesake, which in turn becomes a straightforward wrapper of this function.
ACKs for top commit:
w0xlt:
ACK 3e5dc61035
sedited:
ACK 3e5dc61035
Tree-SHA512: b8e4a4ebfe4935aa97920cb7068445ea93e571f80e679b8791343ac8750b48484d4288e083e07bf433b397cb6071171a2b77d34758a4627056b34cc63d06f0f4
b02d6b0567 ci: drop -lstdc++ usage in msan fuzz job (fanquake)
655a39ee14 ci: use llvm 22.1.3 (fanquake)
Pull request description:
Upstream seems to have solved their issues.
Tested locally on aarch64 & x86_64.
ACKs for top commit:
maflcko:
lgtm ACK b02d6b0567
sedited:
ACK b02d6b0567
Tree-SHA512: 378a78af65730431ba28424b5c9e8b0b95ec7dbf9a68ced78bcfc39108d75bb9e84121f92d315a6d582c865cf6fb080124fa5eb4c0d16c9b95661ddb401fbcbc
fbffe8a64a bench: improve `VerifyNestedIfScript` benchmark precision (make stack clearing untimed) (David Gumberg)
616ee6fe74 bench: add script verification benchmark for P2TR script-path spends (Sebastian Falbesoner)
Pull request description:
Similarly as #34472 already did for key-path spends, this PR adds a benchmark for P2TR script-path spends. So far we don't have a benchmark on master yet that exercises the verification of taproot commitments ([`VerifyTaprootCommitment`](141fbe4d53/src/script/interpreter.cpp (L1903))).
Note that the tapscript leaf intentionally includes a single OP_CHECKSIG as it likely reflects the real world best. Spending tapscript leafs without any signature checks don't make much sense (they could be trivially tampered with and thus stolen by miners), and doing more than one signature check seems the exception rather than the rule.
The primary motivation for this PR is to evaluate how potential secp256k1 changes in pubkey tweaking (e.g. [#1843](https://github.com/bitcoin-core/secp256k1/pull/1843)) may impact script verification performance.
ACKs for top commit:
davidgumberg:
reACK fbffe8a
l0rinc:
ACK fbffe8a64a
sedited:
ACK fbffe8a64a
Tree-SHA512: 6fa1f2c336d6332b4f2d22173279ee29ad3ec5e5431109913a6978fef32e22a34d3247729ac9092bfdbbcd8f9dfcad8da50bc4ede2cb7efc1f93d6a744ddf41b
fa16bc53d7 test: Require named arg for create_block ntime arg (MarcoFalke)
fab352053d test: Remove unused create_coinbase imports (MarcoFalke)
fad6deb3cb scripted-diff: Use new create_block height option (MarcoFalke)
fa5eb74b96 test: Allow to set height in create_block (MarcoFalke)
Pull request description:
The `create_block` helper is often called with `create_coinbase`: `create_block(prev_hash, create_coinbase(new_height))`
This is fine, but a bit verbose and tedious to type each time. Also, it requires an additional import.
Improve this by allowing to set `height` in `create_block` directly, similar to other options, such as `ntime`, or `hashprev`.
ACKs for top commit:
l0rinc:
reACK fa16bc53d7
theStack:
re-ACK fa16bc53d7
Tree-SHA512: 41ca7327aac714b446d53b1a29f83e792f7fc13d567cd0f063f743d50b193fa0c71cc651454ae1f3c960b0b82cc8658932ea08b3be8632f69a18ccd09a052c17
fae807ed25 test: Remove unused, confusing and brittle connect_nodes.wait_for_connect (MarcoFalke)
fab2772647 test: Fix all races after a socket is closed gracefully (MarcoFalke)
fa21edddb2 test: Stricter checks in rpc_setban.py (MarcoFalke)
faa404e119 test: Add is_connected_to helper (MarcoFalke)
Pull request description:
Currently, functional tests may intermittently fail, due to a lack of synchronization after a node connection socket was closed gracefully: If node A is connected to node B, and node B closes the connection, node A *must* wait for the connection to be closed before continuing the test. Otherwise, subsequent re-connections may not work while a stale connection is still alive.
This can be reproduced locally via something like:
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 6b79e913e8..32bd061500 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2200,2 +2200,3 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
}
+ UninterruptibleSleep(599ms);
pnode->CloseSocketDisconnect();
```
With this diff, the tests should fail on master, and pass after this fix.
The fix is placed inside the `connect_nodes` helper.
ACKs for top commit:
rkrux:
ACK fae807ed25
w0xlt:
ACK fae807ed25
davidgumberg:
(re-ish) crACK fae807ed25
sedited:
ACK fae807ed25
Tree-SHA512: 053825bbd319d7c49e08810bbabbf9c3a43d89897d697c0ca9232fd8a88ae475b4f9fbd79dff549b4e0a8941cf926ca4567e7fd43dc1749bf023d8ee7dd49608
037ea2c714 walletdb: Remove m_mock from SQLiteDatabase (Ava Chow)
59484e2fdb wallet: Make Mockable{Database,Batch} subclasses of SQLite classes (Ava Chow)
b69f989dc5 wallet, bench: Use TestingSetup in CoinSelection benchmark (Ava Chow)
e7d67c9fd9 test: Make duplicating MockableDatabases use cursor and batch (Ava Chow)
964eafb71c bench, wallet: Make WalletMigration's setup WalletBatch scoped (Ava Chow)
Pull request description:
`MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.
This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database implementation.
ACKs for top commit:
brunoerg:
code review ACK 037ea2c714
sedited:
Re-ACK 037ea2c714
furszy:
Code review ACK 037ea2c714
Tree-SHA512: 0a99c27ef4e590966b3af929bf3acf99666861905aabf150fe5660ea07c881a49935a4e7dcd676dcd5e70616898d89d872b6e156ae9c600de1361c1b2469b64d
fc736013a5 rpc: Add in_memory option to dumptxoutset with rollback (Fabian Jahr)
d0fd718948 test: Extend named pipe sqlite tool test to use rollback (Fabian Jahr)
ab9463efac test: Add dumptxoutset fork test (Fabian Jahr)
49d5e835a8 rpc: Don't invalidate blocks in dumptxoutset (Fabian Jahr)
fe58eb9850 blockstorage: Add DeletePruneLock (Fabian Jahr)
Pull request description:
This is an alternative approach to implement `dumptxoutset` with rollback that was discussed a few times. It does not rely on `invalidateblock` and `reconsiderblock` and instead creates a temporary copy of the coins DB, modifies this copy by rolling back as many blocks as necessary and then creating the dump from this temp copy DB. See also https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-1978480989, https://github.com/bitcoin/bitcoin/issues/32817#issuecomment-3012406102 and #29565 discussions.
The nice side-effects of this are that forks can not interfere with the rollback and network activity does not have to be suspended. But there are also some downsides when comparing to the current approach: this does require some additional disk space for the copied coins DB and performance is slower (master took 3m 17s vs 9m 16s in my last test with the code here, rolling back ~1500 blocks). However, there is also not much code being added here, network can stay active throughout and performance would stay constant with this approach while it would impact master if there were forks that needed to be invalidated as well (see #33444 for the alternative approach), so this could still be considered a good trade-off.
ACKs for top commit:
stratospher:
tested ACK fc73601. very nice!
sedited:
Re-ACK fc736013a5
theStack:
re-ACK fc736013a5
Tree-SHA512: d3d674f68184ac3ada87d969d0fca7bc38203ee939853864adcd235ee3a954914c7e351b817800b885a495606e323392c27d88ba8d8e018eaf8567c098eb0e9c
f49a2afd94 test: interface_http follow-ups (Matthew Zipkin)
Pull request description:
This addresses post-merge and slightly-pre-merge comments from #34772 and closes#35082
- Only one node needed for test
- Use ascii encoding instead of utf-8
- Make tests independent of each other
- Expect HTTP error code 413 for too-large request
- Clarify python client race condition in comment
ACKs for top commit:
maflcko:
lgtm ACK f49a2afd94
hodlinator:
re-ACK f49a2afd94
Tree-SHA512: 47014f7e81ba05b3ab83878b26185ae31d541d2d6ddf14c803bed8b9f39535998624d9df733cda8d30f7438564cf241b98f20643a9e0d8e0ef5f72b26ce1a8cd
- Only one node needed for test
- Use ascii encoding instead of utf-8
- Make tests independent of each other
- Expect HTTP error code 413 for too-large request
- Clarify python client race condition in comment
bdc8e496da doc: fix typos and formatting in CONTRIBUTING, i2p, bitcoin-conf, files (Guillermo Fernandes)
Pull request description:
Fix four small documentation issues found in actively maintained files:
- `CONTRIBUTING.md`: "Valid areas as:" → "Valid areas are:" (grammar)
- `doc/i2p.md`: remove double space after period
- `doc/bitcoin-conf.md`: "some negating some lists" → "some negating lists" (duplicate word)
- `doc/files.md`: missing space after comma in `**macOS**,the`
ACKs for top commit:
maflcko:
lgtm ACK bdc8e496da
NellyNakhero:
ACK bdc8e496da
Tree-SHA512: 15a5e284d517f7311a8c7c02a17292504a9eeafbaa81dc12a63a22fa1d49122e437e5647d9d0fbf397b5d150d0586632daccf0d2a7e7775a9bfd38083f0dfdcd
- CONTRIBUTING.md: "Valid areas as" -> "Valid areas are"
- doc/i2p.md: remove double space after period
- doc/bitcoin-conf.md: "some negating some lists" -> "some negating lists"
- doc/files.md: missing space after comma in "macOS,the"
f1e14dfbe9 depends: remove workaround for Make older than 4.2.90 (fanquake)
Pull request description:
This was introduced for distros shipping older `make`, such as Ubuntu `20.04` (`4.2.1`). It's likely that any distros being used for Darwin and Windows cross compilation, are shipping a newer make at this point.
ACKs for top commit:
hebasto:
ACK f1e14dfbe9, I have reviewed the code and it looks OK.
Tree-SHA512: bb5d785e4804f0b0d51c5abba31ee4537cf04247801edef51a5da728c7df7fff1189625d222f91b8f5896f9ec71dc8dbd11614a69e13e0dcad6017cab7dd5874
Previously, it was only possible to set the height indirectly by calling
create_coinbase outside the create_block function. This is fine, but
verbose and not needed.
Just like hashprev can be set directly (instead of using the value from
tmpl), allow height to be set directly.
Also, use it in one place. The other places are done in a scripted-diff.
Also, add a unit test for the new feature.
Add a -rpcid CLI argument to bitcoin-cli that allows setting a custom
string as the JSON-RPC request ID instead of the hardcoded default of 1.
This enables correlating requests and responses when debugging or when
multiple clients are making concurrent calls.
On the server side, include the request ID in the RPC debug log line
when it differs from the default value of 1, so that custom IDs are
visible in the debug.log output.
577a3e74c8 test: Add check for return type in `HasToBytes` concept (yuvicc)
1ad551281a kernel: Add Block Header serialization method (yuvicc)
86662623ec Add `SpanWriter` class for zero-allocation stream writing (yuvicc)
Pull request description:
This adds serialization for `btck_BlockHeader` API. Also, updated the `CheckHandle` to compare the byte content instead of size.
The changes here is done in two commits. First commit adds the `SpanWriter` class and next one moves the block header serialization to `SpanWriter`. See commit message for more details.
Follow-up to #33822 .
ACKs for top commit:
stickies-v:
re-ACK 577a3e74c8
alexanderwiederin:
ACK 577a3e74c8
theStack:
Code-review ACK 577a3e74c8
w0xlt:
ACK 577a3e74c8
Tree-SHA512: 1eda5b204588ccb23e9357f68c5529474e7d248736a371c47d8db71ba6ca95e121869514478ad7a519d190e4c30725f64fd1ef4dd9f97d2627dc4441e51458e0
fa02eb87df test: Add missing self.options.timeout_factor scale in tool_bitcoin_chainstate.py (MarcoFalke)
Pull request description:
Without the factor, the test may fail when run cross-arch, possibly with sanitizers. E.g. in qemu-aarch64 in docker in a x86-VM.
ACKs for top commit:
fanquake:
ACK fa02eb87df
Tree-SHA512: 7dbf492de7478bcc95a62576499947fa83031c43496700657014aa9b29f6665c2a226b71287b71b8cd7a5240036c00f069f12ab231539e3387fe86ca298d6933
49895b9cbd kernel: build: remove unused serfloat dependency (Sebastian Falbesoner)
Pull request description:
The serfloat module (more concretely, its two functions `{Encode,Decode}Double`) is only used for [fee estimation](7844a2f083/src/policy/fees/block_policy_estimator.cpp (L21)), which is not included in the bitcoinkernel library, so the linking dependency can be removed.
(Not terribly important in practice, but: this reduces the static library size by ~1.5 KB (~2.7 KB unstripped) on my arm64 Linux machine.)
ACKs for top commit:
davidgumberg:
ACK 49895b9cbd
stickies-v:
ACK 49895b9cbd
Tree-SHA512: c3b4feec55cdc7476f54185db95c94e7365f5feadf09d3740d51dce32a1ceeef4d38ecf5c8cf7ca4ed326a12dab386bb104da978ff06bdf739889b2cfa81603d
Add `btck_block_header_to_bytes` serialization method to serialize a
`btck_BlockHeader` into an 80-byte buffer using `SpanWriter` to ensure
zero-allocation serialization.
To reflect the likely most common real-world scenario, a single
OP_CHECKSIG is contained in the Tapscript leaf.
While touching this benchmark, also set the operation unit to "script"
and do some minor refactorings to deduplicate code and improve readability.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
422ca211ec test: ensure HTTP server enforces limits on headers and body size (Matthew Zipkin)
0c1a07e890 test: ensure HTTP server timeout is not caused by a delayed response (Matthew Zipkin)
f06de5c1ea test: clean up and modernize interface_http (Matthew Zipkin)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/32408 and a new prerequisite for #32061 in response to a few review comments there.
### New test: `check_server_busy_idle_timeout()`
In https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2872150590 it is pointed out that the idle timeout set by `-rpcservertimeout` could disconnect a client unfairly if it was the server that was taking too long to process a response. That misbehavior was confirmed and #32061 was updated to match current libevent behavior. This PR asserts the current libevent behavior by adding another test using RPC `waitforblock` past the `-rpcservertimeout` value and then verifying that the HTTP connection is still open.
### Expanded tests: `check_excessive_request_size()` and `check_chunked_transfer()`
https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2941508964 made me realize that I was not testing HTTPRequest body size at all, and that headers size was being tested one line at a time but not in total. The libevent server does both things so that behavior is asserted in these expanded tests, and the mechanism in #32061 was improved to match.
### Clean up `interface_http.py`
Since I am extending this test module again I refactored the monolithic test to resemble the current functional test style in the repository, de-duplicating HTTP connection code with a helper class and separating individual test cases into functions.
ACKs for top commit:
fjahr:
ACK 422ca211ec
Bortlesboat:
re-ACK 422ca211ec. Checked out the rebased branch and ran interface_http.py locally, passes clean. Went through all three commits -- the helper class deduplication reads well and check_server_busy_idle_timeout is a nice addition from the #32061 discussion. One minor note inline.
hodlinator:
Concept ACK 422ca211ec
theStack:
ACK 422ca211ec
Tree-SHA512: 1165c9c49c8b1ae5a40a15e1d0f8275bb4d5e08a5eea7ae8b9d900bb34a85b29ba89c728b5e0866fbf289dd49aa5ece0af63e410e64aee70c89a19759c5ea3ff
The `Socks5Server` utility handles multiple incoming connections,
which are handled in separate background threads.
The `stop()` method unblocks and waits for the main background thread
cleanly, but it doesn't attempt to wait for any handler threads.
This change stores handler threads and connections, and attempts
to shut them down before `stop()` returns.
Co-authored-by: vasild <vd@FreeBSD.org>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
8783cc8056 refactor: inline `CCoinsViewBacked` implementation (Lőrinc)
86296f276d coins: make `CCoinsView` methods pure virtual (Lőrinc)
b637566c8d coins: add explicit `CoinsViewEmpty` noop backend (Lőrinc)
90c635c01c fuzz: keep backend assertions aligned to active backend (Lőrinc)
a9f92e3497 refactor: normalize CCoinsView whitespace and signatures (Lőrinc)
38a99f3344 scripted-diff: normalize `CCoinsView` naming (Lőrinc)
06172ef0d5 refactor: rename `hashBlock` to `m_block_hash` to avoid shadowing (Lőrinc)
Pull request description:
### Problem
`CCoinsView` is the coins view interface, but historically it also provided built-in no-op behavior:
* It provided default no-op implementations (returning `std::nullopt`, `uint256()`, `false`, or `nullptr`) instead of being pure virtual.
* Callers could instantiate a bare `CCoinsView` and get silent no-op behavior.
* Mixing the interface definition with a built-in dummy implementation blurred the abstraction boundary.
### Context
This is part of the ongoing coins caching cleanup in #34280.
### Fix
This PR separates the interface from no-op behavior and makes `CCoinsView` pure virtual in incremental steps:
* Add `CoinsViewEmpty` as an explicit no-op coins view for tests, benchmarks, and temporary backends.
* Replace direct bare-`CCoinsView` test and dummy instantiations with `CoinsViewEmpty`.
* Make all `CCoinsView` methods pure virtual (`PeekCoin`, `GetCoin`, `HaveCoin`, `GetBestBlock`, `GetHeadBlocks`, `BatchWrite`, `Cursor`, `EstimateSize`).
* Remove the legacy default implementations from `coins.cpp`.
* Update fuzz and dummy backends to use `CoinsViewEmpty` explicitly.
ACKs for top commit:
w0xlt:
reACK 8783cc8056
ryanofsky:
Code review ACK 8783cc8056. Just updated comments and variable name since last review. The fuzz test code is much clearer now IMO
andrewtoth-exo:
ACK 8783cc8056
ajtowns:
ACK 8783cc8056
Tree-SHA512: cfc831578aa309788c1b5dafbfecca3de388cc4215534c3f3df24f90d7770ed37b1fd7aa134df91d611d0a1ca75929accb98d5ed7df7b52851c259e04f08e4a3
