To minimize chance of erroneous nullptr dereference, `CChain::Next()`
is changed to take a reference instead of a pointer.
Call sites have been adapted. Notably, NextSyncBlock() now checks
the FindFork() result before calling into Next(), because
the fork lookup may return null.
The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
To avoid this possibility, its input is changed to a reference instead.
Call sites are adapted accoringly, extra nullptr-check is added as
needed.
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
The new cache is keyed with the hash of 'vcpkg.json', which reduces
cache storage consumption compared to keying by run ID.
The `vcpkg/downloads/tools` subdirectory is excluded to further save
space.
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>
