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

💬 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_r1476755003)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 Can now use C++20 `std::set#contains` method.

```suggestion
if (commands.contains(command)) {
```

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476795724)
If we get the same txid but different wtxid, I'm not sure that the result is necessarily one that makes sense. Most things in the wallet are keyed and listed by txid, so with this change, we'd end up with a transaction that's listed as conflicted by it's own txid. Also, we won't ever update the wallet to store a transaction with a different wtxid.

💬 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_r1476798503)
Using an unordered set instead of a set, `find`/`contains` complexity can be reduced from O(log n) to O(1). Remember to add the `#include <unordered_set>` header in this case.

⚠️ nsvrn opened an issue: "IBD stalling issue in v26.0 and assumevalid=0"
(https://github.com/bitcoin/bitcoin/issues/29374)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I'm using `v26` with `assumevalid=0` and the IBD keeps stalling after the log message says `Potential stale tip detected, will try using extra outbound peer`, it retries but same message keeps repeating unless I restart the software.
I saw someone on bitcoin stack exchange suggested to not use VPN, I'm not using any VPN or tor. Just simple clearnet on a fresh Linux OS.

### Expected behav
...

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476798246)
I think this hasn't been reverted yet?

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476797365)
In 5fbf18b89d6768bed1be95dcbc98797df5788dfc "wallet: track mempool conflicts"

I'm a little bit confused by the "erase this wtx entry" part of the comment. AFAICT, no entries are being erased, Perhaps this should say:

```suggestion
// If this wtx has no other mempool conflicts, mark it as inactive
```

✅ nsvrn closed an issue: "IBD stalling issue in v26.0 and assumevalid=0"
(https://github.com/bitcoin/bitcoin/issues/29374)