💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476632077)
in "refactor: SetAddressBookWithDB, minimize number of map lookups" (https://github.com/bitcoin/bitcoin/pull/26836/commits/10f9639fd6332cdd440f540c2202082044aab98c):

nit: unnecessary formatting change

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476711835)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):

This is a pretty thorny block of code with all of the booleans, let me summarize just to make sure I'm following:

* If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
* If it requires transfer and does belong to the current wallet in the loop
...

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476626408)
in "refactor: SetAddrBookWithDB, signal only if write succeeded" (https://github.com/bitcoin/bitcoin/pull/26836/commits/0b83c8691b277d407e61a8ffca807e65866f4ded):

Maybe do `encoded_dest = EncodeDestination(address)` and then use that to avoid calling `EncodeDestination` twice?

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476718674)
in "wallet: migration, batch addressbook records removal" (https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39):

nit: most other places in the codebase we use `DB` in functions names (as opposed to `Db`)

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476616953)
in "wallet: clean redundancies in DelAddressBook" (https://github.com/bitcoin/bitcoin/pull/26836/commits/da3b8f927c2da5b7761e36fe548eb8ade29171d7):

nit: your commit message mentions "Call IsMine only once (instead of two)" but I don't see any IsMine related changes in the commit?

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1476724914)
in "refactor: wallet, simplify addressbook migration" (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):

nit: C++20 has `.contains(arg)` for sets (yay!). No performance difference, but semantically I think it makes a lot more sense to use when testing for existence.

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924710289)
ACK https://github.com/bitcoin/bitcoin/commit/46b3c569cfd01a45e5d9cf9428a07b06652e3df9

Looks good, thanks for taking the suggestions! Having the callbacks seems like a really nice feature: based on one of your review comments, I took a stab at using the callbacks with fast wallet rescans and using them simplifies that code quite a bit.

💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1924738424)
> Code looks good. I have been testing this with the asmap.dat file from https://github.com/fjahr/asmap-data/pull/6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?

Just checked it, @fjahr. I think `assert_debug_log` is slowing it down. Can you please check with the following diff?
```diff
diff --git a/contrib/asmap/test_bucketing.py b/contr
...

💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476750977)
In commit "wallet: Reload the wallet if migration exited early" (78ba0e6748d2a519a96c41dea851e7c43b82f251)

Not related to this PR, but I think the `fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()` expression repeated throughout this function is unnecessarily verbose and also potentially dangerous if it is used in another context, or if the surrounding context changes.

It happens to be safe currently, because we know this code is only called after MigrateToSQLite is call
...

👍 ryanofsky approved a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1860423051)
Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.

I left a review comment about improving derivation of wallet paths in the MigrateLegacyToDescriptor function, but that isn't really about this PR and would be better addressed in a separate followup.

💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1924743815)
If @furszy wants to re-ack, or if someone else adds a review, I think this could be merged

💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#discussion_r1476776290)
It's noted.

🤔 jonatack reviewed a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1860423961)
Reviewed the first commit fa48d460334beef43fa73455aa9e30971d33c1b2. It looks like the prefix in its commit name should be `cli` instead of `gui`. Will review the second commit later today or tomorrow.

<details><summary>combined suggested diff for <code>fa48d46</code></summary><p>

```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index a53c2a1b0d0..b342382fc96 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -53,6 +53,7 @@ using CliClock = std::chrono::system_
...

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476756492)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 `continue` here would be more idiomatic C++ and avoid indenting the next 30 lines.

```suggestion
if (!IsSwitchChar(argv[i][0])) continue;
```

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476751537)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and the duplicate element would be the bitcoin-cli command.

```suggestion
throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
```

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476753642)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.

```suggestion
throw std::runtime_error(strprintf("invalid bitcoin-cli argument; if \"%s\" is a valid option, try passing it before the \"%s\" command", argv[i], exclusively_command));
```

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476759727)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 These two lines could be reduced to the following to keep `pos` scoped to the conditional only (and use `string_view::npos`).

```diff
- std::size_t pos = command.find('=');
- if (pos != std::string::npos) {
+ if (const auto pos{command.find_first_of('=')}; pos != std::string_view::npos) {
command = command.substr(0, pos);
}
```

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476781372)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 This seems like a lot of code for what could be 2 lines instead, and you could make the commands data structure an `constexpr` array constant and have it run in O(n) instead of O(log n) (and order the commands by probable frequency of invocation). The resulting code is self-explanatory, so could drop the comments too.

<details><summary>Suggested diff</summary><p>

```diff
@@ -53,6 +53,7 @@ using CliClock = s
...

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476783931)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 could do the cheaper check first

```suggestion
if (!exclusively_command.empty() && !gArgs.IsArgSet(std::string(command))) {
```

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476752815)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.

```suggestion
throw std::runtime_error(strprintf("you can only run one bitcoin-cli command at a time, either %s or %s", exclusively_command, command));
```