💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275576921)
I'm not set on it, it's just the language the JSON RPC spec [uses](https://www.jsonrpc.org/specification). Otherwise I'm okay with your suggestion.

📝 jonatack opened a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166)
Add test coverage for passing an invalid sighashtype to RPCs signrawtransactionwithwallet, signrawtransactionwithkey, walletprocesspsbt, and descriptorprocesspsbt.

💬 kevkevinpal commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652725607)
> > Not exactly sure why the unused vars were added initially
>
> Not sure if it might worth to add the asserts, it seems to be a leftover from a copy/paste since the functions (in general) are similar.

Ya that makes sense, lgtm

💬 luke-jr commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1275618446)
This will need `aRules` in `rpc/mining.cpp` updated

🤔 luke-jr requested changes to a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1548838256)
`-blocksonly` docs need updating

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275643872)
`node_services` has been discarded here, likely as an oversight trying to rebase it past 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67.

Best approach I see is to export it outward, ensuring callers need to handle the API change.

🤔 ajtowns reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1548859239)
utACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe

Various nits only. I think adding some redundant net-category debug logs about trying to make ipv6 connections even on systems with no ipv6 connectivity is fine given it's at about the same rate as there's log entries for the extra block relay only connections anyway.

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642065)
dev notes says `++i` preferred over `i++`, here and elsewhere

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275642506)
Having a map where a `std::array` would be fine is kind of lame... That said, this is the same setup as in addrman_impl.h for `m_network_counts`.

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275648306)
Could be a `const` member function, if you weren't using `map::operator[]`. That would mean either switching to an array, where you can use `[]` for just a lookup, or doing a manual find:

```c++
if (auto it = m_network_conn_counts.find(net) && it != m_network_conn_counts.end()) {
return it->second > 0;
}
return false;
```

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275653315)
Maybe report the `preferred_net` name here as well?

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275652683)
Isn't `network_peer == true` the same as `preferred_net != std::nullopt` ? Could drop the bool.

🤔 theStack reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1548892755)
Regarding the stream API discussion, I'd also prefer simplicity over efficiency for now and hence vote for keeping the implementation as-is; the potential advantage of "slightly lower latency when receiving a large P2P message" doesn't seem to outweigh the increased complexity / review power needed, even if it would mostly affect test code. @jamesob made some convincing points for that and I'm also sharing the following sentiment:
> For me, V2 is a very important feature to get deployed. I woul
...

💬 luke-jr commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1275671827)
This seems like a very serious bug that should have blocked the merge... :|

💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275674549)
>maybe mentioning that this aligns DisconnectNode and the disconnectnode rpc.

Internal APIs really don't seem like they belong here

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1652836631)
Updated to remove some unnecessary constructor changes and misleading comments associated with them.

💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677257)
Why not drop this entirely? Seems like a bug specifying an IP won't disconnect all nodes connecting from that IP...

💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677709)
In fact, maybe this should be split into a bugfix commit before adding subnet support.

💬 luke-jr commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1652845994)
>Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511.

If it's going to be used for this, it shouldn't be hidden/test-only...

💬 luke-jr 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_r1275685570)
Is there a reason we shouldn't just support this? Normally it is okay to pass switch options in any order.