-
Notifications
You must be signed in to change notification settings - Fork 119
A new protocol #171
Comments
More code / bugfix to add #170 |
Allow option of maker not having a change address #27 (comment) |
@chris-belcher is the current algorithm/protocol written up anywhere? |
Not really, the code is the documentation right now. |
I might try to help with documentation in the future... |
Worth noting that this won't be the last time we update the protocol. We'll probably end up adding segwit and op_schnorr to JoinMarket eventually too. So the code for new protocol should be written in a way that makes adding more order types easier. |
And anything we decide on regarding this issue will be included in a new protocol too: #156 |
#568 is now a protocol change (so as to find a logical way to use multiple messaging servers without impersonation). I am not sure what people feel comfortable with as a reasonable set of changes; anti- #156 is priority 1, it seems; in line with #343 goals for 0.2 I think the above multi-mc should also be included. Re: the above list, I'm not sure, but maybe we should hash out, here, what set is both highly desired, and also a realistic goal for a new version. |
I agree with that, I was thinking of including a few small other things in the first new protocol if theres time/effort (like fixing patientsendpayment.py and the #537 signing change) |
@chris-belcher I'm thinking to start a 0.2.0 branch to get up and running (take current develop + #568 and go from there). (i know there's an existing "newprotocol" branch but seems too old). Make sense? |
Yes that's a good idea. One thing we should get from this protocol break is that the codebase is written so that it's easier to add future new offer types (coinswap, segwit, #229) |
OK, got started. Tweaking/testing a basic podle PR on the now-existing 0.2.0 branch. Won't be final but will be something to look at. Re: new ordertypes, yeah, good thing is I think it mostly works out of the box (bots just ignore what's not in their list of known ordertypes right). Although: when I did segwit I noticed there was some rather ugly hardcoding of ordertypes allowed, but it's just a matter of cleanup. |
That's what I meant regarding the hardcoded ordertypes. And both relorder and absorder today invoke the same functions when the order is filled (CoinJoinOrder in maker.py.) When adding segwit and coinswap it would need an entirely different function. Plus a nitpick: rename it all to offer instead of order. |
https://gist.github.com/AdamISZ/baf93ce2589854a7992383b3c69fae13 An overview, so we can see what the new message protocol looks like. Of course, only a first cut. I'll just push the initial PoDLE update to the 0.2.0 branch now, after all we are just going to keep working on it, so I guess no point having some kind of PR process really. |
43f6b2a is specifically to address the formatting inconsistencies of It seemed wise to isolate it in that one commit. |
7d83041 addresses #88 after prompting by @chris-belcher on IRC. This implementation simply takes the first utxo that the maker has decided to spend and extracts its key from the wallet and uses that for the authorisation pubkey and signature. Hence the protocol is changed in line with the proposal in #88:
to
and the taker verifies that the address derived from Presumably other cases could be handled with additional code, but without needing to change the protocol again. I'll update the protocol gist to reflect this change. |
Another thing I want to mention before I forget: the current PoDLE implementation requires secp256k1 since it makes extensive use of EC point arithmetic. I'm leaning more towards ditching non-secp256k1 (it also finally clears out the mess from the bitcoin module). |
As per IRC, 8ecae43 for standardised bitcoin signature format with prefix. Whole build test passes except tx_broadcast. I think I know what that is, but can be addressed later. |
@chris-belcher thanks for paying attention, fixed in ad1d876 ; it is not actually a change, just a tidy up, as you said pybitcointools version was already "standard". However I did get a chance to investigate more how it works in libsecp256k1 + Core; they use SignCompact with a recoverable signature, which is a recid single byte + 64 bytes. I'm left unsure about the whether, to support hardware wallet, we should be using this kind of signature; I guess probably so. Afaict this should not be a problem as the python binding supports it. But, it's slightly non-trivial work. |
I hope one day we figure out an alternative to the age restriction because it's not always great for user experience; often people want to receive coins and then spend them right away. |
Re: bad user exp, yeah, but it won't be a problem for those with "fuller" wallets. I guess a 1hr delay is quite annoying for a completely new user but given all the other steps you have to go through to get started, I feel like it's not a dealbreaker. At least it doesn't cost or lock up any money. And a heavy user may choose to create external commitments as a backup to avoid problems. Re: ordernames, I like the suggestion, a little worried that it confuses with all the other stuff in the code; but, let's go with that, we could change the use of the word "order" later in the code and logs without breaking anything. |
f152495 PROTOCOL BREAK (Let's write this from now on for any commits that change the proto - hopefully there should be very few more, just feature adds/changes) All hardcoded relorder/absorder changed to reloffer/absoffer. This could be encapsulated better to remove the hardcoded instances at the top level; but that can be done later easily. Note it does not change the use of code variables called 'order' anywhere. Tests OK. I deliberately checked the different ygs in the tests too. Edit: guess it's worth mentioning, though anyone reading this probably knows: offers with a different ordername (so, including 'relorder') are just ignored if they're not in |
fe9f6d5 prints out detailed debug info in a file This is a bit crude (just a single re-used static file), but is the least that's needed to give a user some guidance. A more sophisticated approach would be better; note that it's written to a file as it's too much stuff for a terminal output (which is already super noisy). This is the kind of thing where a GUI really helps. I used the Here is an example of
While doing this I noticed something very important that I hadn't noticed about Tests passing. |
c5860c6 nkuttler noticed when I removed the legacy sig conversion - bot can crash on an invalid signature during verify (asserts in the Python binding code), so wrapped the parsing in a try block; only for verify, not sign of course. |
db8a2ae PROTOCOL BREAK -
note the ":" in CHANLIMIT=#:120, hence parsing to ":" doesn't work. 126a206 just removes hostid from config basically. |
I missed the : thing in testing, I should've tried it on many different IRC networks in hindsight. Will fix soon. |
np, and thanks. btw feel free to push the fix directly to the branch. |
a255d5d allows Makers to broadcast used commitments to the pit with a command An important subtlety is when the broadcast occurs: in a typical scenario Important to remember in this that each individual hash value/commitment is indeed intended to be used only once globally, that's why we have "retries" that allows multiple (default 3) for the same utxo. I ummed and ahhed about this broadcast element for a while, but the more concretely I think about it the more I'm convinced it should be used to get the real rate-limiting effect. So, currently have the Tests passing. |
891cfe9 is designed to reduce the effect whereby broadcasting commitments on the public pit channel unambiguously marks the maker as a participant in a transaction, making it easier for a spy to focus their queries on certain maker bots. This is accomplished with a new method Worth noting the possible imperfections - the receiving party may simply choose not to broadcast; the receiving party may be acting on behalf of a snooper, or be a snooper. But, this seems unambiguously better than direct public broadcast, since failure to broadcast is not in any way critical (this is especially true since there will usually be ~ 3-5 different makers broadcasting the commitment at once). Note that none of this changes the existing Not a protocol break. |
d50ae7c addresses a bug: if a bot cycles nick on one channel it doesn't on the other, leaving the message channel layer confused. Just changed get_irc_nick so that the fixed length (without underscores) nick is picked up. (Hmm this may need a tweak actually, will update in a bit). |
Fixed bug in IRC network name finding 66fdcce Protocol break because Freenode is correctly found now |
53e30d7 seems to be the most logical way to deal with nicks changing, after discussion with @chris-belcher : force the nick/username change on all channels if it happens on one. I'd like to get a solid test of this, but it'll take some considerable work, and it's a very unusual condition, so will start running it in the test pit straight away. Review would be appreciated. |
I'm thinking of making a yg-algo that has the best privacy effect, at the moment all I have is: offer announce is max from maxmixdepth (so one price only, not like current *mixdepth), and requests are responded to with utxos from the appropriate mixdepth depending on requested amount, then (here a big difference to earlier perhaps): the cjout goes to some other mixdepth than maxmixdepth, to avoid reannouncement. Two obvious limitations are: this obviously will not always avoid reannouncement (presumably impossible!), OK that's fine, but also: coins tend to de-concentrate. Thoughts? Edited to add: |
0537a69 (edited after bugfix, seems to work in live tests now) does 2 things: (1) a Second, re the previous comment, created |
cdcbc67 removes the legacy pybitcointools entirely. Some notes:
With respect to Windows, the existing instructions here seem to work exactly as before, albeit it's an ugly business (installing MinGW to get 1 DLL file). With respect to OSX, I have no idea, but the dependencies are listed, one assumes they probably work.
There are also a few redactions from the tests due to the removal of the old code, but that's a very minor matter; the tests were mostly already ignoring the legacy code. I've attempted to reach out to users about this change via IRC, reddit, bitcointalk. Not much response. I hope people can appreciate how hard it is to maintain two different Bitcoin interfaces. |
Complete tumbler test (unedited) run ran OK, so that's a good sign, although it isn't really a test of how commitments can gum things up right now since it starts with a "big" wallet. |
59443a2 does the following:
|
c6c67ba is trying to streamline processes for adding external commitments, or extending the utxos in a wallet, which will be important for anyone trying to act as a Taker (especially tumbler) if they're in a "from scratch" scenario - i.e. fund a new wallet and immediately run. There is a new So your problem is to make sure you have enough commitments to do the job. If you're doing 1 sendpayment your life is much easier; you could just wait 5 blocks before starting, and/or you can fund with multiple utxos instead of 1. You can also add a bunch of external utxo commitments for longer term use. With tumbler your job is harder: even funding with multiple utxos and waiting 5 blocks might not be enough, since they are liable to get consumed during the run. You can make the wait times long enough that they safely cover 5 blocks, but that's long. If you're starting with a fresh wallet you may well need to follow the "external commitments add" workflows described here. So there's various workflows for this: (a) you want to add external commitments, (a1) you have an existing joinmarket wallet (a2) you only have other wallets (like Core, Electrum), (b) you want to make a bunch of utxos in your wallet instead of only 1, (b1) you have an existing joinmarket wallet (b2) you only have other wallets. (a)
(a2) One can also enter single utxos with The script has other minor options like delete and verify utxos too. (b1) Second tool: (b2) This send-to-many feature exists in other wallets like Electrum, Core (I think?). This is obviously better where possible, you can just send 1 utxo to 5-10 new addresses in your joinmarket wallet, instead of only to 1, without doing anything difficult. The bottom line as I see it: we want to use (b2) and in any case, not (a), because it doesn't require any exotic signing and therefore private key manipulation. But I doubt one can really run tumbler this way (even spreading deposit across multiple mixdepths can't work since it breaks the privacy model), and so, since we want to keep instructions simpler for users, I'm more inclined to tell them to use (a) as a mostly one-off job when they start: give yourself say 5-10 utxos (so 15-30 "free tries") from another wallet, using (a2) probably. But this does mean dicking around with private keys. It could also mean some extra privacy loss to makers since they read the utxo that was used on commitment opening. Perhaps the only way to cut the Gordian knot here is to require tumblers to run with a wait > N blocks, and perhaps 5 is too aggressive? |
I've been doing a lot of testing to see how this looks in practice. A couple of things:
Apologies for using this as a dumping ground, but I don't want to forget this either: the privacy enhanced yield generator could implement this feature: if mixdepths that have enough has more than 1 entry, and the last-used-coinjoin output utxo is included in one of that list, then filter it out. To be clear, the utxo must be spent eventually, but I would assume the spy's analysis is made at least somewhat harder by delaying. Edit: Also in 077d3ed : bugfix in choose_sweep_orders - previously, if after filtering no orders were left, then the call to weighted_order_choose via chooseOrdersBy could crash, because element 0 of the list is accessed at the start of that function. The liquidity check that checked for that condition (no orders) only previously was called before the for loop. |
82f09da couple of bugfixes and: add-utxo reads wallet directly rather than creating a file with private keys as above (not secure + inconvenient). |
dcf9333 - update the a4ac313 was essentially a bugfix, because externally sourced commitments weren't being checked for age/size limits, now they are. Full tumbler has been run a couple of times, done a variety of other transactions, I think this is largely stable now. I even wrote some primitive attacker code to try to sweep up utxos from makers, although it needs a lot of work. But with only a few bots in the pit and no real activity, there's not much that can be done in terms of simulation. Spent some time trying to get sensible measures of how many utxos the spy needs; it's nearly impossible, there are so many variables*, but overall I get the impression that at current levels they really won't need much. Could be anything from 10-200 in an hour I think, for the current pit ("hour" is a sensible measure if we stick to ~5 block age requirement, since that's the "refresh rate" they'll need over the long term). And they have to have the right amount range, of course. I think the deterrent factor of creating fresh utxos all the time will be limited, but a combination of people using more "guarded" yg algos that don't advertise tons of information, forcing them to use up more utxos, plus the fact that their own utxos will be very visible and easily identified, might change the dynamics. That's the short term perspective, the long term perspective, if being optimistic, is that if we can scale up, it'll get harder and harder for them. * Here are some I looked at; some of them aren't easy to define, let alone measure:
|
a9ede54 - noticed that if multiple bots use the same directory (not something planned for, but in any case), blacklist file being shared means transactions getting unnecessarily blocked. This pushes back the adding commitments to the blacklist to the point they're actually used, i.e. on sending |
Minor bugfix 980c536 README update (also updated/added wiki articles) a6f191e , which includes reference to 00301e1 which formalizes external requirements
with this alternative for those wishing to connect to HS:
It seems like if the default uses all 3, then a maker requiring Tor only is not at a disadvantage, but, if you disagree, let me know. If a third supporting Tor+clearnet option is available, we can add/change to that. |
1f7252a removes all yield generators except yg-basic and yg-pe ; the former inheriting from YieldGenerator in the joinmarket module so there is no longer significant code duplication. The intention is twofold: (1) can't support multiple customised yield generators, it's impractical, but doesn't mean an attempt to stop them: I'd suggest making a separate repo in the Org to host other implementations, starting with what we already had - but note they actually have to be supported by coders, including testing of course. (2) emphasize that simpler, less information providing yield generators are better for privacy and for the overall system. This point is not completely uncontroversial, of course, but again: this is not an attempt to "ban" customizing, it's an attempt to limit the scope of the core project. |
Closing this issue now, as the |
Issue which combines all the features we want when the protocol is updated.
Talking to some people, it seems doing a breaking change, a "hardfork", is quite an unpopular idea. It would split the liquidity, updating would end up having to be rushed by some people, giving not enough time to check the code. So it seems the compromise solution is to code it that new bots will accept both protocols and the old protocol is gradually phased out.
Stuff to fix:
#88 Fix bug where maker must known the private key of the coinjoin address.
#83 Using p2sh / multisig as inputs.
#45 Some cleanup, changing names, adding a field
#29 Minimum other maker count field
#90 (comment) Allowing taker to not authenticate
#120 Fixing miner fees. Changing maker's tx fee contribution to a fee/kB and having a new field of "lowest fee/kB tx this maker is willing to sign off for"
It should also be noted that the patientsendpayment.py script does not work at all in the current protocol, because makers must know the private key of the coinjoin address they're sending to.
#90 Some code already written
The branch https://github.com/chris-belcher/joinmarket/tree/newprotocol
The text was updated successfully, but these errors were encountered: