💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1652150762)
Yes. I find the lighter-weight i2pd router in C++ to be an easier option for node operators to install: `apt install i2pd` / `brew install i2pd`, etc. That router did experience issues with a [floodfills](https://github.com/PurpleI2P/i2pd/discussions/1918) attack in late April to mid-May, but otherwise has been reliable for me over the past couple of years. It's also the router used by [Raspiblitz](https://github.com/openoms/raspiblitz/commit/61be6fb95ad3a3db2fed700185d29a1a073f5ea2) and [Umbrel
...

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1275250233)
> Right, nodiscard is "use it or lose it" (remove it) -- applicable to getter/pure functions and any interfaces where the return value must be checked. Unlike the latter, if the result of a getter is not used it would not be incorrect, but in that case the call is useless and should be removed; nodiscard just enforces it.

I use nodiscard for these two cases I describe, where it can simplify review and find issues for me. For instance, when adding it to a declaration I was touching in a pull,
...

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