Facility to use multiple message channels (including IRC servers or other)

[AdamISZ]

DO NOT MERGE - this is now a protocol break as of 6b8fcce

The goal is to make it transparent to current users - existing config files will still work the same. Adding a new server should be as simple as adding another item in comma separated lists to the config MESSAGING section.

The commit comments give the basic gist of it. Any code review would be great.

I will add some more details later here.

I'll start running it on freenode and cyberguerrilla myself today, probably.

[coveralls]

Changes Unknown when pulling a7a09df on AdamISZ:mc-collection into * on JoinMarket-Org:develop*.

[chris-belcher]

Thanks. I will review the code soon.

From a quick skim of it, I think making the irc options a csv list in joinmarket.cfg is a bad idea. IMO it's better to move them to a separate file (maybe in json format). It's what I did in the old multiirc branch. It's far more extensible, we may thank ourselves later for doing it when adding matrix or another msgchan.

[AdamISZ]

It's not so much that it's a bad idea, as that I put it off. I wanted to use subsections in the config, but then found out that ConfigParser doesn't support that, surprisingly. So I just used something minimal that works OK for now.

So yeah, flexible config is a TODO. I hadn't intended to go as far as a separate file, but sure, that works.

[coveralls]

Changes Unknown when pulling 088ca1d on AdamISZ:mc-collection into * on JoinMarket-Org:develop*.

[AdamISZ]

I started running on cyberguerrilla (normal pit) and freenode #joinmarket-pit

If anyone wants to join me in testing it, do the usual trick (I'm posting this assuming there are other git ignoramuses out there, or people with equally bad memories): git fetch [remotename] refs/pull/568/head:pull_568 and git checkout pull_568 .

Edit the joinmarket.cfg (see above for comments on this, it will probably change to a better model soon): this is what I have:

host = irc.cyberguerrilla.org, chat.freenode.net
channel = joinmarket-pit, joinmarket-pit
port = 6697, 6697
usessl = true, true
socks5 = false, false
socks5_host = localhost, localhost
socks5_port = 9050, 9050
#for tor
#host = 6dvj6v5imhny3anf.onion
#port = 6697
#usessl = true
#socks5 = true
maker_timeout_sec = 60

If you connect over Tor instead to cyberg (not freenode, you can't apparently), it ought to work just as well as before, so if it doesn't, that's an important bug report.

[coveralls]

Changes Unknown when pulling 65bb4de on AdamISZ:mc-collection into * on JoinMarket-Org:develop*.

[AdamISZ]

Been running for several days on the latest version with apparently no problems, including a lot of transactions. But since I'm the only one on the freenode channel, it's a bit artificial. Anyone could give it a go?

[chris-belcher]

Could we go to another IRC server instead of freenode? Because freenode doesn't scramble the hostnames so anybody can see the bots IP addresses.

I suggest maybe Rizon, QuakeNet or EFNet.

[AlexCato]

Will run a YG using this later today. And I'd prefer not to leak IP adress info as well.

[chris-belcher]

Even if yield-generators don't care about their IP, takers almost certainly will.

I've got a list somewhere of good IRC servers to use that I wrote when I coded the multiirc branch a few months ago.

[AdamISZ]

Good stuff, thanks. I remember a suggestion on IRC: https://www.oftc.net/Tor/ didn't really look into it though.

[AdamISZ]

Somehow I was not happy with any particular way of doing the config, but I paid attention to your advice @chris-belcher and I agree that the extra file is probably best due to being extensible. The problem is it's not really as human editable as it should be (most would rightly balk at editing json by hand) and so we should really create a "add msgchan" py script, which is a bit of a shame. Or did I miss a better way? I'll start from your template config, looks like a good layout.

[chris-belcher]

maybe XML ?

[AdamISZ]

Both json and XML are human editable, but aren't intended to be used like that, right. Maybe I'll just add a type variable to the config for now, because people can easily just add an extra set of entries for another server and have each one be type=irc, irc,.. (matrix is not in a state to be used, and we don't have any other candidates). This way there'll be minimal intervention. Better configuration can be a separate PR once this is stable; seem reasonable?

[AlexCato]

Just to document this here as well:
Adam and me found a problem with this yesterday. If a privmsg was sent to our bot and our bot has never seen a public message in the IRC channel from the nick, then the bot cannot reply to message because he does not have a record of which IRC server the user sending the privmsg is on. It just doesnt reply in that case.
This often happens in long-running tumbles, where the tumbler just sends a privmsg out of the blue to any maker. If the maker joined the irc after the tumble originally started, the maker has never seen any communication from the tumbler before.

[chris-belcher]

I realised it's only very rarely that we'd want the actual users to edit the IRC servers to connect to. So I'd say it doesn't have to be a very human-editable file format, it could be json.

[coveralls]

Changes Unknown when pulling eda7c64 on AdamISZ:mc-collection into * on JoinMarket-Org:develop*.

[AdamISZ]

Thanks to @AlexCato a few days ago for the keen eye, see above, added code to "see" a counterparty if their first communication is via privmsg (fill) rather than only if via pubmsg. Live testing it some more.

[AlexCato]

tACK
Ran this since you patched the privmsg and got quite a few successful joins since then. The error mentioned 3 days ago did not occur again.

Edit:
I'd recommend to change this message to something less scary. Sounds like there's a problem, even though it's perfectly okay:
Failed to find an alternative message channel for: [nick], marking as left from: ('[irc_server]', [port])

Maybe something like:
[nick] left [irc_server]:[port] and seems not to be on any other irc server i know.

If that message is even needed at all.

[AlexCato]

Still up an running for several days in a row without any problems.
Before merging, I'd recommend to add at least a 2nd (and possibly 3rd) IRC server to the default options to mitigate that single point of failure, without any additional config required by the user.

[AlexCato]

The moment I say everything's fine...

13:52:12,989 [ThrottleThre] [DEBUG] failed to send ping message on socket
13:53:12,989 [PingThread ] [DEBUG] irc ping timed out
13:53:12,989 [PingThread ] [DEBUG] errored while trying to quit: error(9, 'Bad file descriptor')
13:53:22,682 [MCThread ] [DEBUG] <<pubmsg nick=Teyemijer message=!orderbook
13:53:45,300 [MCThread ] [DEBUG] Cannot reconnect to dropped nick, no connections available.

I have 2 IRC servers configured, and since there's still an "!orderbook" coming through (so that will be CgAn), the one to irc.rizon.net probably broke but does not/cannot be re-established?

I still had coinjoins after that time, so this doesnt break the whole system/other irc connection.

Edit:
Tonight it also affected the CgAn-IRC. Unsure whether it really was offline for a while, but at the minimum the code was not able to reconnect (error messages above ad inifitum, every 6 minutes, for several hours until i manually restarted). Stopping and restarting the yieldgen fixed it, so there must be some state it reaches that it cannot get out of, which is different from a restart.

[AdamISZ]

Hmm. The "errored while trying to quit" is not particularly indicative; we've always had those.

The tests in the test suite test cases of one channel going down and having transactions continue/complete.

The case of a channel going down and then restarting correctly is more difficult to figure out. The specific scenario you described: it dropped from CgAn, failed for a long time, but didn't manage to restart, does indeed suggest there's a problem. Could you email me the log of that yg? (scrub it if you like). Thanks.

[AdamISZ]

A brief look at the code suggests it ought to work (see MessageChannelCollection.on_connect_trigger and .on_disconnect_trigger). I'll try to find a way to create that scenario in a test. I thought I had already seen it work.

[AdamISZ]

(Edit: for clarification, I did this test)

Test: start 4 ygs on 2 local miniircd servers. Check they respond to !orderbook.

Then, manually kill one of the irc servers and tail -f the log of one of the ygs. see the on_disconnect fire, see the connect attempts (every 30 seconds). !orderbook still works OK on the non-shutdown server.

Then, manually restart the (second) irc server. See the bots reconnect OK. Then, !orderbook them on that IRC server, and they respond on that server.

Seems basically correct. I guess looking at the log might help.

[AdamISZ]

Did a sendpayment with manual picking of our running bots on the 3 channels. It seems OK, at least I can't see anything wrong in the log. Removed the inappropriately scary error message.

[AlexCato]

Yeah, it's the best kind of error, an undeterministic one (/s)... because my ISP disconnects every 24h automatically for a second, it usually does recover just fine. Just sometimes it does not.

Will send you the logs later when I'm back home.

[AdamISZ]

@AlexCato you could take a look at that transaction I made too, in that log. Easy to find, the amount is 1000000 ; what is interesting is this: I was watching the channels, and while both your and my yg bot participated successfully, your bot did not send out any orderbook updates after either the unconfirmed or the confirmed event (while mine did).

[AlexCato]

The non-update is intended:
If there's e.g. 3 mixdepths with coins in it:
mix0: 1 btc
mix1: 2 btc
mix2: 3 btc
That will announce offers with a maximum of 3 btc.

And if you request to join 0.5 btc, it moves those from mix0 to mix1. But the published amounts available for coinjoining are not changing, that's still 3 btc. So no update will be sent out.

[AdamISZ]

Oh my mistake, I forgot, thanks.

[coveralls]

Changes Unknown when pulling c3646f0 on AdamISZ:mc-collection into * on JoinMarket-Org:develop*.

[AdamISZ]

@AlexCato The above includes a bugfix: on_disconnect_trigger calls the on_disconnect callback in the bot (such as yieldgenerator). If this is set to None (which it is by default, and is for yg), then it would crash before because I didn't check that it was not None. So that's fixed and similarly for other callbacks. Also un-scarified the debug for nicks leaving.

I believe this was the cause of both your and my case of "dying after disconnect". I tested manually that that specific code change allowed reconnection after all channels were lost (that's what seemed to happen in both your and my cases).

I think one thing remains: on_nick_change, need to check that it doesn't do anything naughty. I'll look at that soon.

[AlexCato]

@AdamISZ thanks for fixing this and sorry for the late answer, busy times lately. I'll get the updated version tonight and keep an eye on it the next few days.

[AdamISZ]

Latest commit 6b8fcce is intended as a proposal for discussion, and note it is a protocol break. Do not merge as of now.

What is implemented is:

• Nicks are 1 identifier byte + 1 jm_version byte (currently '4') + 10 bytes hex prefix of sha256 of ephemeral ecdsa pubkey (with the same nick across all message channels of course)
• All privmsgs are signed using this ephemeral key, the signature is of the same type as already used in tx negotiation, and both pubkey and signature are appended (hex, b64)

With this arrangement, it is safe for participants to dynamically switch channels at any point in the conversation, because we ignore messages that are not correctly signed.

Current approach: pubmsgs are still allowed without verification. The entry point to private messages is announce_orders which will be a privmsg with a signature; importantly, the maker sends this on all channels that he has available (TODO make sure it chooses the channels on which the taker !ob was seen); at that point the taker knows the maker's nick is verified, and the taker will mark the maker as available/seen on all those channels, so that he can dynamically switch from one to the other in case of a disconnect. Likewise, the maker is marking the taker as seen where the verified privmsgs were received, so if a taker drops one connection and then sends a (verified) message on a new channel, conversation can continue on the new channel. (This part is tested by the test code, seems OK).

I'm going to review the logic of the above, feel free to chime in on how it should work.

What should be fixed/improved, at least my current thoughts:

• Pubkey recovery should be used to allow sig only (~100 base64 bytes i suppose).
• Hex encoded 10 bytes is way too small (5 bytes entropy), need ~8 at least for a reasonable safety margin against brute force, but therefore need a sufficiently compact encoding that nicks are not too long (to do: find out the longest length of nick that's going to at least be OK on IRC, not to mention other m-channels)
• First two bytes, for "type" and "version" let's say, completely open to discussion on that

I don't think an extra ~100 bytes per message is going to change much; at the margin there will be a little more throttling, but that is mainly happening with messages that are between 1000-5000 bytes.

(Passed build test locally, but there is a merge conflict apparently)

[AlexCato]

I do not see any more weaknesses in this approach when "TODO make sure it chooses the channels on which the taker !ob was seen" is implemented.

Since we're talking just a few bytes back and forth in IRC channels here, I dont see a need to optimized the hash to the absolute minimum effective length; why not just use at least an order of magnitude more of what you deem safe? This data will not be stored forever, contrary to data on the blockchain, so there should be some safety margin imho. Some makers probably will use the same ephemeral key for several weeks.

What's the identifier byte good for?

Overall, this seems like a good change to make.
If we already break protocol though, other protocol-breaking changes should be combined with this for the next release.

[AdamISZ]

Since we're talking just a few bytes back and forth in IRC channels here, I dont see a need to optimized the hash to the absolute minimum effective length; why not just use at least an order of magnitude more of what you deem safe? This data will not be stored forever, contrary to data on the blockchain, so there should be some safety margin imho. Some makers probably will use the same ephemeral key for several weeks.

I'm worrying about maximum nick length enforced on IRC servers. We might need to find the most compact encoding to fit a reasonable entropy in ~ 14 characters (pure guess as to what is reasonable there).

What's the identifier byte good for?

I don't have a strong argument for it; I was thinking of dropping it too. I think it first entered my head thinking "there might be different rules for different messaging servers", plus "some messaging servers won't allow a number as first character". I can imagine other arguments for it too, e.g. "different bot types in the future might need it".

Overall, this seems like a good change to make.
If we already break protocol though, other protocol-breaking changes should be combined with this for the next release.

Yes, sorry this was tacitly assumed. It seemed most sensible to keep developing this on this branch, but at some point we should have all updates on one new branch (I think we already have a 'newprotocol', but probably a new one).

[AdamISZ]

e8ad6fa , after some chat on IRC: most (all?) IRC servers allow NICK_LEN >> 20, so we seem safe to have a system like: 1 type byte, 1 joinmarket_version byte, 14 bytes base58 encoding (but not b58check) of first 10 bytes of sha256 of pubkey. This b58 encoding is right padded with 'O' to its max len (since b58 is not a fixed length encoding).

Base58 chosen due to alphanumeric requirements of message channel usernames/nicks.

This should leave room for nick cycling. Build test passes.

NICK_HASH_LENGTH and NICK_MAX_ENCODED are set as module vars in message channel; any change to the former needs the corresponding change in the latter. 10 bytes is considered an adequate defence against brute force (considering this doesn't have anywhere near the requirements of a private key).

[AdamISZ]

Current approach: pubmsgs are still allowed without verification. The entry point to private messages is announce_orders which will be a privmsg with a signature; importantly, the maker sends this on all channels that he has available (TODO make sure it chooses the channels on which the taker !ob was seen); at that point the taker knows the maker's nick is verified, and the taker will mark the maker as available/seen on all those channels, so that he can dynamically switch from one to the other in case of a disconnect. Likewise, the maker is marking the taker as seen where the verified privmsgs were received, so if a taker drops one connection and then sends a (verified) message on a new channel, conversation can continue on the new channel. (This part is tested by the test code, seems OK).

Had another think about this part, and I believe what is currently coded is the right approach. In detail:

• Taker !orderbook on all channels he connected to.
• Maker marks taker as "seen" whereever orderbook was sent.
• Maker sends order announcement to every channel on which taker was seen
• Since the above is public information, it's OK if it was sent to a nick-squatter
• Taker responds with a !fill privmsg with a signature, and maker checks, so if it's a squatter it gets ignored
• All privmsgs received on any channel are sig-checked before the nick is marked as "seen" on that channel, so an impostor cannot switch the conversation to his channel.
• A nick which drops triggers the on_nick_leave callback, which marks him/her as unseen , so an opportunistic impostor cannot reconnect after seeing the genuine user leave, and successfully pretend to continue the conversation (would fail to trigger see_nick).