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

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202208_onlynet_init changing 4 files +23 −1
  1. mzumsande commented at 8:47 pm on September 2, 2022: contributor

    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.

  2. mzumsande renamed this:
    init: abort if i2p/cjdns are chosen via -onlynet but unreachable
    init: abort if i2p/cjdns are chosen via -onlynet but are unreachable
    on Sep 2, 2022
  3. brunoerg commented at 8:51 pm on September 2, 2022: contributor
    Concept ACK
  4. ghost commented at 7:55 am on September 3, 2022: none

    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?

  5. DrahtBot commented at 2:59 pm on September 3, 2022: contributor

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

    Conflicts

    No conflicts as of last run.

  6. in test/functional/feature_proxy.py:335 in b3576b7e59 outdated 
    331@@ -332,6 +332,16 @@ def networks_dict(d):
332         msg = "Error: Invalid -i2psam address or hostname: 'def:xyz'"
333         self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
334 
335+        self.log.info("Test passing invalid -onlynet=i2p without -i2psam raises expected init error")
    vasild commented at 12:15 pm on September 5, 2022:

    nit: “invalid” looks unnecessary here and below:

    0        self.log.info("Test passing -onlynet=i2p without -i2psam raises expected init error")
  7. vasild approved
  8. vasild commented at 12:16 pm on September 5, 2022: contributor
    ACK b3576b7e594dc75a8100e231089f3f5985207bb7
  9. brunoerg commented at 2:17 pm on September 5, 2022: contributor

    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?

  10. mzumsande commented at 5:59 pm on September 6, 2022: contributor

    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.

  11. brunoerg commented at 6:19 pm on September 6, 2022: contributor

    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.

  12. luke-jr changes_requested
  13. luke-jr commented at 11:04 pm on September 10, 2022: member
    If you set -onlynet=cjdns -onlynet=ipv6, it shouldn’t fail IMO. Maybe a warning.
  14. vasild commented at 8:28 am on September 12, 2022: contributor

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

  15. fanquake commented at 11:37 am on September 13, 2022: member

    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.

  16. DrahtBot added the label Needs rebase on Sep 13, 2022
  17. init: Abort if i2p/cjdns are chosen via -onlynet but unreachable 
    ...because -i2psam or -cjdnsreachable are not provided.
This mimics existing behavior for -onlynet=onion and non-specified proxy.
    a8a9ed67cc
  18. rpc: make addpeeraddress work with cjdns addresses 
    This allows us to add cjdns addresses to addrman for
testing and debug purposes (if -cjdnsreachable is true)
    68209a7b5c
  19. mzumsande force-pushed on Sep 19, 2022
  20. mzumsande commented at 3:25 pm on September 19, 2022: contributor

    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.

  21. fanquake removed the label Needs rebase on Sep 19, 2022
  22. fanquake added the label P2P on Sep 19, 2022
  23. fanquake added this to the milestone 24.0 on Sep 19, 2022
  24. mzumsande commented at 3:34 pm on September 19, 2022: contributor
    @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).
  25. fanquake commented at 3:36 pm on September 19, 2022: member

    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.

  26. bitcoin deleted a comment on Sep 19, 2022
  27. vasild approved
  28. vasild commented at 4:07 pm on September 19, 2022: contributor
    ACK 68209a7b5c0326e14508d9cf749771605bd6ffe7
  29. fanquake requested review from naumenkogs on Sep 20, 2022
  30. fanquake requested review from dergoegge on Sep 20, 2022
  31. dergoegge approved
  32. dergoegge commented at 4:44 pm on September 20, 2022: member

    ACK 68209a7b5c0326e14508d9cf749771605bd6ffe7

    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.

  33. fanquake merged this on Sep 21, 2022
  34. fanquake closed this on Sep 21, 2022
  35. fanquake referenced this in commit 45ef64d1a6 on Sep 21, 2022
  36. fanquake referenced this in commit ba8cdb2df2 on Sep 21, 2022
  37. fanquake commented at 10:07 am on September 21, 2022: member

    Added to #26133 for backporting to 24.x.

    We’ll do it in #26145.

  38. fanquake referenced this in commit 05f7937810 on Sep 21, 2022
  39. mzumsande deleted the branch on Sep 21, 2022
  40. sidhujag referenced this in commit ae23f43f61 on Sep 23, 2022
  41. bitcoin locked this on Sep 21, 2023


mzumsande brunoerg ghost DrahtBot vasild luke-jr fanquake dergoegge


naumenkogs

Labels
P2P

Milestone
24.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 04:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me