⚠️ ishaanam opened an issue: "Creating a Wallet Feature Guidelines Document"
(https://github.com/bitcoin/bitcoin/issues/28062)
It would be useful to the project if there was a document detailing what features are considered "out of scope" of the Bitcoin Core Wallet. This would make evaluating wallet PRs that add features easier for reviewers. It would also be useful to contributors so that they can get a better sense of which features are more likely to get merged. I'm curious if anybody else thinks that such a document would be useful, and if there are any guidelines that should be added. These guidelines would probabl
...

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629250320)
> Don't revert the commit; just remove the stray file and squash.

I think it should be fine now, can you please check once again?

💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629252705)
You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629255377)
> You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

I think I did squash them, atleast that's what it shows in the commits tab.

💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629257234)
Apologies, you did indeed!

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629260496)
> Apologies, you did indeed!

No worries, thanks for the guidance throughout this PR!

👍 theStack approved a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1522431786)
Code-review ACK 0bf87476f55dceb106563156c7c8d6bfb8162e29

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629302650)
> Apologies, you did indeed! The commit title is "Deleted stray file `vcpkg`" now, though.

Should I amend this commit or leave it as it is?

👍 theStack approved a pull request: "test: Check expected_stderr after stop"
(https://github.com/bitcoin/bitcoin/pull/28028#pullrequestreview-1522532024)
ACK faf902858d38150caa8991b0ab9d7cfee2905684

Makes sense to do the stderr check at the very end after shutdown.

💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629352299)
Yes, amend the commit title to be accurate. You can rewrite the entire PR at any point.

💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258032967)
nit
```suggestion
//! Result type for use with std::variant to indicate that an operation should be interrupted.
```

🤔 stickies-v reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1521747346)
Concept ACK

💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258600239)
I don't see in which scenario `blockTip()` would return `kernel::Interrupted` here, but the docs seem to indicate that it could. I'd suggest to either update the docs to reflect that we never expect this to interrupt, or add a logging statement similar to what's done in `ThreadImport`?

💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258586027)
nit: would `SetNotificationArgs` be a better name to clarify the side effects this function has?

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258613252)
Done where the code was more than just moved.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258613989)
Perhaps, but I think we should test it in this test as it's important to have the upgrade and downgrade tests for the older wallet version.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258614077)
Done

👍 ryanofsky approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1522637462)
Code review ACK 4b405318d4c476351cccc30588f9126edd94cc35

💬 ryanofsky commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#discussion_r1258649315)
In commit "test: indexes, fix on error infinite loop" (4b405318d4c476351cccc30588f9126edd94cc35)

I found "As index sync failures" comment difficult to understand because it was hard to see how it directly related to code. Would suggest something more literal like "Assert shutdown was not requested to abort the test, instead of looping forever, in case there was an unexpected error in the index that caused it to stop syncing and request a shutdown"

💬 ryanofsky commented on pull request "wallet: address book migration bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28038#discussion_r1258636424)
This looks ok, but I think would have been more straightforward to write as:

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8fa93b97d655..ac6520e95a29 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4059,10 +4059,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
WalletBatch batch{wallet.GetDatabase()};
for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
auto
...