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

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

Ah, I see. I wonder if the shasum of the full content can be used as an id for the translation string. Conceptually it seems simpler to get a stable id, than to try to artificially number and re-number the strings, depending on the history. (Obviously this wouldn't help with https://github.com/bitcoin/bitcoin/pull/33224, but this instance should be trivial to
...

💬 fanquake commented on pull request "[29.x] depends: remove xinerama extension from libxcb":
(https://github.com/bitcoin/bitcoin/pull/33238#issuecomment-3239301554)
> We could keep things as they are, treating this dependency as an artifact of the legacy Qt5-based GUI.

If that's the decision, then #32097 should be addressed for all branches.

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3239432543)
Many thanks for the review and the suggested fixes!
Squashed most the fixes into the above commits and force-pushed: https://github.com/bitcoin/bitcoin/compare/4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69...5e1c80a22f3d3c6ecf6d4e2777d67656423dd3f3

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2312042115)
https://github.com/bitcoin/bitcoin/compare/4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69...5e1c80a22f3d3c6ecf6d4e2777d67656423dd3f3

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2312042183)
Good catch, will investigate.

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2312042250)
https://github.com/bitcoin/bitcoin/compare/4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69...5e1c80a22f3d3c6ecf6d4e2777d67656423dd3f3