init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #25989

mzumsande · 2022-09-02T20:47:20Z

If the networks i2p / cjdns are chosen via -onlynet but the user forgot to provide -i2psam / -cjdnsreachable, no outbound connections will be made - it would be nice to inform the user about that.
The solution proposed here mimics existing behavior for -onlynet=onion and non-specified -onion/-proxy where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so.

The second commit adds CJDNS support to the debug-only addpeeraddress RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if -cjdnsreachable=1)

This is the result of an IRC discussion with vasild.

brunoerg · 2022-09-02T20:51:42Z

Concept ACK

ghost · 2022-09-03T07:55:47Z

If the networks i2p / cjdns are chosen via -onlynet but the user forgot to provide -i2psam / -cjdnsreachable, no outbound connections will be made - it would be nice to inform the user about that.

It's mentioned in the i2p doc that i2psam is required. Although I am not sure if everyone reads it.

The second commit adds CJDNS support to the debug-only addpeeraddress RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if -cjdnsreachable=1)

Is this commit related to first change in any way?

DrahtBot · 2022-09-03T14:59:32Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

vasild

ACK b3576b7

vasild · 2022-09-05T12:15:01Z

test/functional/feature_proxy.py

@@ -332,6 +332,16 @@ def networks_dict(d):
        msg = "Error: Invalid -i2psam address or hostname: 'def:xyz'"
        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)

+        self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")


nit: "invalid" looks unnecessary here and below:

Suggested change

self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")

self.log.info("Test passing -onlynet=i2p without -i2psam raises expected init error")

brunoerg · 2022-09-05T14:17:39Z

The solution proposed here mimics existing behavior for -onlynet=onion and non-specified -onion/-proxy where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so.

This approach is ok for me. But I slightly prefer a warning and it could set up -i2psam / -cjdnsreachable for me (like a "SoftSetBoolArg") if I chose one of them in -onlynet. Are there any malefits in doing so?

mzumsande · 2022-09-06T17:59:54Z

Is this commit related to first change in any way?

Not directly, it's a small and imo uncontroversial improvement to addpeeraddrress, which also deals with cjdns (and which might help to manually add cjdns peers to addrman if we can't find any, even if it's currently debug-only), so I thought might make sense to include here. If preferred, I could drop it.

This approach is ok for me. But I slightly prefer a warning

I chose an error because of the existing behavior with -onlynet=onion and missing -proxy, and I don't really see a reason why a different approach for different privacy networks would make sense. That being said, I'd be fine with a warning too, but in that case we probably should also just warn in the -onlynet=onion case.
I think it would be best to wait for the discussion in #24991 (which is more important than this PR and hopefully gets into 24.0, because it fixes a bug) and adapt this to PR to imitate whatever approach was chosen there.

(...) and it could set up -i2psam / -cjdnsreachable for me (like a "SoftSetBoolArg") if I chose one of them in -onlynet. Are there any malefits in doing so?

I think that soft-setting -i2psam would entail guessing the i2p proxy (default "127.0.0.1:7656" but configurable), which seems not ideal to me.

brunoerg · 2022-09-06T18:19:29Z

I think that soft-setting -i2psam would entail guessing the i2p proxy (default "127.0.0.1:7656" but configurable), which seems not ideal to me.

You're right. Thanks.

luke-jr

If you set -onlynet=cjdns -onlynet=ipv6, it shouldn't fail IMO. Maybe a warning.

vasild · 2022-09-12T08:28:24Z

@luke-jr,

Requesting -onlynet to an unreachable network is a sign the user does not know what they are doing / misconfigured bitcoind / the provided configuration will not have the expected effects. In such a case I think it is best to stop at startup.

In particular, that means:

for -onlynet=onion to work, a tor proxy must be provided either via -proxy, -onion or -torcontrol+-listenonion=1
for -onlynet=i2p to work, I2P proxy must be provided via -i2psam
for -onlynet=cjdns to work, the CJDNS network must be reachable in the environment where bitcoind is running and signaled to bitcoind by -cjdnsreachable

regardless of whether additional -onlynet options are used.

fanquake · 2022-09-13T11:37:16Z

I think it would be best to wait for the discussion in #24991 (which is more important than this PR and hopefully gets into 24.0, because it fixes a bug) and adapt this to PR to imitate whatever approach was chosen there.

#24991 has now been merged, and will be part of 24.0.

...because -i2psam or -cjdnsreachable are not provided. This mimics existing behavior for -onlynet=onion and non-specified proxy.

This allows us to add cjdns addresses to addrman for testing and debug purposes (if -cjdnsreachable is true)

mzumsande · 2022-09-19T15:25:42Z

b3576b7 to 68209a7: Rebased

I kept it at InitErrors, not warnings, see @vasild's rationale, plus I can't see a good reason why we should just have warnings for certain networks, but errors for others in a comparable situation.

mzumsande · 2022-09-19T15:34:53Z

@fanquake: I don't see a strong need to push this last-minute in 24.0, at least it shouldn't delay anything. In contrast to #24991 which fixed a bug (users with a reasonable configuration would get an InitError at startup) this is just a nice-to-have (users who don't read the docs and choose an unreasonable configuration will get an InitError now instead of getting no peers from that network).

fanquake · 2022-09-19T15:36:29Z

I don't see a strong need to push this last-minute in 24.0, at least it shouldn't delay anything.

@mzumsande I've added the milestone as a nice-to-fix for 24.x, and this should be simple to backport. This wont be blocking branch-off.

vasild

ACK 68209a7

dergoegge

ACK 68209a7

fun fact: passing -onlynet=i2p takes a lot longer to error than passing -onlynet=cjdns because we load the chainstate (and other chain related data structures) before the i2p checks but after the cjdns checks. Not sure if that is worth a refactor (in a follow up) but in my opinion there is really no reason to have the chainstate loaded before doing all networking checks.

...because -i2psam or -cjdnsreachable are not provided. This mimics existing behavior for -onlynet=onion and non-specified proxy. GitHub-Pull: bitcoin#25989 Rebased-From: a8a9ed6

This allows us to add cjdns addresses to addrman for testing and debug purposes (if -cjdnsreachable is true) GitHub-Pull: bitcoin#25989 Rebased-From: 68209a7

fanquake · 2022-09-21T10:07:50Z

~~Added to #26133 for backporting to 24.x.~~

We'll do it in #26145.

… but are unreachable 68209a7 rpc: make addpeeraddress work with cjdns addresses (Martin Zumsande) a8a9ed6 init: Abort if i2p/cjdns are chosen via -onlynet but unreachable (Martin Zumsande) Pull request description: Identical commit from #25989 ACKs for top commit: fanquake: ACK 68209a7 Tree-SHA512: eec335df06b4c209cfe3473cb623828effd00c45a5dd605bb920edd265de1c789627482b005a51e89b8fc79cc4c5d26ff1fc306f2e4573897c5c7f083aa22861

… but are unreachable 68209a7 rpc: make addpeeraddress work with cjdns addresses (Martin Zumsande) a8a9ed6 init: Abort if i2p/cjdns are chosen via -onlynet but unreachable (Martin Zumsande) Pull request description: If the networks i2p / cjdns are chosen via `-onlynet` but the user forgot to provide `-i2psam` / `-cjdnsreachable`, no outbound connections will be made - it would be nice to inform the user about that. The solution proposed here mimics existing behavior for `-onlynet=onion` and non-specified `-onion`/`-proxy` where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so. The second commit adds CJDNS support to the debug-only `addpeeraddress` RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if `-cjdnsreachable=1`) This is the result of an [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-09-01#848066;) with vasild. ACKs for top commit: vasild: ACK 68209a7 dergoegge: ACK 68209a7 Tree-SHA512: 6db9787f01820190f14f90a0b39e4206603421eb7521f792879094d8bbf4d4d0bfd70665eadcc40994ac7941a15ab5a8d65c4779fba5634c0e6fa66eb0972b8d

mzumsande changed the title ~~init: abort if i2p/cjdns are chosen via -onlynet but unreachable~~ init: abort if i2p/cjdns are chosen via -onlynet but are unreachable Sep 2, 2022

DrahtBot mentioned this pull request Sep 4, 2022

init: allow startup with -onlynet=onion -listenonion=1 #24991

Merged

vasild approved these changes Sep 5, 2022

View reviewed changes

luke-jr suggested changes Sep 10, 2022

View reviewed changes

DrahtBot added the Needs rebase label Sep 13, 2022

mzumsande added 2 commits September 19, 2022 11:06

init: Abort if i2p/cjdns are chosen via -onlynet but unreachable

a8a9ed6

...because -i2psam or -cjdnsreachable are not provided. This mimics existing behavior for -onlynet=onion and non-specified proxy.

rpc: make addpeeraddress work with cjdns addresses

68209a7

This allows us to add cjdns addresses to addrman for testing and debug purposes (if -cjdnsreachable is true)

mzumsande force-pushed the 202208_onlynet_init branch from b3576b7 to 68209a7 Compare September 19, 2022 15:21

fanquake added P2P and removed Needs rebase labels Sep 19, 2022

fanquake added this to the 24.0 milestone Sep 19, 2022

bitcoin deleted a comment from DrahtBot Sep 19, 2022

vasild approved these changes Sep 19, 2022

View reviewed changes

fanquake requested review from naumenkogs and dergoegge September 20, 2022 10:04

dergoegge approved these changes Sep 20, 2022

View reviewed changes

fanquake merged commit 97f865b into bitcoin:master Sep 21, 2022

fanquake mentioned this pull request Sep 21, 2022

[24.x] Backports for rc2 #26133

Merged

maflcko mentioned this pull request Sep 21, 2022

[24.x] init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #26145

Merged

mzumsande deleted the 202208_onlynet_init branch September 21, 2022 18:24

bitcoin locked and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #25989

init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #25989

mzumsande commented Sep 2, 2022

brunoerg commented Sep 2, 2022

ghost commented Sep 3, 2022 •

edited by ghost

Loading

DrahtBot commented Sep 3, 2022 •

edited

Loading

vasild left a comment

vasild Sep 5, 2022

brunoerg commented Sep 5, 2022

mzumsande commented Sep 6, 2022 •

edited

Loading

brunoerg commented Sep 6, 2022

luke-jr left a comment

vasild commented Sep 12, 2022

fanquake commented Sep 13, 2022

mzumsande commented Sep 19, 2022

mzumsande commented Sep 19, 2022

fanquake commented Sep 19, 2022

vasild left a comment

dergoegge left a comment

fanquake commented Sep 21, 2022 •

edited

Loading

	self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")
	self.log.info("Test passing -onlynet=i2p without -i2psam raises expected init error")

init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #25989

init: abort if i2p/cjdns are chosen via -onlynet but are unreachable #25989

Conversation

mzumsande commented Sep 2, 2022

brunoerg commented Sep 2, 2022

ghost commented Sep 3, 2022 • edited by ghost Loading

DrahtBot commented Sep 3, 2022 • edited Loading

Conflicts

vasild left a comment

Choose a reason for hiding this comment

vasild Sep 5, 2022

Choose a reason for hiding this comment

brunoerg commented Sep 5, 2022

mzumsande commented Sep 6, 2022 • edited Loading

brunoerg commented Sep 6, 2022

luke-jr left a comment

Choose a reason for hiding this comment

vasild commented Sep 12, 2022

fanquake commented Sep 13, 2022

mzumsande commented Sep 19, 2022

mzumsande commented Sep 19, 2022

fanquake commented Sep 19, 2022

vasild left a comment

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

fanquake commented Sep 21, 2022 • edited Loading

ghost commented Sep 3, 2022 •

edited by ghost

Loading

DrahtBot commented Sep 3, 2022 •

edited

Loading

mzumsande commented Sep 6, 2022 •

edited

Loading

fanquake commented Sep 21, 2022 •

edited

Loading