💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844010301)
should also test invalid descriptors

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844122547)
would be great if there was a case showing flipping descriptor ordering doesn't change result ordering

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844118607)
testing that this is results in the call getting rejected even if it's f.e. the second blockhash in a list is also a good idea

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844117146)
should also test that repeated blockhashes is acceptable(AFAICT it just repeats the event)

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844115700)
block order matters, run this case twice, once with blocks in reverse history order?

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844043247)
I think the lift for downstream wallets to encode spk to address is trivial, so I'd rather no expose "send from" type addresses, but I don't feel super strongly about it.

If not, making the address field an optional return as @tdb3 says is my ask. Makes it a little more clear conceptually what's being exposed.

regardless of the result here, there should be test coverage for an output with no address type and the delta from the other test case `test_multiple_addresses` kind of demonstrates
...

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844026636)
I think it deserves the coverage

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1844126187)
another nice case would be demonstrating the order of activity results intra-block (order in block seems to be respected)

can either set fees differentially, or explicitly construct a block via `node.rpc.generateblock` to be quick about it

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-2479385402)
> The release note helps a lot. To err on the side of caution, it seems appropriate to include a `-deprecatedrpc=` option, to enable a period of deprecation for users.

I attempted this, but it's a bit non trivial with some if-else branching. I also had to add a new `TransactionError` enum type to handle `sendrawtransaction` case.
<details>
<summary> see **untested** diff </summary>

```diff
diff --git a/src/common/messages.cpp b/src/common/messages.cpp
index 5fe3e9e4d86..3142ca07b4c 10
...

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