💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127077341)
Okay, added

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127079196)
You’re right, fixed

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127079909)
perhaps rename `newOnly` to `new_only`?

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127083746)
Suggestion:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 8c36af13f..66d54d2b4 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -746,8 +746,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly, std::optiona
search_tried = insecure_rand.randbool();
}

- int bucket_count;
- search_tried ? bucket_count = ADDRMAN_TRIED_BUCKET_COUNT : bucket_count = ADDRMAN_NEW_BUCKET_COUNT;
+ const int bucket_count{search_tried ? ADDRMAN_TRIE
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127091583)
I decided to leave it as is for now.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1457092354)
Changes since https://github.com/bitcoin/bitcoin/commit/32123cfc11ad22cdd7fe5ef26ac0550e1ab2fe7b:
- Addressed remaining review from @LarryRuane
- Fixed crash bug surfaced by @dergoegge
- Addressed review by @furszy

📝 achow101 opened a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Instead of storing and passing around fixed strings for the purpose of an address, use an enum.

💬 achow101 commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1457199020)
ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44

🚀 achow101 merged a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806)

💬 achow101 commented on pull request "http: Track active requests and wait for last to finish - 2nd attempt":
(https://github.com/bitcoin/bitcoin/pull/26742#issuecomment-1457271622)
ACK 60978c8080ec13ff4571c8a89e742517b2aca692

🚀 achow101 merged a pull request: "http: Track active requests and wait for last to finish - 2nd attempt"
(https://github.com/bitcoin/bitcoin/pull/26742)

💬 achow101 commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1457287964)
Missed this on last review:

```
../../../src/validation.cpp: In member function ‘SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(std::function<void(bilingual_str)>)’:
../../../src/validation.cpp:5450:14: error: catching polymorphic type ‘struct StopHashingException’ by value [-Werror=catch-value=]
5450 | } catch (StopHashingException) {
| ^~~~~~~~~~~~~~~~~~~~
```

👍 rserranon approved a pull request: "wallet: finish addressbook encapsulation"
(https://github.com/bitcoin/bitcoin/pull/26836)
tAck
* unit tests
* functional tests
* qt & bitcoin-cli manual tests

🤔 andrewtoth requested changes to a pull request: "cli: add validation to cli side commands besides when it's used with -rpcwallet"
(https://github.com/bitcoin/bitcoin/pull/26990)

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127232884)
I'm not sure if this new message makes it more clear. Specifically, "For the CLI" seems like it might confuse users because there's nothing opposed to that in the message.

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127233443)
Where does `bitcoin-cli -h` show how to solve this problem specifically? Telling users to run that here might not be very helpful. The second part could be useful information though.

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127233611)
nit: `Initialize set`

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127234775)
```suggestion
"Multiple wallets are loaded. Specify the wallet by requesting the RPC through /wallet/<filename> URI path.");
```

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127235671)
Again, I don't think "For the CLI" message is useful if it doesn't have a "as opposed to the ".

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127236411)
```suggestion
INVALID_CLI_COMMAND_ARGUMENT = 'Error: \"{}\" is not a valid cli-command argument. If \"{}\" is a valid option try passing it before the \"{}\" command.'
```