💬 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.
(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
...
(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.
(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.
(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)
(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.
(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.
(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
...
(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,
...
(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.
(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?
(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)."
(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.
(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.
(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.
(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.
(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?
(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>`
(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()));
```
(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
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1275371316)
looks good,
runs usdt test: https://cirrus-ci.com/task/4683666694078464?logs=ci#L4600-L4604