💬 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
...

💬 dergoegge commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684377201)
Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.

As a side note, this test does too many things at once in my opinion and should probably be split up.

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835)
> Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... _shrug_

Fixed in https://github.com/bitcoin/bitcoin/pull/30485 (but not yet removing the `m_started_new_line` to avoid a merge conflict here). I'll do it in a follow-up, after this one is merged, or if I get the OK to do it before this one is merged.

In any case, according to the cove
...

💬 dergoegge commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684384876)
```suggestion
FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
```

💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684401200)
> Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.

Good idea. Done!

👍 dergoegge approved a pull request: "fuzz: Limit parse_univalue input length"
(https://github.com/bitcoin/bitcoin/pull/30473#pullrequestreview-2188274893)
utACK fa80b16b20dffcb85b80f75fee64ed333f2062f9

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2239221514)
Rebased, and fixed an issue with the most recent push. Guix build (aarch64):
```bash
c7aa6bd428ba4ea1c925dafe4df3505ad92e84a196b17c8cb7965d7db231e6a0 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/SHA256SUMS.part
944e734a719886ec26aff12a80be67d28f2c4b7781a341894d3e2ca8477e3497 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/bitcoin-0388ad0d65b6-aarch64-linux-gnu-debug.tar.gz
087ac7a0c1d87a95adacb9fa138aced37a172d4d5199fc75c64831d9f6211972 guix-build-0388ad0d65b6/output/aarch64-linux-gn
...