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

💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844244829)
Is this being tested? It seems we have P2WSH(multisig) tests inside `ismine_tests.cpp` and `wallet_migration.py` but these type of script should be covered inside the first loop already. Because if the wallet owns the P2WSH, it will also contain the inner multisig script, which will appear in the first loop.

💬 adamandrews1 commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2479591513)
I am working on this issue, will issue a patch soon.