🤔 janb84 reviewed a pull request: "Release: Prepare "Translation string freeze" step"
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3128184088)
ACK d32fcd3df22671b633cae605ed06c6d0a353e2bf

PR updates translation strings, for 30.0 freeze.

Ran the commands against this PR; got 0 changes (reproduced the results) ✅
Ran the commands against Master; got the changes as provided in this PR ✅

🤔 hebasto reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128236494)
It seems no need to track `in_msgstr`:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -34,23 +34,20 @@ function(extract_strings output)
set(messages)
set(msgid)
set(in_msgid OFF)
- set(in_msgstr OFF)

file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
- if(in_msgstr AND NOT msgid STREQUAL "\"\"")
+ set(in_msgid ON)
+ set(msgid "${CMAKE_MATCH_1}")
+
...

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282357987)
I think we need to return a `nullptr` here instead of returning a `btck_BlockTreeEntry` wrapper with a null `m_block_index`. The current behavior causes segfaults when callers try to access the returned entry. What do you think?

💬 fanquake commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3196844620)
Guix Build:
```bash
410caa60435c79c8c862c95acac2e9d29d08ecce8540f8b999dff2c1bac595d6 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/SHA256SUMS.part
177315e236e00d9e8d3d36e2b6fff0ce2d8cfcdd554f35f97547721617782822 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu-debug.tar.gz
a140b60a3e77d6bfcf2ca8a0808f5ca10ef1318bc5480b8401a82727076ac94d guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu.tar.gz
ed981094dbe36e3e
...

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2282385073)
> I ran some benchmarks comparing `.5s` timeout to no timeout and this commit was always sligthly faster.

The reason why `.5` is picked here is to preserve the observable behavior when `print('.', end='', flush=True)` is hit: During the sleep/wait you'll only want a dot every half second, not in a busy-loop without delay. Using a busy-loop also sounds like a plausible explanation for your observed slow down, when decreasing it to `0`.

Other than that, I don't think the exact value here sho
...

💬 sipa commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3196869655)
ACK 732c134bfc1208377f449e5956e01dd8b78d73d9. See also tests added in #33201.

I think #31802 should be seen as a successor to this PR, regardless of which release it goes into.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2282400346)
Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?

Also, it may be good to document what the goal of this function is. Maybe with an `Assert(mempool.GetMinFee()==min_fee)` or similar?

💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282414301)
`GetPrivKeyForSilentPayment` returns `{}` which is a default initialized object if the coin's keys are not available. `std::holds_alternative<CKey>` will pass for the retuen object and add the default intialized CKey to the `plain_keys` array.

💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282367304)
https://github.com/bitcoin/bitcoin/pull/28201/commits/7c1093b08c0363a6d341967659f7b49e0f6d0266: We get a segfault later if the keys are not valid. We should check that keys are valid before adding them to `taproot_keys` or `plain_keys`

💬 fanquake commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197036114)
> You'll probably want to include https://github.com/bitcoin/bitcoin/commit/71f29d4fa90aaeb6472b3ce9d4f4e97f85ed487b from that PR here to update the build docs.

You'll also need to update the msan fuzz, valgrind and valgrind fuzz CI jobs (run in qa-assets / elsewhere), otherwise they wont work, after this is merged.

💬 furszy commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197067329)
> > Raw idea: I think we can still rewind the indexes even without the block data
>
> How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.

It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bitcoin/blob/d31d
...

💬 fjahr commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197122941)
> > > Raw idea: I think we can still rewind the indexes even without the block data
> >
> >
> > How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.
>
> It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bi
...

👍 hebasto approved a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128665447)
ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86.

Another line can be saved:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -33,7 +33,7 @@ function(extract_strings output)

file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")

- set(messages)
+ set(messages "\"${COPYRIGHT_HOLDERS}\"")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
set(msgid "${CMAKE_MATCH_1}")
@@ -63,8 +63,6 @@ static const char UNUSED *bitcoin_strings
...

💬 Crypt-iQ commented on issue "ci: failure in `logging_tests`":
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3197223283)
> This is however not what happened in https://cirrus-ci.com/task/5318274667249664?logs=ci#L721, as the test started and failed all in a span of a few milliseconds.

I think the CI logs are a bit weird and are printing the time from the boost test log -- the test actually took ~22.7 seconds: `[09:01:31.798] [1;34;49mtest/logging_tests.cpp(448): Leaving test case "logging_filesize_rate_limit"; testing time: 22705157us`. This would be enough to trigger the `Reset`. I am not sure why it took this
...

🤔 danielabrozzoni reviewed a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3124489895)
Thanks for the reviews!

In my last push:
- Rebased onto the right commit :sweat_smile: (see https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269846966)
- Addressed review comments

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281794387)
Ooops, not sure what happened there, thanks for noticing!

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281924397)
Thanks, I didn't like the initial formatting either, but I thought it was common (looking at other constructors in the repository, no, it's not...)

I updated the formatting both in `HeadersSyncState` and `CompressedHeaders`!

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2279448653)
I think it fits slightly better with the span change... since in c++20 span doesn't have a `cbegin` method, we needed to change the initial code, and update to `ranges::all_of`

🤔 janb84 reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128722396)
re ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86

changes since last ACK:
- dropped shebang
- script optimizations
- ifdef style change (thanks for including my nit)

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282651470)
If `0` is success, we should invert this, right?

```suggestion
return result ? 0 : 1;
```
The earlier error returns should also be updated from `return 0;` to `return 1;` if I am not wrong.