💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137647274)
> I strongly dislike using the empty string to indicate deletion. I think it would be better to have a separate `RemoveReceiveRequest`.

You are right. This is shitty and I will add a separate method to avoid continuing this pattern.

💬 hebasto commented on pull request "Correct poor grammar in wallet synchronization warning.":
(https://github.com/bitcoin-core/gui/pull/720#issuecomment-1470692898)
Translation files should be modified during the translation process only. Please drop those changes from this PR.

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137679417)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137458690

> I'm not convinced that this needs to be a map. Receive requests are only created by the GUI, and the GUI only creates them for a newly retrieved address, so it shouldn't be possible to have multiple receive requests.
>
> Additionally, receive requests are identified by an int which is being (un)stringified. This could just be an int to avoid that constant conversion in the GUI.

You could definitely be right about
...

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1137722557)
Moved it to a separate function, although I did not split it up into multiple as that does not seem to be actually useful. Also added some additional comments.

👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK db521d3e89b23046f73410a875bb9660178e3842. Thanks for adopting suggested changes. I left some new suggestions, but they are not important and can be ignored.

I think this PR should be straightforward for others to review now, and nice because it should demystify the purpose field and wallet address book as a whole.

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

Would maybe add a comment like "// In very old wallets, address purpose may not recorded, so derive it from IsMine value"

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

Note for reviewers: in a few places in this PR, struct order or argument order is changed so the `AddressPurpose` field follows IsMine or IsChange fields. This is done because the fields hold overlapping information, and in most cases new code should rely on the other fields instead of `AddressPurpose`.

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

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

💬 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)};
```