💬 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.'
```

💬 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_r1127237286)
```suggestion
MULTIPLE_CLI_COMMAND_ERROR = "Error: you can only run one cli-command at a time, either {} or {}"
```

💬 jonatack 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_r1127275368)
> Where does `bitcoin-cli -h` show how to solve this problem specifically?

`./src/bitcoin-cli -h | grep -A4 rpcwallet`

💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1127278080)
That looks dangerous -- generally I think we shouldn't ever call `.value()` on a std::optional object without checking for `.has_value()` first (similar as for and pointer dereferences), potentially risking a crash. In this case the GUI flow seems to ensure that the IP passed here is valid at an earlier stage (via the `ProxyAddressValidator`) so `LookupHost` _should_ always return a value, but I'd prefer to be rather safe than sorry.

💬 jonatack 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_r1127282385)
The alternative is provided in the preceding sentence:

`Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<filename> URI path. Or for the CLI, specify the "-rpcwallet=<filename>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).`

A good point is that the functional test would make it more clear if the full message is provided.

```diff
index 0f2e076fe8b.
...

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127282542)
hm, this suggestion makes sense within the context of the first commit. however, the `search_tried` logic gets updated over the commits so it would be expanded anyways

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127291828)
nice, done

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127291922)
done

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1457435139)
thanks for the review @brunoerg ! responded to all your feedback

💬 theStack commented on pull request "test: add coverage for sigop limit policy (`-bytespersigop` setting)":
(https://github.com/bitcoin/bitcoin/pull/27171#discussion_r1127302187)
The intention was to express the implication of exceeding the sig-op limit from a user's perspective (tx is treated as larger than it's serialized vsize => more fees needed to reach the same fee-rate; or as PR desc of #8365 put it "high-sigop transactions [...] need to pay a fee corresponding to the maximally-used resource."). Agree that the comment is rather misleading and not very helpful for the test, replaced it.

Generally struggled a bit with terminology for this PR (not even sure if tal
...