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.
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
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
3fd68a95e6 scripted-diff: replace remaining Python test equality asserts (Lőrinc)
301b1d7b1f test: add missing `assert_equal` imports (Lőrinc)
06a4176c42 test: convert truthy asserts in `wallet_miniscript` and `rpc_psbt` (Lőrinc)
d9a3cf20a4 test: convert simple equality asserts in excluded files (Lőrinc)
23c06d4e6d test: convert equality asserts with comments or special chars (Lőrinc)
dcd90fbe54 test: prep manual equality assert conversions (Lőrinc)
4f4516e3f6 test: split equality asserts joined by `and` (Lőrinc)
76a5570b36 test: use `in` for two-value equality asserts (Lőrinc)
Pull request description:
### Problem
Plain `assert x == y` is a poor fit for this test framework, `assert_equal()` gives more useful failure output and keeps equality checks consistent with the rest of the functional tests.
### Design
A simple scripted diff cannot safely rewrite all of them because many files use `==` inside larger expressions, such as chained conditions, comprehensions, and other compound assertions. That makes a one-shot mechanical conversion either incorrect or harder to review.
### Fix
This series first rewrites the non-mechanical cases into standalone assertions so patterns are easier to identify, then applies a scripted diff to the remaining cases that are safe to convert mechanically.
Partially fixes https://github.com/bitcoin/bitcoin/issues/23119 by adjusting the `==` case only (which is similar to the `BOOST` alternatives so it's expected to be uncontroversial).
ACKs for top commit:
maflcko:
review ACK 3fd68a95e6🚆
rkrux:
lgtm ACK 3fd68a95e6
theStack:
ACK 3fd68a95e6
Tree-SHA512: 6950ffa044db72d6235305f4c0247254e7e8f57ee1c8300e553953963914a6360ca71569fe315ecb333cf0e62f78b3a24603717f64229783761f8c1b5958fc12
dfd54c959e Squashed 'src/secp256k1/' changes from 57315a6985..7262adb4b4 (fanquake)
Pull request description:
https://github.com/bitcoin-core/secp256k1/pull/1760 was merged upstream, which improves test parallelism, and reduces overall runtime. Our longest running tests are secp256k1 tests (`secp256k1_tests`), so we might want to pull this down, to speedup all of our ctest invocations.
The new upstream code increases output verbosity, i.e the test list is now ~330, and we have output like:
```bash
Label Time Summary:
secp256k1_exhaustive = 19.52 sec*proc (1 test)
secp256k1_noverify_tests = 45.65 sec*proc (91 tests)
secp256k1_tests = 90.55 sec*proc (91 tests)
```
maybe we can/should mute that somewhat?
ACKs for top commit:
hebasto:
ACK b7f9178976.
Tree-SHA512: 098a8b54d3a489e6ad7866f4e5152a5bc6a0e1da69b2a84d8795423e64faa42772cbcb194feac228a547869d2b3be2198aaf96a5e1bb7d45a2e385fd5e78bafd
ea893cff07 doc: fix typo 'parlor' to 'parlance' in developer-notes (ArvinFarrelP)
Pull request description:
Fix a small typo in doc/developer-notes.md.
Replace "parlor" with "parlance" to use the correct term in the context of C++ terminology.
ACKs for top commit:
maflcko:
lgtm ACK ea893cff07
l0rinc:
ACK ea893cff07
Tree-SHA512: 5437e0eb1b40aa1eafe8a0482350ed6259ec3492f64e7ba8d046358a3dd9cf95bfc38825b781350fd321c453cc23dddb6a14fe7f5087e56c77a17e3d3e233de4
faad08e59c test: Use NodeClockContext in more tests (MarcoFalke)
fa8fe0941e fuzz: Use NodeClockContext (MarcoFalke)
fa9f434df8 test: Allow time_point in boost checks (MarcoFalke)
Pull request description:
Currently mocktime is written to a global, which may leak between sub-tests (albeit some tests try to reset the mocktime on a best-effort basis). Also, when advancing it, one has to keep a counter variable around.
Fix both issues by using the recently added `NodeClockContext`, which resets the mocktime once it goes out of scope. Also, it has a method to advance the mocktime by a delta.
ACKs for top commit:
achow101:
ACK faad08e59c
seduless:
Tested ACK faad08e59c
frankomosh:
Tested ACK faad08e59c. Ran all relevant tests, all clean. Also verified that the default-constructor call sites in orphanage_tests and addrman_tests behave identically to the explicit `{Now<NodeSeconds>()}` form.
ryanofsky:
Code review ACK faad08e59c but had a question about dropping +1 in one test below.
Tree-SHA512: bd56931970eed02bfcf3f3593ef64a61a8a1d8cc8adf190d6903b35df0fd7e6d865678c7d5bd23ce53d074cb2cf53a0a19212fdeb593b047dac5561859bc86b0
bb00fd2142 test: use dynamic ports and add coverage in feature_bind_port_discover (b-l-u-e)
4f19508ae7 test: dont connect nodes in feature_bind_port_discover (b-l-u-e)
b8827ce619 net: Fix Discover() not running when using -bind=0.0.0.0:port (b-l-u-e)
Pull request description:
This PR fixes two related issues with address discovery when using explicit bind addresses in Bitcoin Core:
- #31293
- #31336
When using `-bind=0.0.0.0:port` (or `-bind=::`), the `Discover()` function was not being executed because the code only checked the `bind_on_any` flag. This led to two problems:
- The node would not discover its own local addresses if an explicit "any" bind was used.
- The functional test `feature_bind_port_discover.py` would fail, as it expects local addresses to be discovered in these cases.
This PR:
1. Checks both `bind_on_any` and any bind addresses using `IsBindAny()`
Ensures `Discover()` runs when binding to 0.0.0.0 or ::, even if specified explicitly.
2. Ensures correct address discovery
The node will now discover its own addresses when using explicit "any" binds, matching user expectations and fixing the test.
3. Maintains backward compatibility
The semantic meaning of `bind_on_any` is preserved as defined in `net.h`:
> "True if the user did not specify -bind= or -whitebind= and thus we should bind on 0.0.0.0 (IPv4) and :: (IPv6)"
4. Updates the test to use dynamic ports
The functional test `feature_bind_port_discover.py` is updated to use dynamic ports instead of hardcoded ones, improving reliability.
References
- Implementation follows the approach proposed in my [review comment on #31492](https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2941773645).
Closes#31293
The #31336 has the overall test failure and requires both this PR and #33362 to be fully resolved.
ACKs for top commit:
NicolaLS:
tested ACK bb00fd2142
pinheadmz:
ACK bb00fd2142
vasild:
ACK bb00fd2142
achow101:
ACK bb00fd2142
Tree-SHA512: 6f1b0b29cc539e4b49b3594f9ad70be90cfff28e31497a8b85e11d8a2509faaa8ca803e542ea3280588ece5a065c4c54193cec87cad221f1abae34d8b13cfdc5
858a0a9c96 test: Add SRD maximum weight tests (Murch)
fe9f53bf0b test: Add SRD success tests (Murch)
2840f041c5 test: Rework SRD insufficient balance test (Murch)
64ab97466f Test: Add new minimum to tested feerates (Murch)
65900f8dc6 test: Init coin selection params with feerate (Murch)
Pull request description:
This transfers the tests from the old coin selection test framework that was based on ignoring fees to the new coin selection framework that tests the coin selection algorithms with realistic feerates and fees.
The PR also includes a minor improvement of the test framework to allow the CoinSelectionParams used by the tests to be initialized with other feerates, and adds some feerates around the new minimum feerate to the tested feerates.
ACKs for top commit:
achow101:
ACK 858a0a9c96
w0xlt:
ACK 858a0a9c96
brunoerg:
reACK 858a0a9c96
Tree-SHA512: cbd7ed3169d225a0b0f751481ca936a10939ff899c345e9469821d46083b04e835d164c50a72f18851544314611c3d596eb4956c1d86903c22e8b4996b8eb861
1ed1a12402 net_processing: don't modify addrman for private broadcast connections (Vasil Dimov)
Pull request description:
It is best if the internal addrman database is not modified with information coming from private broadcast connections because that information can potentially later be sent via other connections.
instagibbs suggested this change, thank you!
ACKs for top commit:
l0rinc:
code review ACK 1ed1a12402
instagibbs:
ACK 1ed1a12402
achow101:
ACK 1ed1a12402
andrewtoth:
ACK 1ed1a12402
danielabrozzoni:
tACK 1ed1a12402
Tree-SHA512: 13845778445e56e3015a569f3b4165a749011e9dd67dcab38e960a368a0f5d3822173af5e3cb950998326549a021d1c8df644ce6d761cd46dd7597106b9ceec9
2af003ae37 test: Use BasicTestingSetup when TestingSetup is not necessary (Hodlinator)
9ee77701dd refactor(test): Only specify TestChain100Setup in test cases (Hodlinator)
Pull request description:
Improves run-times (4 warmups + 60 runs each):
| test | improvement |
|:---|---:|
| checkqueue_tests | 1.12 |
| merkle_tests | 1.14 |
| orphanage_tests | 1.48 |
| prevector_tests | 1.07 |
| rbf_tests | 1.01 |
| validation_tests | 1.60 |
---
Removed overhead is barely measurable on full test suite runtime sequential `test_bitcoin`or parallel `ctest --test-dir build`.
---
Initial version of PR also extracted a `DataDirTestingSetup` from `BasicTestingSetup`, making the latter not use the disk, but the added complexity didn't deliver sufficient speedups to clearly justify itself (at least not while running on NVMe).
---
Follow-up to https://github.com/bitcoin/bitcoin/pull/34562#discussion_r2918752158
Arguably in a similar vein as #22086.
ACKs for top commit:
l0rinc:
ACK 2af003ae37
achow101:
ACK 2af003ae37
ryanofsky:
Code review ACK 2af003ae37. Changes seem good: switching to minimum required test fixtures and avoiding expensive `TestChain100Setup` usage for whole fixtures.
w0xlt:
ACK 2af003ae37
Tree-SHA512: 56eba7595647447fd1940f205dac37605ae4b88f4947d542f1fc1c6c66791a7dbde244d4943f699dc11463e805725991d9d4db7f9be8c7f4d7054ca9dc2d79e1
ba01b00d45 refactor: use for loops in FindMostWorkChain (stratospher)
aa0eef735b test: add InvalidateBlock/ReconsiderBlock asymmetry test (stratospher)
1b0b3e2c2c validation: remove redundant marking in FindMostWorkChain (stratospher)
Pull request description:
recent PRs like #31405, #30666 mark all `m_block_index` descendants as invalid immediately whenever an invalid block is encountered in `SetBlockFailureFlags`. so by the time we reach `FindMostWorkChain`, the block in `setBlockIndexCandidates` already has `BLOCK_FAILED_VALID` set on it - not just on its ancestor. this means `pindexTest = pindexFailed` whenever `fFailedChain` fires, and the inner `while (pindexTest != pindexFailed)` loop body is never reached!
I think we can remove it but I've just replaced it with `Assume` in this PR for safety + good to document this invariant in case the code changes in future. (noticed by @ stickies-v in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2815053885)
the second commit is unrelated and adds a unit test for the situation in https://github.com/bitcoin/bitcoin/issues/32173
ACKs for top commit:
fjahr:
re-ACK ba01b00d45
optout21:
crACK ba01b00d45
w0xlt:
ACK ba01b00d45
ryanofsky:
Code review ACK ba01b00d45, just tweaking comment and for loop condition since last review.
Tree-SHA512: a8be3c30b1c41b76690d16d850e87e9e71fa6a1ecaa8b90ec997ffee1aace48b336a7009a480cd016103759d79c964b3d761a13ae936523808b2930beb68dae5
It is best if the internal addrman database is not modified with
information coming from private broadcast connections because that
information can potentially later be sent via other connections.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
242b0ebb5c btcsignals: use a single shared_ptr for liveness and callback (Cory Fields)
b12f43a0a8 signals: remove boost::signals2 from depends and vcpkg (Cory Fields)
a4b1607983 signals: remove boost::signals2 mentions in linters and docs (Cory Fields)
375397ebd9 signals: remove boost includes where possible (Cory Fields)
091736a153 signals: re-add forward-declares to interface headers (Cory Fields)
9958f4fe49 Revert "signals: Temporarily add boost headers to bitcoind and bitcoin-node builds" (Cory Fields)
34eabd77a2 signals: remove boost compatibility guards (Cory Fields)
e60a0b9a22 signals: Add a simplified boost-compatible implementation (Cory Fields)
63c68e2a3f signals: add signals tests (Cory Fields)
edc2978058 signals: use an alias for the boost::signals2 namespace (Cory Fields)
9ade3929aa signals: remove forward-declare for signals (Cory Fields)
037e58b57b signals: use forwarding header for boost signals (Cory Fields)
2150153f37 signals: Temporarily add boost headers to bitcoind and bitcoin-node builds (Cory Fields)
fd5e9d9904 signals: Use a lambda to avoid connecting a signal to another signal (Cory Fields)
Pull request description:
This drops our dependency on `boost::signals2`, leaving `boost::multi_index` as the only remaining boost dependency for bitcoind.
`boost::signals2` is a complex beast, but we only use a small portion of it. Namely: it's a way for multiple subscribers to connect to the same event, and the ability to later disconnect individual subscribers from that event.
`btcsignals` adheres to the subset of the `boost::signals2` API that we currently use, and thus is a drop-in replacement. Rather than implementing a complex `slot` tracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly in `std::function`s.
The new tests work with either `boost::signals2` or the new `btcsignals` implementation. Reviewers can verify
functional equivalency by running the tests in the commit that introduces them against `boost::signals2`, then again with `btcsignals`.
The majority of the commits in this PR are preparation and cleanup. Once `boost::signals2` is no longer needed, it is removed from depends. Additionally, a few CMake targets no longer need boost includes as they were previously only required for signals.
I think this is actually pretty straightforward to review. I kept things simple, including keeping types unmovable/uncopyable where possible rather than trying to define those semantics. In doing so, the new implementation has even fewer type requirements than boost, which I believe is due to a boost bug. I've opened a PR upstream for that to attempt to maintain parity between the implementations.
See individual commits for more details.
Closes#26442.
ACKs for top commit:
fjahr:
Code review ACK 242b0ebb5c
maflcko:
re-review ACK 242b0ebb5c🎯
w0xlt:
reACK 242b0ebb5c
Tree-SHA512: 9a472afa4f655624fa44493774a63b57509ad30fb61bf1d89b6d0b52000cb9a1409a5b8d515a99c76e0b26b2437c30508206c29a7dd44ea96eb1979d572cd4d4
0e712b3812 Make DynSock accepted sockets queue optional, with precise lifetime (Matthew Zipkin)
3de02abf3f util/test: Add string_view constructor to LineReader and remove StringToBuffer (Matthew Zipkin)
b0ca400612 string: replace AsciiCaseInsensitiveKeyEqual with CaseInsensitiveEqual (Matthew Zipkin)
8172099293 util: get number of bytes consumed from buffer by LineReader (Matthew Zipkin)
Pull request description:
This is a follow-up to #34242 and is the first few commits of #32061
As review and refinement of the replacement HTTP server progresses, some new utilities were needed and added. This PR updates those utilities as work continues on #32061.
### LineReader
In order to enforce strict limits on the total size of headers in HTTPRequest, we add a method to `LineReader` to give us the total amount of data that has been read from the buffer so far. See https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2949287329
### CaseInsensitiveEqual
HTTP headers are case-insensitive. An early version of #32061 used an unordered_map for this and therefore we needed a comparator struct. However that unordered_map was replaced by a simpler `std::vector` of `std::pair` so we can remove the struct and use methods that already exist in the codebase.
### StringToBytes
`StringToBuffer` was introduced in #34242 to test LineReader but review of #32061 indicated that it would be more optimal to return a span of bytes instead of a vector. See https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2892431378
### Split DynSock constructor for two usecases: listening / accepting sockets
See https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2895891437. DynSock was introduced in #30988 and is not used anywhere in master yet. If it's used as a listening socket, it provides connected sockets. If it's used as a connected socket, it provides I/O pipes. By making the queue of connected sockets optional we can clean up the ownership / lifetime if the class members.
ACKs for top commit:
fjahr:
Code review ACK 0e712b3812
vasild:
ACK 0e712b3812
Tree-SHA512: 234c79a00c03cb3952dce2a3c5e59859bd0cbfc5f0a552ad2065e998320a12b533b06adbe294745c690a9e19c2f5f79bca3aa5a44342ee1820037342799566f2
c74c6cfd84 ci: Match `VCPKG_HOST_TRIPLET` to `VCPKG_TARGET_TRIPLET` (Hennadii Stepanov)
Pull request description:
Using a non-default target triplet in https://github.com/bitcoin/bitcoin/pull/34883 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/34883#issuecomment-4204744932) because packages with `"host": true` in their vcpkg configurations were still picking up the default `x64-windows` triplet, effectively building both release and debug packages.
This PR fixes this regression by setting `VCPKG_HOST_TRIPLET` explicitly.
Upstream issue: https://github.com/microsoft/vcpkg/issues/50927.
ACKs for top commit:
hodlinator:
utACK c74c6cfd84
Tree-SHA512: ee17d800a2db16eccfd2751da296c0c30d4d41f60e94513098be96116aedd83efe258349be824ff1f571c11adffac6a72e4a0c9b8b7278ab736d453ff7c089d6
1401011f71 test: Add test for exceeding max line length in torcontrol (Fabian Jahr)
84c1f32071 test: Add torcontrol coverage for PoW defense enablement (Fabian Jahr)
7dff9ec298 test: Add test for partial message handling in torcontrol (Fabian Jahr)
569383356e test: Add simple functional test for torcontrol (Fabian Jahr)
4117b92e67 fuzz: Improve torcontrol fuzz test (Fabian Jahr)
b1869e9a2d torcontrol: Move tor controller into node context (Fabian Jahr)
eae193e750 torcontrol: Remove libevent usage (Fabian Jahr)
8444efbd4a refactor: Get rid of unnecessary newlines in logs (Fabian Jahr)
6bcb60354e refactor: Modernize member variable names in torcontrol (Fabian Jahr)
a36591d194 refactor: Use constexpr in torcontrol where possible (Fabian Jahr)
Pull request description:
This is part of the effort to remove the libevent dependency from our code base: https://github.com/bitcoin/bitcoin/issues/31194
The current approach tries to reuse existing code and follows roughly similar design decisions. It replaces the libevent-based async I/O with blocking I/O utilizing the existing `Sock` and `CThreadInterrupt`. The controller runs in a dedicated thread.
There are some optional code modernizations thrown in made along the way (namings, constexpr etc.). These are not strictly necessary but make the end result with the new code more consistent.
ACKs for top commit:
achow101:
ACK 1401011f71
janb84:
re ACK 1401011f71
pinheadmz:
ACK 1401011f71
Tree-SHA512: 167f1d98a634524568cb1d723e7bdb7234bade2c5686586caf2accea58c3308f83a32e0705edc570d6db691ae578a91e474ae4773f126ec2e1619d3adf7df622
`CCoinsViewBacked` is a simple delegating wrapper around another `CCoinsView`.
Inline its one-line overrides in `coins.h` so the view hierarchy can be read without jumping between `coins.h` and `coins.cpp`.
`CCoinsView` provided default no-op implementations, which allowed constructing a bare view and silently getting dummy behavior.
Make all interface methods pure virtual and remove the legacy default definitions from `coins.cpp` so callers must choose an explicit implementation.
Move the virtual destructor to the beginning to avoid mixing it between the methods.
No-op backing behavior remains available via `CoinsViewEmpty`.
Introduce `CoinsViewEmpty` as an explicit no-op `CCoinsView` implementation, and define its singleton accessor out of line in `coins.cpp` to avoid `-Wunique-object-duplication` in shared-library builds.`
Use it at call sites that intentionally want a no-op backend instead of constructing anonymous placeholder views.
`CCoinsViewTest` and `CoinsViewBottom` now inherit defaults from `CoinsViewEmpty` (e.g. the unused `EstimateSize()`, which now returns 0).
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
