💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137732694)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Note for reviewers: this is preserving previous behavior. Default value for `CAddressBook::purpose` field previously was `"unknown"` when a purpose was not recorded, and now it is null which is translated to `"unknown"`

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137736576)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

If not updating the code above, would be good to add `assert(purpose)` or a similar check here, because that condition would indicate a bug.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137754658)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

This change should be moved to previous commit "wallet: add AddressPurpose enum to replace string values" (2932d7be3cdf3de79b2767735a91180192d4efce), instead of being here. Alternately, could also keep the fixed width type, though IMO it is not helpful.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137745598)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

I think it might be a little better to drop the IsValidPurposeString function and just write:

```
purpose = PurposeFromString(purpose_str);
if (!purpose) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid purpose argument, must a known purpose string, typically 'send' or 'receive'.");
}
```

since underlying code should work for any past purpose string like "refund", or f
...

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137760371)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

s/spend/spent/

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1137756043)
In commit "wallet: Replace use of purpose strings with an enum" (db521d3e89b23046f73410a875bb9660178e3842)

Could add the same comment suggested previously here above NotifyAddressBookChanged ("In very old wallets, address purpose may not recorded, so derive it from IsMine value")

👍 ryanofsky approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
Code review ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6. Only changes since last review were recent review suggestions.

💬 Xekyo commented on pull request "wallet: 25806 follow-up":
(https://github.com/bitcoin/bitcoin/pull/27227#issuecomment-1470869150)
utACK 475c20aa568d597c7850c784058596ae26f37496

💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1137792608)
I have now added test coverage for the lock as well.

💬 LarryRuane commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1137821241)
```suggestion
if (RPCExecutor::executeConsoleOnlyCommand(executableCommand, wallet_model)) {
return;
}
```
(or could put `return` on the same line)

💬 LarryRuane commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1137822858)
these can be removed

💬 LarryRuane commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1137822991)
```suggestion
const std::vector<std::string> parsedCommand{parseHelper(command)};
```

💬 LarryRuane commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1137827055)
That's strange, when I run `clang-format-diff`, it moves this left, where they should be:

<details>
<summary>click to expand</summary>

```
$ git show -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
Formatting src/qt/rpcconsole.cpp
$ git diff
diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 1408b289f..63820464b 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -419,12 +419,12 @@ void RPCExecutor::request(const QString &command, const WalletMo
...

💬 achow101 commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#issuecomment-1470936630)
ACK fadf8b818226dc60adf88e927160f9c9680473a4

🚀 achow101 merged a pull request: "Remove almost all blockstorage globals"
(https://github.com/bitcoin/bitcoin/pull/25781)

💬 achow101 commented on pull request "wallet: 25806 follow-up":
(https://github.com/bitcoin/bitcoin/pull/27227#issuecomment-1470952578)
ACK 475c20aa568d597c7850c784058596ae26f37496

🚀 achow101 merged a pull request: "wallet: 25806 follow-up"
(https://github.com/bitcoin/bitcoin/pull/27227)

💬 achow101 commented on pull request "rest: add verbose and mempool_sequence query params for mempool/contents":
(https://github.com/bitcoin/bitcoin/pull/26207#issuecomment-1470979803)
ACK 1ff5d61dfdaf8987e5619162662e4c760af76a43

🚀 achow101 merged a pull request: "rest: add verbose and mempool_sequence query params for mempool/contents"
(https://github.com/bitcoin/bitcoin/pull/26207)

💬 ishaanam commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#discussion_r1137912817)
In 7cb4dc157a711210e18f3c9b492150b6eb984b30 " bumpfee: enable send coins back to yourself "

While this certainly looks correct, I think there is more concise way of doing this within the outputs loop before this one. I've implemented it here if you would like to take a look: https://github.com/ishaanam/bitcoin/commit/5b1017c1ea11c8fe2351a9110e4c6ddad6e77f8a. The advantage of this would be that the sanity check would not be required and imo it would be a bit more readable.