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

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

Choice of 2**14 arbitrary?

💬 achow101 commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1275442608)
In 56882622faf469b6f948f79a69c3c8ddbde92ff8 "crypto: update CKey, CPubkey for silent payments"

Same data copying concerns as `AddTweak`.

💬 achow101 commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1275442463)
In 56882622faf469b6f948f79a69c3c8ddbde92ff8 "crypto: update CKey, CPubkey for silent payments"

We shouldn't be copying secret data out of a secure structure into an insecure one. When `tweaked_seckey` is destroyed, the underlying data may still linger in memory and end up being read by something else.

If this function must return a new `CKey`, then it should instantiate it here and perform the operation on it's `data()` rather than doing this copy.

However, I would prefer if this functi
...