💬 pinheadmz commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1275138846)
You don't need to clear here, or on L125 because you are opening the conf file with "w" so the contents of the file get overwritten. Tests pass without the two clear lines, and I checked with extra open/read/print statements that the expected configuration settings were present in the file at each step.

💬 0xB10C commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1652061642)
ACK, looks like the tests [are run](https://cirrus-ci.com/task/6074291154845696?logs=ci#L3392).
```
interface_usdt_coinselection.py | ✓ Passed | 17 s
interface_usdt_mempool.py | ✓ Passed | 37 s
interface_usdt_net.py | ✓ Passed | 14 s
interface_usdt_utxocache.py | ✓ Passed | 35 s
interface_usdt_validation.py | ✓ Passed | 8 s
```

💬 vasild 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-1652088394)
Reported at https://i2pgit.org/i2p-hackers/i2p.i2p/-/issues/399

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1652089161)
I guess allowing more connections beyond the max is a DoS vector, which could potentially crash a node and break all existing connections anyway... hm/

💬 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-1652101826)
> [geti2p.net/en/download](https://geti2p.net/en/download) has just updated their available macOS version from 1.9 to 2.3 -- will retest.

FWIW, I didn't manage to make the v2.3 jar work and had to reinstall v1.9 (available as a dmg) at https://geti2p.net/en/download/mac. Per that page: *"Important Note: The 2.1.0 Mac OSX Easy Install Bundle release is delayed. Please install the 1.9.0 release below. You will be notified in the router console when the 2.1.0 update is available. Thank you for y
...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1275188943)
> I think this should either be reverted, or the usdt requirement for root be dropped.

How would you feel about adding an environment variable to "force" root in `00_setup_env_native_asan.sh`? The functional test I added no longer skips if root (just expects a different behavior), so this actually would be kinda nice to have one task *does* run all functional tests as root.

💬 fanquake 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-1652117028)
> FWIW, I didn't manage to make the v2.3 jar work and had to reinstall v1.9 (available as a dmg) at

I don't think that's something we'd want to advise anyone todo, given https://geti2p.net/en/blog/post/2023/01/31/mac-easy-install-notarization:
> 1.9.0 has known security issues and is not suitable for hosting services or any long-term use. Users are advised to migrate away as soon as possible. Advanced users of the Easy-Install bundle may work around this by compiling the bundle from source
...

💬 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-1652135083)
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 some unusual issues with flood attacks in June, but otherwise has been reliable for me over the past couple of years.

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275213995)
I think the contexts are different. `notifications` is working with descriptor and here is legacy, I believe I'll have something better when I finish the `DescriptorScriptPubKey` target. Also, I believe the `FuzzedWallet` in `notifications` is disabled for spkm.

👋 brunoerg's pull request is ready for review: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153)

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#issuecomment-1652137723)
CI failure seems unrelated.

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#issuecomment-1652142508)
Pushed to re-run CI.

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