👍 l0rinc approved a pull request: "build: add `-Wleading-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482#pullrequestreview-3169950067)
Core review ACK 8c60dc39a353c9d66cbe3a5a0a934eca6b2a287a

💬 l0rinc commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2311047700)
nit: there are a few remaining trailing spaces e.g. in https://github.com/bitcoin/bitcoin/blob/07f0a61ef711a2f75ded3d73545bfabdf2a64fef/src/minisketch/src/fields/clmul_common_impl.h#L39, we could add a PR to https://github.com/bitcoin-core/minisketch to get rid of those as well

💬 l0rinc commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2311037992)
nit: given that we're fixing non-trailing tabs as well, the only remaining .cpp/.h file that still contains tabs is https://github.com/bitcoin/bitcoin/blob/75a5c8258ec5309fe506438aa3815608430b53d6/src/util/subprocess.h#L1149, if you edit again, consider including that

💬 l0rinc commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2311103606)
> Admittedly they are imprecise

I'd say ~85000 vs ~400 is a bit more than imprecise :)

> almost double on same line

That's not how we usually decide outcomes, rather by better explanations - especially since many of those aren't counting declarations, e.g
* https://github.com/bitcoin/bitcoin/blob/f7d2db28e90297664390d527881ebbbf2c6ce6f0/src/bitcoin-cli.cpp#L241
* https://github.com/bitcoin/bitcoin/blob/face8123fdc10549676c6679ee3225c178a7f30c/src/httpserver.cpp#L565
* https://github.
...

👋 l0rinc's pull request is ready for review: "stabilize translations by reverting old ids by text content"
(https://github.com/bitcoin/bitcoin/pull/33270)

🤔 enirox001 reviewed a pull request: "test: Fixup fill_mempool docstring"
(https://github.com/bitcoin/bitcoin/pull/33269#pullrequestreview-3170325100)
ACK fa3f682 - docstring cleanup, looks good.

💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#issuecomment-3238341125)
> Are there any stats on increased coverage, or is this just checking more values in the same paths?

I'm going to share a coverage report but yes, it increases the coverage and reaches 100% of `wallet/fees.cpp`. One of the motivations for this PR is that I noticed that the following code was uncovered due to mempool min fee being always zero:

```cpp
// Obey mempool min fee when using smart fee estimation
CFeeRate min_mempool_feerate = wallet.chain().mempoolMinFee();
if (feerate_needed <
...

👍 pablomartin4btc approved a pull request: "test: Fixup fill_mempool docstring"
(https://github.com/bitcoin/bitcoin/pull/33269#pullrequestreview-3170406861)
ACK fa3f682032a3292604f363a5ee4557937f3d8950

(_Or adding back the assert correcting the value (0.00000100) and the comment (0.1)?_)

💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3238407406)
@maflcko, this is for every translation update, after this change previously translated values are kept and recognized by Transifex.

@achow101 created a test Bitcoin Core translation sandbox for me where I've uploaded the latest `bitcoin_en.xlf`.
Between Core versions most of the translations need to be reassigned (in the example of 29 vs 30 only 10% of the original IDs were kept so the translations all showed that most of the strings don't have pairs)
<img src="https://github.com/user-atta
...

💬 l0rinc commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3238412102)
I have checked the problem against the actual Core translations on Transifex and I could reproduce the massive invalidations and https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3238407406 does fix it successfully so that translators only need to check the modified entries:
<img src="https://github.com/user-attachments/assets/a90798da-fab9-4732-bc4a-075711e10558" />

🤔 w0xlt reviewed a pull request: "test: Use extra_port() helper in feature_bind_extra.py"
(https://github.com/bitcoin/bitcoin/pull/33260#pullrequestreview-3170731462)
ACK https://github.com/bitcoin/bitcoin/pull/33260/commits

Adding the patch below can confirm that the condition described in https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225111710 is reached.

```diff
diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py
index 7dfa9c06ee..48b31092d2 100755
--- a/test/functional/feature_bind_extra.py
+++ b/test/functional/feature_bind_extra.py
@@ -27,7 +27,7 @@ class BindExtraTest(BitcoinTestFramewor
...

💬 w0xlt commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2311675883)
If you apply the patch below, the patch above will work.
Is there a reason why the starting port index should be the same as the number of nodes?

```suggestion
extra_port.index = 0
```

💬 l0rinc commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2311676593)
We're introducing extra confusion here now with assigning different types to the same variable.
I'm not suggesting a "rewrite", just to make it better than it was before - which it mostly is, hence the concept ack from me.

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311525928)
what's the reason for keeping the old indentation here?

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311506722)
Removing this changes the scheduling overlap to be more unpredictable since the new tasks will be scheduled immediately. That's what we want it to do to avoid wasted cycles, but I can imagine this will result in new test failures we haven't seen before.

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311544368)
It's likely not strictly needed, but if you edit again consider `self.executor.shutdown(wait=True)` once all tests are done

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311678930)
I find this confusing - as mentioned below -, we're replacing a value with a different typed one. Is the mutation strictly necessary or would something like
```python
def proc_wait(task):
name, start_time, proc, testdir, log_out, log_err = task
return_code = proc.wait()
return [name, start_time, return_code, testdir, log_out, log_err]
```

also work?

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311565152)
this is a bit confusing now, this isn't the `proc` anymore, it's the return value of the `proc` now

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311575990)
formatting is off now that the leading `(` was removed

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2311551003)
these extra commas make reformatted code look funny
```python
stderr=log_stderr,
),
testdir,
```

💬 Raimo33 commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3238765987)
Concept ACK
Approach ACK
ACK 585fa7e1e3352ad5c2293ade2dbd1185f6979ac4

I haven't tested the code but I have reviewed it and it looks reasonable; i think the approach is sound and the two class design better addresses the matter.