JoinMarket-Org / joinmarket

CoinJoin implementation with incentive structure to convince people to take part
398 stars 119 forks source link

Organizing: tags and releases - call for opinions #343

Open AdamISZ opened 8 years ago

AdamISZ commented 8 years ago

I want to open a discussion on what needs to be done to start creating tags. How best to do it and what to include. Whether to use releases or version numbers, also.

I would propose we have 2 milestones as of now. In broad terms, the first is "tidy up what we currently have, and merge tested, useful improvements", the second is "make all improvements to the protocol that have broad agreement, maintaining backwards compatibility"( here '0.1' and '0.2' for short).

0.1 : Tidy up and merge improvements.

  1. PEP-8 and related code tidy-up. Including code formatting, import tidy up, and moving out dependencies: libnacl, secp256k1 python binding (note: issue with donation addresses). bitcoin can no longer be moved out. See #152 #340 #216 #254 . But I think significant refactoring should remain out of scope for now, even if that seems plausible.
  2. Secp256k1, requires a little more work to make installation more seamless. Currently on branch secp256k1 in this repo, and see #291 . Note: this addresses security concerns about existing code (RFC6979 implementation, message sign/verify are two identified issues but there may be others), and performance.
  3. Move yieldgenerator config into joinmarket.cfg. Seems entirely logical and uncontroversial, see #246.
  4. Although serious anti-DOS should be deferred to '0.2', we should consider addressing #298 and #311 - let's say, to avoid someone's bug jamming the system too easily.
  5. 329 - hopefully @chris-belcher can decide whether this should be merged, if it is an improvement. Perhaps one possibility is to add it as a second version of the simplest yield-generator script for now?

  6. 308 #338 - not the highest priority, and not yet complete, but very desirable; if it's too much work to do now, at least bump up the default fees.

  7. 283 is a bugfix.

    0.2 : Upgrade the protocol (try to preserve backwards-compat.)

Two points to start: (1) @adlai emphasises the idea of using new order types to avoid breaking compat. This seems like a strong principle, although no guarantees it will cover all situations. (2) Our goals in making these changes seem to be broadly: to preserve privacy, to preserve usability for taker side, to prevent DOS and to improve scalability.

  1. Privacy strengthening: Counterparties making some form of commitment to the utxos they claim to own. There is a long and meandering discussion in both #156 and #328 and probably elsewhere about this. It seems far from settled, but, without some changes in this regard I think we all agree that we're too open to at least the type of attack in #156. The ideas in this gist came out of IRC discussions, and might help also. Creating new order types that expect such commitments in !fill or !auth messages etc. would be a way to implement whatever we decide without breaking old bots. We also have to take into consideration that spending from external wallets might limit or change what such commitments can look like #315.
  2. Scalability: See #307 for an example of the problems that exist today. See #248 for some high level discussion of possibilities. The multiirc branch (from #116) has a step towards this, @chris-belcher can comment on progress there. There have also been discussions about code related to flooding/rate limiting, and suggestions like #300 and #31 - arguably this could be a '0.1' thing, as it's a more basic improvement.
  3. 171 is an attempt to bundle up protocol changes from earlier - some of it interacts with some of the things mentioned above, but it should be included/merged with the results of these discussions. There are various suggestions for this, difficult to categorize. E.g. #229 #287 #334 #337 #105 .

To summarise this '0.2' milestone: 2 basic objectives: general improvement of the protocol (mostly for privacy but also usability), and creating a more scalable and stable messaging layer.

Since this is so complex I would propose documentation. We currently have this only for the messaging layer, and #336, and a lot of useful user level information in the Wiki, but to give a framework for discussion on the protocol changes, I suggest a full "protocol documentation" doc (target for 0.2, so not immediately).

OK, to finish: let me know what you think about the above, but also how milestones, tags, releases should be managed. I have no real experience in that kind of thing, so my opinions are limited.

ghtdak commented 8 years ago

A couple months ago, I did a quick but fairly in depth reorganization of the codebase. The first to go was the path adjustments in the code which preclude static analysis (linting). Then, I created a joinmarket module... moved the code around, followed by fixing how imports were used... etc.

I also found some module-level dynamic variables (dynamic global shared (danger!!!)) which, IIRC, I was fairly successful in changing (I'll take another look soon).

There is some code which was bright red under static analysis. From what I can tell, much of that is unused (different kind of red flag :-)

Joinmarket isn't a large codebase and many of the larger structural problems can be fixed in a couple days. However, recognizing / communicating that there is a problem needs to happen first. The code doesn't really follow "best practices" and has a few major red flags (e.g dynamic module level variables) which are guaranteed to cause bugs and will be impossible to find. Because these bulk changes will affect everyone, community buy-in is critical.

My efforts can be found at: https://github.com/ghtdak/joinmarket/commits/master-refactor_v1

raedah commented 8 years ago

"Move yieldgenerator config into joinmarket.cfg. Seems entirely logical and uncontroversial, see #246."

Actually complicates things when you consider that we are moving towards multiple yieldgens which in all likelihood wont be using the same variables.

AdamISZ commented 8 years ago

@raedah I wouldn't say that. Probably I oversold it :) But, different bots can after all read different config variables.

One can argue that command line flags are better than config file variables. I think the counterargument is that if the command line has too many flags, it gets unwieldy. I'm easy either way on that, but both are clearly better than editing the .py script.

Anyway, guess it's a low priority one either way.

chris-belcher commented 8 years ago

Broadly agree with OP.

An issue with #329 is that it makes the attack in #156 much easier since the taker can partially-fill every mix depth much easier.

Multi-IRC as in #116 is mostly written I believe. It only needs more testing and actually finding a few suitable IRC servers that different people should register.

I think the first thing we should do is get #340 merged.

raedah commented 8 years ago

responded to @chris-belcher about #329 over there, https://github.com/JoinMarket-Org/joinmarket/pull/329#issuecomment-161892537

AdamISZ commented 8 years ago

@chris-belcher Are you OK with a 'develop' branch, we could fold #340 into it now and start using it from now on for testing. @adlai and I both like this model, as described (and nicely illustrated) here. IIUC this will mean on this repo, only having a develop branch and a master branch, with occasional release branches created. Feature branches (like our current multiirc or secp256k1) would be kept on separate developers repos individually, not here, typically.

AdamISZ commented 8 years ago

Pushed a develop branch with #340 and a small testing fix. If we agree this is a good way to go, then we should start doing our testing on that branch. I guess I'll fix up travis to work again there too.

ghtdak commented 8 years ago

On Fri, Dec 4, 2015 at 3:10 AM, Adam Gibson notifications@github.com wrote:

Pushed a develop branch with #340 https://github.com/JoinMarket-Org/joinmarket/pull/340 and a small testing fix. If we agree this is a good way to go, then we should start doing our testing on that branch. I guess I'll fix up travis to work again there too.

It appears that something has gone wrong on the way to / from github. The blame preservation seems to have gotten un-preserved

Edit:

All good now, see: #345

chris-belcher commented 8 years ago

@AdamISZ Yes a develop branch is a good idea.

AdamISZ commented 8 years ago

Small progress update: develop branch is now active. See 0.1.0 for progress on the first of the above goals.

Major refactoring done by @ghtdak . He has considerably more work of that type planned, but don't want to go further with the running code at this stage. New yieldgenerator types provided by @raedah. Dynamic fees done, although it's rough around the edges. Open question: whether and what should be done about secp256k1.

Testing on the develop branch is greatly appreciated.

AdamISZ commented 8 years ago

I think we are quite close to done, but given the usage impact, I think #300 #31 should really be added to the goal of 0.1.0, or at least something should be done about it, just to throttle and prevent flooding, albeit a more sophisticated full solution will be needed longer term. Apart from that, I think we should treat 0.1.0 as done, and postpone #291 secp256k1 .

AdamISZ commented 8 years ago

Added #366 PR, I have tested but it for sure needs more tests (any critique welcome), to address #300 and #31 to at least the extent of "if people are using this and not trying to attack we can prevent flooding fails". I'll sneakily add #367 to the 0.1.0 milestone as this is something much wanted - be able to see unconfirmed coins in the wallet.

AdamISZ commented 8 years ago

v0.1.0 is now merged to master. Notes on the process in #374.

AdamISZ commented 8 years ago

I'm doing some work on documentation (see the comment at the end of the OP), see the repo. The idea of having a separate repo is: (1) wiki is user level docs, this is for technical docs (2) want to make it easy to have multiple contributors (wiki can do this too but PRs are a nice feature) (3) can create multiple different file types if we want (although I'm going with .md generally as it's easier for most to deal with than tex/pdf). If anyone wants to contribute/make PRs, go ahead.

raedah commented 8 years ago

thanks. docs are important. ill try to contribute.

chris-belcher commented 8 years ago

Here are some smaller projects I think it would be worth finishing for the next release.

403 shouldn't be too hard and can make a big difference for debugging crashes.

239 is newly re-broken but shouldn't take too long to write. It's an easy fix that can make a huge difference for users of tumbler and sendpayment.

118 also makes a big difference to tumbler users without too much code. It links in well with #56 which also makes use of timeouts, #408 should be merged and that script extended by checking if the transaction did in fact reach the bitcoin network using the timeout code.

Adding this https://github.com/JoinMarket-Org/joinmarket/issues/156#issuecomment-123385964 to detect whether spies are attacking JoinMarket probably shouldn't take that long compared to how good it is, and sometimes I get the feeling its a very urgently-needed update.

There are many issues tagged Easy / Quick which could be some easy projects for any new contributors who'd like to build experience on something.

AdamISZ commented 8 years ago

@chris-belcher feel free to make a 0.2.0 milestone and start adding issues to it ... I think the bigger/more general points above have to be included, but we haven't made any concrete progress on them yet (anti-sybil measures and scalability/messaging).

Re #239 it seems that the debug replacement was a complete disaster, sorry about that, I have to take some responsibility for not paying enough attention there...