💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1684216567)
Oh, I see. You are saying that valgrind is not supported on Windows and hard links in the test framework are not supported on Linux, so there is nothing to be done here?

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1684221025)
(Resolving discussion for now. Thanks for explaining and sorry for the lengthy replies.)

💬 TheCharlatan commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2238921578)
Rebased 049719b3858f2ae88cc2752ffe240b667c3d2996 -> 598dd9c3df8afbab92f706e1d4ce72bd3fa83e95 ([noNetChainTest_0](https://github.com/TheCharlatan/bitcoin/tree/noNetChainTest_0) -> [noNetChainTest_1](https://github.com/TheCharlatan/bitcoin/tree/noNetChainTest_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noNetChainTest_0..noNetChainTest_1))

* Fixed conflict with #30407 , and adapted commit changes to use the new options

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#discussion_r1684225319)
Looks like this is called twice, no?

💬 TheCharlatan commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2238962032)
Updated 598dd9c3df8afbab92f706e1d4ce72bd3fa83e95 -> f46b2202560a76b473e229b77303b8f877c16cac ([noNetChainTest_1](https://github.com/TheCharlatan/bitcoin/tree/noNetChainTest_1) -> [noNetChainTest_2](https://github.com/TheCharlatan/bitcoin/tree/noNetChainTest_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/noNetChainTest_1..noNetChainTest_2))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/30399#discussion_r1684225319), removed redundant call for adding th
...

💬 TheCharlatan commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#issuecomment-2238964166)
Updated eb3d9cd7ab2fa3d588325ec3352ce914c1d94880 -> ae2a4c8f6e0b44cd69b057ae8bc3d542786413e4 ([fuzzScriptCache_0](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_0) -> [fuzzScriptCache_1](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/fuzzScriptCache_0..fuzzScriptCache_1))


* Addressed @dergoegge's [comment](https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1677491573), keeping testing setup init
...

💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2238995080)
Guix Build (x86_64, aarch64):
```bash
cd661a8196c71db10430898b78555489f2fb5a1b1e35bb633c18a5d21692bc3d guix-build-6ccf95586047/output/aarch64-linux-gnu/SHA256SUMS.part
8c340c73ed52b60db257032f18cc39db2ffee35f78c67f8fe4dc67557f33ebb7 guix-build-6ccf95586047/output/aarch64-linux-gnu/bitcoin-6ccf95586047-aarch64-linux-gnu-debug.tar.gz
f77bf0c350d58b9a8755e1fab3c560e5aa1e5202181ddab0c098d3f9da96863f guix-build-6ccf95586047/output/aarch64-linux-gnu/bitcoin-6ccf95586047-aarch64-linux-gnu.tar.gz
...

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2239012205)
review ACK f46b2202560a76b473e229b77303b8f877c16cac

Seems fine to do. I haven't benchmarked this commit, but I expect the peak memory usage of the coins_view fuzz target to go down. Also, the utxo_snapshot fuzz target may be faster and use less peak memory.

(When I benchmarked an earlier version of this, I didn't find a significant speedup, but I may have done something wrong and the changes here seem useful either way)

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1684250867)
nit: typo
```suggestion
// Perform equality checks only if SearchCandidateFinder claims an optimal result.

Comment

```

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1684218549)
```suggestion
Assume(!AllDone());
std::optional<ClusterIndex> best;
```

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1684253359)
why is 12 selected here, does this implies that the optimal search done with `todo.Count()` > 12 using `SearchCandidateFinder` and `SimpleCandidateFinder` are not optimal we can do better with exhaustive search?

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1684261836)
Add span include here and the tests
```
#include <span.h>
```

👋 fanquake's pull request is ready for review: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)

🤔 stickies-v reviewed a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2187915974)
Approach ACK 9984bdc83b4a187629f26f35e4ee00e35d2ac04d

Straightforward improvement to make the code more robust, very nice.

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684336117)
Perhaps worth it to introduce the `FromHex` method you [mentioned](https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787) here already, as it's not much extra LoC and probably makes for a more final interface? We can just do it for `transaction_identifier` now and do `base_blob` later?

<details>
<summary>git diff on 5ac06abeb9</summary>

```diff
diff --git a/src/rest.cpp b/src/rest.cpp
index b0d0d928f6..ff06c4191f 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -792,14
...

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684333878)
review note nit: perhaps worth adding in the commit message that this implementation is ~identical to `ParseHashStr` (even though it's later deleted)?

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684188133)
nit: or just initialize directly?

<details>
<summary>git diff on 0d57f058e4</summary>

```diff
diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp
index efca540240..0a9137636a 100644
--- a/src/test/pow_tests.cpp
+++ b/src/test/pow_tests.cpp
@@ -83,31 +83,28 @@ BOOST_AUTO_TEST_CASE(get_next_work_upper_limit_actual)
BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_negative_target)
{
const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus();
-
...

👍 dergoegge approved a pull request: "locks: introduce mutex for tx download, flush rejection filters once per tip change"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2188198953)
Code review ACK c85accecafc20f6a6ae94bdf6cdd3ba9747218fd

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824)
In addition to the `-bind` [thing](https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236) there is also a complication with the port: if `-bind=10.0.0.1:20000 -bind=[5566::1]:30000` has been used, then `GetListenPort()`, currently used by `mapport.cpp`, is not the right thing to do because it would return `20000`.

To resolve both, we could get the listening addresses + the correct ports from:
1. `AddLocal()` / `mapLocalHost`, is called after bind (good), but also from other pl
...

📝 maflcko opened a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485)
`NOLINT(bitcoin-unterminated-logprintf)` is used to document a missing trailing `\n` char in the format string. This has many issues:

* It is just documentation, assuming that a trailing `\n` ends up in the formatted string. It is not enforced at compile-time, so it is brittle.
* If the newline was truly missing and `NOLINT(bitcoin-unterminated-logprintf)` were used to document a "continued" line, the log stream would be racy/corrupt, because any other thread may inject a log message in the
...