🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2439119880)
reACK 5736d1ddacc4019101e7a5170dd25efbc63b622a

📝 maflcko opened a pull request: "refactor: Prepare compile-time check of bilingual format strings"
(https://github.com/bitcoin/bitcoin/pull/31295)
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#issuecomment-2479397661)
Rebased 1e9ea7de0b5ee6d3179930d4fc93c8e276e525bd -> 85df2fbf26c73f97f85797868155247c11a4ccd6 ([`pr/bfmt.4`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.4) -> [`pr/bfmt.5`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bfmt.4-rebase..pr/bfmt.5)) after #31287 (just for clarity, there were no merge conflicts)

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844161051)
micro-nit: less important but a case that would be nice is showing multi-utxo spending txs show up as multiple spends

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844164966)
last test nit: for test cleanliness it'd be nice if we knew that the mempool was empty each sub-case

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2439149936)
Code review ACK f4df783f1ca22d96476d52ec5d1929547691ba13. Just rebased and reordered commit since last review

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844167729)
```suggestion
{RPCResult::Type::STR, "address", "The address being spent from, empty string if none"},
```

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844172214)
Well you can have blockhashes *and* search mempool, so I'm -1 on this suggestion. "Can be empty for mempool-only results"?

📝 ryanofsky opened a pull request: "wallet: Translate [default wallet] string in progress messages"
(https://github.com/bitcoin/bitcoin/pull/31296)
Noticed while reviewing https://github.com/bitcoin/bitcoin/pull/31287#discussion_r1843809721 that the [default wallet] part of progress messages remains untranslated while the rest of the string is translated.

Fix this in all places where Wallet::ShowProgress (which has a cancel button) and chain::showProgress (which doesn't have a cancel button) are called by making "default wallet" into a translated string.

💬 ryanofsky commented on pull request "refactor: Avoid std::string format strings":
(https://github.com/bitcoin/bitcoin/pull/31287#discussion_r1844206720)
> Maybe a follow-up can fix this, so that this remains a refactor?

Thanks, I experimented with various ways to clean up this show progress code, and I think it would probably be best if the string formatting was done in the GUI. This would require interface class changes we probably want anyway, but would be a bigger change. For now just went with a simple bugfix translating "default wallet" to match the rest of the string in #31296

💬 laanwj commented on pull request "guix: remove `util-linux`":
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2479481183)
Post-merge ACK
````
b681575760a0d19445a17d0d54f50a65d705923caee3a7cd7502e6611950dc11 guix-build-4d668549825c/output/aarch64-linux-gnu/SHA256SUMS.part
7af1420fedd8b4d10df650e92840287106dd74c289b6ef6fd2fd039387824647 guix-build-4d668549825c/output/aarch64-linux-gnu/bitcoin-4d668549825c-aarch64-linux-gnu-debug.tar.gz
bd6520732b5c7805002a3227fe2d16f56a77453d913f5204e5be869aaf4a9534 guix-build-4d668549825c/output/aarch64-linux-gnu/bitcoin-4d668549825c-aarch64-linux-gnu.tar.gz
83230468eb9289d2
...

👍 ryanofsky approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2439241478)
Code review ACK b28972cd85e4472b386349d6cda8c233faeffd4f

💬 ryanofsky commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1844221466)
In commit "Add destroy to BlockTemplate schema" (b28972cd85e4472b386349d6cda8c233faeffd4f)

note: Missing a space before the number this line

📝 Sjors opened a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)
It's useful to be able to wait for an active chaintip to appear, by calling ` waitTipChanged(uint256::ZERO);`.

Unfortunately this doesn't work out of the box, because `notifications().m_tip_block` is `ZERO` until a new block arrives.

Additionally we need an early return before calling `wait_for`.

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479536761)
cc @TheCharlatan life would be siugnifancly better if `notifications().m_tip_block` just jumped to the tip as soon as there is one, rather than wait for the next block to come in. That applies to #31283 too.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2479552572)
Added early return logic. Similar to #31297 this would be less tedious if `notifications().m_tip_block` was set the tip earlier.

💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309)
I wonder if it is clearer to make `m_tip_block` nullopt, instead of a magic value? Would be nice to see the diff and pick the better one.

🤔 furszy reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2434167216)
Half way through it. Small comments so far.

💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841007320)
nit:
if you move this to use an structured binding, could use the script id directly instead of calculating it on every ´ScriptHash(script)´ call.

e.g.

```c++
for (const auto& [id, script] : mapScripts) {
// ... stuff ...
spks.insert(GetScriptForDestination(ScriptHash(id)));
// ^^ this does not perform the extra Hash160(script) call.
}
```

💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844166304)
Note for reviewers:
This is because we store scripts (witness programs, witness scripts and redeem scripts) in a map whose key is a `CScriptID` -> which is a Hash160 of the script -> which is a `RIPEMD160(SHA256(script))`.
As P2WSH are in the form of `OP_0 <SHA256(witness_script)>`, the SHA256 part is already there and we only need to perform the RIPEMD160 calculation to obtain the `CScriptID`.