-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only onion message channels #1182
Conversation
1a54ead
to
f1bea1d
Compare
Maybe i2p could have been better considering the differences. |
That's an interesting thought for sure. One point in its favour is that we are operating in a kind of isolated way here, we don't need to be interoperable with any other nodes outside of Joinmarket. The biggest problem for me is that I would need a library that allows me to interact with i2p using python twisted; does one exist? And also, I'm not sure I would want to sacrifice the low latency we are able to get currently with Tor connections (I think it's a sensible tradeoff for now, even with all the limitations). To state the obvious, it's not either-or; if we have the code to do it, we should add it as an alternative. Just like in this case, we would be keep the same "internal" Joinmarket line-based messaging so that part will all be interoperable. |
f7a8cd2 is because what remained as changes in install.sh was just an artifact; this PR does not impact the installation script. |
Repeating myself from #1000, just to be sure it does not get lost. I'm strongly in favor of a non-reachable taker mode. In fact, the more I think about it, the more I don't see why it should be any other way. |
Well thanks for repeating, because I honestly didn't remember (sorry!). I think you're correct (and come to think of it, though it was long ago, I think #415 had the same thing in mind, so, just a lapse on my part). At some point shortly I'll try adding a patch so that, first the taker/maker mode of operation is passed into the OnionMessageChannel constructor, then conditional on that var, the handshake message is altered to show the location as some dummy value representing taker, instead of an onion, and the hidden service is not started in this case. It should still work since the dir node defaults to sending back on the inbound anyway (i.e. on the connection opened by its clients), and then when taker-maker comms happen, both will opportunistically try to connect to the other, but the maker will fail to do so as it has no valid endpoint. (It's also a nice detail that it reduces the possibility of Tor configuration annoyances for the taker!) But that should not hold up anyone testing as-is because that patch won't change things in any fundamental way. |
@PulpCattel let's play devil's advocate: is there not an argument that a user should deliberately be obfuscating their role? Certainly the ability to play both sides of the market is desirable. We want users being both intermittently where possible, as it reduces the traceability via fixed behaviour. I feel like the happy medium is something like: there is one mode of operation, which includes "serving", and in that mode you can do both activities. There is another mode of operation, "not serving", where you can only be taker. And then it's just a question of what client side code prefers. Although it's only a subtle difference, it could mean e.g. we add a config variable that can optionally be set (if it's not we default to maker:serving taker:not-serving), so that a taker can choose to serve an onion if they want. Thoughts? |
Uhm, I don't follow. Isn't the taker role by definition distinguishable from the maker role? How can a taker obfuscate its role, doesn't he behave clearly differently from a maker? Also, obfuscating against who?
I certainly agree that a user has to have the option to play both sides, why is this related to the non-reachable taker mode? Can the two categories of scripts (taker vs maker) behave differently? The taker never spawn any onion service, the maker always does, then the user picks his role normally.
I don't see the point, I guess if they want they could but they are gaining nothing(?). Even if they announce an onion, they are the taker, how can they hide that from either the directory node or the makers? Just the fact that they come and ask for a list of onions, that's already a fingerprint. I feel like I'm missing a piece. |
Coming back to this today, I think I was wrong here, but let me unpack my thinking: I'm thinking of how participants in the protocol are seen by other participants. There is an available query to everyone: ask for the list of peers. This is (at least, as currently implemented) different from IRC, even though an IRC client can also get a list of users in-channel. Here there is just one "channel" and requesting peers will give you data that includes reachable location (that's what IRC doesn't do), so that you can talk to those peers off the directory node. That to me was one of the key advantages of this model, because it helps both scalability (definitely) and privacy (at least notionally) to have (most of) the tx negotiation be p2p. So in that sense takers are marked as different from makers if they're not reachable. But still I think I was wrong to raise it since takers voluntarily mark themselves out with |
f7a8cd2
to
662b6dc
Compare
Rebased on master in 662b6dc |
Do you mean specifically the directory node with "other participants"? Everybody else (makers) have no clue what takers are doing, right? That's until a taker reaches to them to do a CoinJoin, and at that point they know is a taker anyway.
Exactly, that makes sense and it is what I was reading from the PR.
I read this, as per above, as in "the directory node can distinguish a taker from a maker". To be clear, as long as the non-reachable one exists, I've no problem with any extra mode deemed worthy (I don't see yet a reason for them to be though). I'd then just argue, and it seems we already concur on this, non-reachable (in this case it could be a config, as you said) should be the default.
BTW, doesn't this turn out to be a good thing? A taker, without an onion, gains "all" the anonimity set of the obwatchers; they do not have an onion either. |
785c8a7
to
2c073ca
Compare
Sure. So I'm working through it now. The code changes are nontrivial, since we now have 3 different types of Peer rather than 2, but it's not anything particularly crazy I think. |
From 2202158 takers should automatically not attempt to serve onions, but will still connect p2p as per the commit comment. I'm currently able to do 3 and 4 party joins on signet with this setup. It's working pretty nicely I think. Oh also I need to issue a new PR for Joinmarket-Docs to rewrite the one I did earlier for lightning-onion. |
So after working on it the last few days, I'm reasonably happy with what we have as of now. Here are some notes on testing:
If you get that far, please test: mixtures of different numbers of bots, more than one directory node, and mixtures of IRC with onion messaging. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review, mostly running the tests and reading the docs.
Test suite passes.
E2E test with both IRC and onion:
- With
1,5
regtest_count -> passes - With
1,15
-> passes - With
1,15
and the taker picks 5 or 9 counterparties instead of default 2 -> hangs in both cases at the end with an "endless" stream of[DEBUG] rpc: generatetoaddress
. I didn't wait a whole lot, so maybe it was just slow
E2E test with only onion:
- With
1,5
regtest_count -> passes - With
1,15
-> passes - With
1,15
and the taker picks 5 or 9 counterparties -> both cases pass and are really fast
Done a small signet CJ, no IRC.
One odd thing, reproducible for me, even with a minimum_makers
config of 1, I wasn't able to do a CoinJoin, not enough liquidity
, with 2 counterparties. By reducing the counterparties to 1, it worked.
EDIT: oh wait, it could be that with 2 counterparties (and I was doing a sweep) the calculated fee was such that the CJ amount was lower than the minimum accepted by the maker (if you see the CJ amount is very low). Didn't test it out but it seems logical.
EDIT2: Nope, tested that, not sure why it's happening.
Oh, silly me, of course it complains, minimum_makers
covers unresponsive makers.
Found some nit.
Thanks for this. I didn't try large numbers, so I'll first try to replicate that and if so, see what it is. Will try to address review 'now' (sort of). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost through onionmc.py
, I can't help much with the twisted stuff but I'll try to do a 2nd pass if I've time after I finished the first read-through of the code.
OK, as of 90bc1e0, a couple more things to mention: Now 2 signet directory nodes, included by default in current config. I want to add two for mainnet on a separate line, with signet commented out (and noted of course, which is which), in the merged-to-master version. Second, I tested a 4+1 party cj with two dns and the new messaging config, and everything went fine. The log of that coinjoin is here. Notice that the messages went through onion/ILITA/darkscience/ILITA for whatever reason. I guess it will be largely random, we'll see. |
Should we (eventually) bias that? Something like, if you can prefer using onion channels? (Maybe another config, jk) |
I expect it's just an organic process. People can start dropping IRC once they get confidence that the onion thing is working. |
Yeah, the idea that the market will do that for us seems logical.
It's sad that with that choice we lose access to all the makers that only use IRC; a taker might be okay with using IRC as long as it's the only option to reach the maker. |
Yeah there's always a long tail with changes like this. I'd switch off IRC, as taker, when I was confident that the large majority were on onion via multiple directories. Anyway it doesn't matter too much if I'm wrong for now. |
Joinmarket bots run their own onion services allowing inbound connections. Both takers and makers connect to other makers at the mentioned onion services, over Tor. Directory nodes run persistent onion services allowing peers to find other (maker) peers to connect to, and also forwarding messages where necessary. This is implemented as an alternative to IRC, i.e. a new implementation of the abstract class MessageChannel, in onionmc.py. Note that using both this *and* IRC servers is supported; Joinmarket supports multiple, redundant different communication methods, simultaneously. Messaging is done with a derived class of twisted's LineReceiver, and there is an additional layer of syntax, similar to but not the same as the IRC syntax for ensuring that messages are passed with the same J5.. nick as is used on IRC. This allows us to keep the message signing logic the same as before. As well as Joinmarket line messages, we use additional control messages to communicate peer lists, and to manage connections. Peers which send messages not conforming to the syntax are dropped. See JoinMarket-Org/JoinMarket-Docs#12 for documentation of the syntax. Connections to directory nodes are robust as for IRC servers, in that we use a ReconnectingClientFactory to keep trying to re-establish broken connections with exponential backoff. Connections to maker peers do not require this feature, as they will often disconnect in normal operation. Multiple directory nodes can and should be configured by bots.
In the previous commit, all peers served an onion. After this commit, taker client instances will automatically send a config var to the jmdaemon backend that instructs the OnionMessageChannel instance to not start an onion service, and the handshake messages sent by these peers replace the onion location with a placeholder string NOT-SERVING-ONION. Directories and maker peers will not therefore to connect outbound to them, but privmsging still happens p2p with connections from takers to makers after the directory has communicated their reachable .onion addresses. This change reduces the configuration requirement for takers and is better for their privacy and security (without sacrificing the gain we get from having p2p connections). The above comments re: takers also apply to ob-watcher bots. This commit also fixes a large number of minor bugs and errors in documentation, as well as many Python cleanups after review from @PulpCattel. A few concrete items are: It fixes the ob-watcher functionality to work with the new subclass of MessageChannel (OnionMessageChannel). It corrects the on_nick_leave trigger to make dynamic nick switching between MessageChannels (as implemented in MessageChannelCollection) work correctly. It corrects the order of events in the add_peer workflow to ensure that a handshake can always be sent so that the activation of the connection always works. It sets a default messaging config with onion, 2 active IRC servers and one inactive IRC server. The onion config has 2 signet directory nodes, so this will change to mainnet after the PR is merged to master.
After 830ac22 I've squashed down to two large commits; the original one is the basic onionmessagechannel setup, the second is the switch to takers not serving onions + plus all the review fixups and general bugfixes. For any who previously reviewed, please note there is only one extra code change in here (just trivial, whitespace-strip the directory nodes in the config). Nothing else has changed. If anyone still wants, please do try the currently configured two directory nodes with or without IRC. I believe it's all working now, I intend to merge this soon (please comment if you disagree!) and also add 1 or more likely 2 mainnet directory nodes after it gets merged. If anyone else wants to run a mainnet directory node please let us know here. |
Here or in a follow-up, the status bar in the QT should be updated. We could make it smart and detect what we are actually using (only onion, only IRC, both), or make it generic and say something like
Getting this warning on 830ac22: [WARNING] Failed to send message to: rr6f6qtleiiwic45bby4zwmiwjrj3jsbmcvutwpqxjziaydjydkk5iad.onion:80, error: AttributeError("'NoneType' object has no attribute 'message'") Is one of the directories down? If that's the case we should consider adding a clearer error message, this is quite cryptic. If I instead set only [INFO] Trying to connect to node: rr6f6qtleiiwic45bby4zwmiwjrj3jsbmcvutwpqxjziaydjydkk5iad.onion:80 |
Yes, that one is down. I'm playing around with tor config on it. Yes, agree on error message. On positive side, that is a useful test :)
Good point.
Yes, we have a 60s give up timeout on IRC servers, I'll investigate that. (To be fair it's not the most critical one but still). |
Agreed, it's not that important, 1. because we don't expect people to run with only 1 directory, and 2. because being stuck on that |
See JoinMarket-Org/custom-scripts#16 This should hopefully make running directory nodes a lot less hassle; the original thing of running a yield generator didn't really make sense, it was just the easiest thing to do at the beginning. I will update the docs here (which are deliberately not hand-holdy for this function, for obvious reasons) to reflect that there's a custom, and customisable script available for doing this (in addition to the tor/systemd stuff you'll want to do). |
After 5d94cb2 I am running two mainnet and two signet d-nodes as long running (hopefully!) services on remote boxes. I'd encourage anyone else to read the new docs on that, and try to run one similarly (or in a better way). I hope the |
Done in 68c64e1 but note I have not removed references to IRC host etc. in the config settings in |
Addressed in cedad9c (the commit comment is pretty detailed so you can read that). I've tested it a bit with the configured d-nodes (shutting them down and starting them up). I believe it's a decent setup/tradeoff and the messages in the console seem pretty clear. |
812afd8
to
755b508
Compare
After 755b508 btc networks must match in handshake; if you try to run a d-node don't forget to set the right network in the config! All my bots are still up, you should still be able to do 3+1 party at least on signet. |
So 3a428fa is basically just trying to make as clean as possible the handling of any handshake failure (especially with directories, and special attention to starting up and being robust to directories not being available or incompatible). |
Also, exports JMMakerClientProtocol for custom directory node scripts (stored in the custom-scripts repo). Modify default config with 2 signet and mainnet directory nodes to start. Handles unreachable directory nodes with a human readable error and adjusts connection timeouts to be realistic. Changes wording in Qt notifications from "IRC" to message channel. Updates docs, new directory node information.
Squashed down latest changes. |
This is proposed as an alternative to #1000 .
After seeing several issues that were related to using Lightning as a backend, for example:
So this PR comes back very close in spirit to what was originally proposed in #415 , that is to say: directory nodes serving on onions, peers use the directories to get locations (onions) of other peers to talk to them directly. Pretty much all the basic design elements here are the same as #1000 (handshakes, how directories and lists of directories work, how peers use direct connections for tx negotiation), it's just the onions are "in-house" in Joinmarket.
I've got to the point of being able to do transactions on signet as before. There is a new doc
onion-message-channels.md
in/docs
which is probably worth perusing.Of particular note is that
test/e2e-coinjoin-test.py
is now added which does a set of transactions on regtest. Note that these 'bare regtest' tests use only localhost connections for efficiency, not onions. The signet stuff is probably what we should focus on, as per the doc.