💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275260090)
> Also, the option shouldn't be set for tests that previously didn't have it set

It's false by default and it has been set up for tests that were already using it.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1275287508)
I added some extra logs to bitcoind and ran the test, these addrs fail `IsRoutable()`. I assume these are CJDNS addresses?

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1275303665)
Does this look good?
I used chatgpt to refine the sentence a bit more to this

"Specify the Tor control host and optional port to use when onion listening is enabled (default: %s). If no port is specified, the default port will be used (default: %s)."

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275142287)
commit d0ac2d6602ab5fd32d8d22b49488f77b33d6f0a9:
In the commit message, "(counting IPv4 and IPv6 as one network)" is no longer true and should be removed.

🤔 mzumsande reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1548029271)
ACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe (with the caveat that I'm coauthor)

I reviewed the commits and tested this on mainnet while supporting multiple networks - everything worked as expected, it would usually be all clearnet connections first, some of which are then slowly replaced by i2p / cjdns / tor so that diversity increases.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1275342509)
gah, thanks. will update if I retouch.

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1275362802)
Maybe check that `cookie_perms_arg` isn't a `1`/parameterless.

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1275365847)
Should probably set permissions before writing to the file (but after opening it, so we already have write access), just in case they are more restrictive?

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1275367672)
Rebase, `<util/fs.h>`

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1275369582)
```c++
LogPrintf("Permissions used for cookie%s: %s\n",
cookie_perms ? " (set by -rpccookieperms)" : "",
perms_to_str(fs::status(filepath).permissions()));
```

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1275371316)
looks good,
runs usdt test: https://cirrus-ci.com/task/4683666694078464?logs=ci#L4600-L4604

📝 brunoerg opened a pull request: "test: remove unused code in `wallet_fundrawtransaction`"
(https://github.com/bitcoin/bitcoin/pull/28164)
This PR removes in `wallet_fundrawtransaction`:
- unecessary variables/calls to `decoderawtransaction`
- unused `totalOut` variable and its related code (`totalOut` is used in some functions to test change, in other ones its value is not used)

💬 jonatack commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1275391537)
Yes, these three are CJDNS.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1275392099)
does the test node need a config flag to route?

💬 kevkevinpal commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652416657)
utACK [108c625](https://github.com/bitcoin/bitcoin/pull/28164/commits/108c6255bc670bbf2732f2b79f6c32c26e181208)

I'm fine with just removing the unused variables but instead of removing the `totalOut` and `dec_tx` would it make sense to add an assertion on what that value is? Not exactly sure why the unused vars were added initially

📝 sipa opened a pull request: "net: transport abstraction"
(https://github.com/bitcoin/bitcoin/pull/28165)
This PR furthers the P2P message serialization/deserialization abstraction introduced in #16202 and #16562, in preparation for introducing the BIP324 v2 transport (making this part of #27634). However, nothing in this PR is BIP324-specific, and it contains a number of independently useful improvement.

The overall idea is to have a single object in every `CNode` (called `m_transport`) that is responsible for converting sent messages to wire bytes, and for converting received wire bytes back to
...

💬 achow101 commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1652419820)
ACK a733dd79e29068ad1e0532ac42a45188a040a7b9

Changes since last are mainly responding to review comments.

👍 jamesob approved a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1548082611)
ACK 180909f2c859be3c79630d9705c26a457715b9ed ([`jamesob/ackr/28008.3.sipa.bip324_ciphersuite`](https://github.com/jamesob/bitcoin/tree/ackr/28008.3.sipa.bip324_ciphersuite))

Looks great. I've spent about a week ramping up on BIP-324, reading the BIPs and underlying RFC 8439, asking @sipa stupid questions about the garbage terminator, and carefully reviewing the code in this pull request. I've verified the ChaCha20-Poly1305 test vectors against an unrelated implementation (Python's chacha20pol
...

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275173737)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Only if you retouch, might be nice to declare the values that don't change here as `const auto`.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275238979)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Slightly confused by why this conditional is necessary - can you explain in what case our expected ciphertext would be empty, but the actually generated ciphertext wouldn't be? I've tried removing this conditional but I get test failures.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275295605)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Small thing, but if you retouch maybe:

```diff
diff --git a/src/test/fuzz/bip324.cpp b/src/test/fuzz/bip324.cpp
index 5df279b7df..376728d61d 100644
--- a/src/test/fuzz/bip324.cpp
+++ b/src/test/fuzz/bip324.cpp
@@ -109,8 +109,10 @@ FUZZ_TARGET(bip324_cipher_roundtrip, .init=Initialize)
}
}

+ auto& receiver{from_init ? responder : initiator};
+
// Decrypt length
- uint32_t dec_length = (from_in
...