👍 Ayush170-Future approved a pull request: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153#pullrequestreview-1548543088)
ACK

I support the idea of having separate files for Legacy and Descriptor because I think the Wallet implementation would differ. Overall, the code looks great to me.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275488694)
Added a comment to explain.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275488776)
Fixed.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275488845)
Fixed.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275488966)
Done.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275490262)
Added a comment to explain, and restructured a bit.

The BIP's test vectors specify either `out_ciphertext` (for short messages) or `out_ciphertext_endswith` (for long messages), the other one is empty. We should only compare the one that was provided.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275490861)
Kind of. Permitting this to grow too large makes the test slower. The plaintext also has this restriction.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275490952)
Done.

💬 kevkevinpal commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1652522093)
would it make sense to add a functional test like the below under
```
ip_port = "localhost:[]".format(p2p_port='add')
self.nodes[0].addnode(node=ip_port, command='add')
```
in `test/functional/rpc_net.py` on line 212
because we shortly after assert that there is only 1 added node added

💬 jonatack commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275510880)
s/now return more specific/now raise the more specific/

💬 brunoerg commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652628071)
> 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.

💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275563768)
It's a JSON RPC response, returning seems correct to me.

💬 jonatack commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275573058)
if you are set on "return", then

`s/return more specific RPC_INVALID_PARAMETER/return the more specific RPC_INVALID_PARAMETER error/`

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