Organizing: tags and releases - call for opinions

[AdamISZ]

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 Formatting #152 Blame preserving reformat - simple merge #340 PEP-8 Reformat and language level bug hunt #216 Changing tabs for spaces and structural updates #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 secp256k1 #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 Move yieldgen configuration to joinmarket.cfg #246.
4. Although serious anti-DOS should be deferred to '0.2', we should consider addressing Orderbook bomb [DoS] #298 and unmask sybil counterparties by hostmask #311 - let's say, to avoid someone's bug jamming the system too easily.
5. New yieldgen algo #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. Estimating fees #308 dynamic fee estimates first draft, not yet applied to tumbler #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. Wallet should auto-pay only to the change address branch #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 Bad faith taker spy not filling orders so that it learns which UTXOs belong to which maker, allowing future unmixing #156 and initial blacklist edits #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 Bad faith taker spy not filling orders so that it learns which UTXOs belong to which maker, allowing future unmixing #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 Facilitate delegation of taker role #315.
2. Scalability: See Sendpayment: error(32, 'broken pipe') during tx signing. #307 for an example of the problems that exist today. See Moving away from IRC as a messaging channel #248 for some high level discussion of possibilities. The multiirc branch (from Multi-server IRC #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 Outgoing messages priority queue #300 and Output thread for IRC code #31 - arguably this could be a '0.1' thing, as it's a more basic improvement.
3. A new protocol #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. Matching input sizes in coinjoin #229 Takers should choose orders by another metric not just price #287 req: An order has both a relative and an absolute fee. #334 req: Enhance privacy with multiple change addresses #337 Use multiple change addresses and coinjoin them #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]

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]

"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]

@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]

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]

responded to @chris-belcher about #329 over there, #329 (comment)

[AdamISZ]

@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]

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]

On Fri, Dec 4, 2015 at 3:10 AM, Adam Gibson [email protected]
wrote:

Pushed a develop branch with #340
#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]

@AdamISZ Yes a develop branch is a good idea.

[AdamISZ]

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]

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]

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]

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

[AdamISZ]

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]

thanks. docs are important. ill try to contribute.

[chris-belcher]

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 #156 (comment) 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]

@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...