💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275411963)
Also I think it's a bit unclear where the "additional flags" go, are they part of `[permissions@]` ?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275409948)
Seems weird to use a string placeholder here for `"incoming only"` when there isn't a variable

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275440729)
I'm not sure this is sufficient functional test coverage. With the exception of the new permissions "in" and "out" which are invalid on master, all tests otherwise pass with this commit (1e09c265a9598df3cc39336e3bcccb993dacf3d8) applied.
I was expecting to see a test that makes an outbound connection to a node that misbehaves and doesn't get banned.

Or was the motivation more about the tx "trickle"? In that case I can see it being hard to test without waiting for time to pass?

👍 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