JoinMarket-Org / joinmarket-clientserver

Bitcoin CoinJoin implementation with incentive structure to convince people to take part
GNU General Public License v3.0
729 stars 179 forks source link

Starting without `directory_nodes` in `[MESSAGING:onion]` #1333

Open kristapsk opened 2 years ago

kristapsk commented 2 years ago

Noticed this while testing #1332.

Currently if [MESSAGING:onion] section is present in joinmarket.cfg, but directory_nodes are commented out, when starting ob-watcher.py you get error:

Traceback (most recent call last):
  File "/home/user/git/joinmarket-clientserver/./scripts/obwatch/ob-watcher.py", line 840, in <module>
    main()
  File "/home/user/git/joinmarket-clientserver/./scripts/obwatch/ob-watcher.py", line 825, in main
    mcs.append(OnionMessageChannel(c))
  File "/home/user/git/joinmarket-clientserver/jmdaemon/jmdaemon/onionmc.py", line 656, in __init__
    for dn in [x.strip() for x in configdata["directory_nodes"].split(",")]:
KeyError: 'directory_nodes'

If instead you add empty directory_nodes setting, there is different error:

2022-08-20 22:44:39,035 [ERROR]  Failed to load directory nodes: InvalidLocationStringError('')
2022-08-20 22:44:39,036 [INFO]  Starting ob-watcher
Unhandled error in Deferred:

Traceback (most recent call last):
  File "/home/user/git/joinmarket-clientserver/jmdaemon/jmdaemon/message_channel.py", line 175, in run
    mc.run()
  File "/home/user/git/joinmarket-clientserver/jmdaemon/jmdaemon/onionmc.py", line 738, in run
    self.hs_up_loop.start(0.5)
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.10/site-packages/twisted/internet/task.py", line 206, in start
    self()
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.10/site-packages/twisted/internet/task.py", line 251, in __call__
    d = maybeDeferred(self.f, *self.a, **self.kw)
--- <exception caught here> ---
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.10/site-packages/twisted/internet/defer.py", line 190, in maybeDeferred
    result = f(*args, **kwargs)
  File "/home/user/git/joinmarket-clientserver/jmdaemon/jmdaemon/onionmc.py", line 831, in check_onion_hostname
    if not self.onion_hostname:
builtins.AttributeError: 'OnionMessageChannel' object has no attribute 'onion_hostname'

Kinda no big deal, but it would sometimes save some time to just allow empty or non-existing directory_nodes setting for development purposes, when switching to regtest / testnet, otherwise in some scenarious I need to comment out whole section instead of single line.

Then again, probably having this section without properly configured directory_nodes is not something user might want to have on mainnet. Not 100% sure here.

In any case, error message could be improved, KeyError / AttributeError is not user friendly.

kristapsk commented 2 years ago

would sometimes save some time to just allow empty or non-existing directory_nodes setting for development purposes, when switching to regtest / testnet, otherwise in some scenarious I need to comment out whole section instead of single line.

Actually figured out I can just comment out [MESSAGING:onion] line. :)

AdamISZ commented 2 years ago

Actually figured out I can just comment out [MESSAGING:onion] line. :)

Right. Guess this is not really an issue, whole section should be deleted if you don't want it, right?

kristapsk commented 2 years ago

I want it for mainnet / signet, but not testnet. Anyway, not a big issue, but error messages could be improved.

AdamISZ commented 2 years ago

OK. On different networks, I personally just keep different *cfg files for each (just because there are a few random vars, like this one, that I might want to change). On error message improvement, sure, I guess.

AdamISZ commented 2 years ago

This could be related to another error report from a user on Telegram: they had a huge stack trace output, but actually the error was simply that they forgot to include the # before a comment in the config file. So we could have a config file sanity check before doing anything to make sure we don't have difficult-to-interpret crashes.