Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

New yieldgen algo #329

Closed

Conversation

raedah
Copy link
Contributor

@raedah raedah commented Nov 22, 2015

Complete replacement of create_my_orders. No longer tied to mixdepths. Now dynamically builds orders based on user configurable settings that are clearly explained. Defaults are sane. Made customer order definitions accessible to users, and added additional safeguards to make sure they the user cant define an order that runs at net loss.

Many more user configurable options now available for tuning of order creation.

Mixdepth selection is now clearly defined. Use largest usable mixdepth is set by default, but other modes are in the code comments, with descriptions of pros and cons.

@chris-belcher
Copy link
Collaborator

I think its better to keep the old algorithm too. Many different algorithms out there is healthy. So I'll make this another branch on master instead of merging/replacing.

Some issues:

Could you explain the reasoning behind the default algorithm? From what I see it announces several orders of different amounts and a different price for each, but the amounts are just chosen randomly, why is that?

Did you forget return here? https://github.com/raedah/joinmarket/blob/new-yieldgen-algo/yield-generator.py#L83-L87

sys.exit() needs an argument, try sys.exit(0) https://github.com/raedah/joinmarket/blob/new-yieldgen-algo/yield-generator.py#L134

I notice the on_tx_unconfirmed() are not changed. They were specific to my new-yieldgen-algo so I'm surprised they still work. Have you been running this for real?

@AdamISZ
Copy link
Member

AdamISZ commented Nov 22, 2015

On that topic @chris-belcher ; I'd always thought it would be nice to have a setup like:

bots/ ...
       maker/...
           basic-yield-generator.py
           new-yield-generator.py
           etc.
       taker/...
           sendpayment.py
           tumbler.py
       mixed/
           patientsendpayment.py

and so on. Or anything like that. Then we could have effectively a "repository" of different scripts. Should be better than different branches, right?

@raedah
Copy link
Contributor Author

raedah commented Nov 23, 2015

Mixing depth correlation for offers was removed do to, "Bad faith taker spy not filling orders so that it learns which UTXOs belong to which maker, allowing future unmixing #156" #156

Algorithm options are #fibonacci, evenly, random , and the default is fibonacci. The offers scale up from the min offer size to the max offer size, at the same rate as the fibonacci seq.

L83-L87 is a generator function, so no return needed.

Yes, sys.exit() can get a zero there.

I looked at on_tx_unconfirmed() and it looks okay to me. Just updates the orderbook as needed. Ive run it a few days and through a few dozen transactions and with no issue.

@raedah
Copy link
Contributor Author

raedah commented Nov 23, 2015

@AdamISZ If there is something this yeildgen doesnt do, I can add it as an option, rather then duplicating code.

@chris-belcher
Copy link
Collaborator

@AdamISZ I'd say to not use the maker/taker terminology, instead use investors/privacy or something. But otherwise agreed.

bots/ ...
       investors/...
           basic-yield-generator.py
           new-yield-generator.py
           etc.
       privacy/...
           sendpayment.py
           tumbler.py
           patientsendpayment.py

@AdamISZ
Copy link
Member

AdamISZ commented Nov 23, 2015

@raedah If you've read #156 then you know that one idea discussed there is having a "polyculture of maker algorithms". You could see it as a different / another approach to improving privacy, compared with making a single bot more sophisticated. Consider that people are inevitably going to write their own custom versions of the yield generator to do exactly what they want - it makes sense to have a repository.

Re: duplicating code - yes, it will happen in this scenario, but it needn't happen more than trivially, if the code is well written (might need a bit of refactoring though - ideally derive subclasses from Yield Generator, which itself becomes a template).

(To be honest, I don't consider any of this super important, but it helps to know that there is this alternative perspective instead of thinking we must make THE yield-generator function in an ideal way).

@mb300sd
Copy link
Contributor

mb300sd commented Nov 23, 2015

I also like the idea of moving the yieldgen algos to separate files. I made this branch a couple days ago that preserved all the git history on new-yieldgen. The branches no longer merge cleanly for me, before, upgrading was just git pull && git merge origin/new-yieldgen-algo.

https://github.com/mb300sd/joinmarket/tree/new-yieldgen-merged

I can submit a pull if anyone likes it.

@raedah
Copy link
Contributor Author

raedah commented Nov 24, 2015

I totally agree with the idea of having multiple yieldgen examples where needed. I also believe they should be organized and with specific purposes. Where code can be merged, to reduce duplication, it should be. My yieldgen is based on the mixdepth yield gen code, and I designed it to remove the privacy leaks that happened by basing the offers on mixdepths. I dont believe we want to leave the mixdepth example because it is a privacy leak and no one has yet to explain a clear reason why someone would want to base their orders on mixdepths. This pull request has the same incentives built in.

If there is a sensible design for another yeildgen, then yes lets include it. If its a feature that can cleanly and sensibly be integrated into the existing code, then lets do that. If there is something this yieldgen code doesnt do, please state it. Lets not create multiple versions just for the sake of having multiple versions. This will only confuse new users and make the code base more difficult to manage.

Also note, Ive been running this code for many days, with lots of transactions, and no errors.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 24, 2015

@raedah I didn't intend to argue against bugfixes (I haven't looked at your code, although I know @chris-belcher has). But "multiple versions for the sake of multiple versions" is not an accurate representation of the point I was making, because multiple versions has a distinct advantage.

If this PR is unambiguously a set of bugfixes and improvements, with no disadvantages or trade-offs, and not an alternative, then that's different, and this separate point is moot.

@raedah
Copy link
Contributor Author

raedah commented Nov 24, 2015

To be more specific, the reason we are talking about multiple versions of the yieldgen is because we want more diversity in the order book. Yieldgen solutions that offer more customization to the user fulfill this purpose quite well, because the configuration to customize is accessible and it has a much lower barrier to entry then writing your own code.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 25, 2015

@raedah Yes, I think that's a fair perspective, although it is limited - you can't write a script that does every possible thing that others might think of. But again, this is more or less a moot point - if your changes here are a straight improvement and not a trade-off, then the diversity issue is separate.

@chris-belcher
Copy link
Collaborator

In the old new-yieldgen-algo, the purpose of basing orders on mixdepths was that each mixdepth has a different price to coinjoin with, this gives takers an incentive to coinjoin with lower balance mixdepths and so keep the maker's coins clumped together in one mixing depth.

This new new-yieldgen-algo also has several orders, their amounts are distributed by fibonacci, evenly, randomly or user-chosen. Is there a reason for this, my issues is I'm not understanding why there is a need for several orders, could it just be replaced by a single relorder or absorder with a maxsize equal to your wallet's balance?

@raedah
Copy link
Contributor Author

raedah commented Nov 25, 2015

"this gives takers an incentive to coinjoin with lower balance mixdepths and so keep the maker's coins clumped together in one mixing depth."

Okay. I was not sure what we were trying to do here, but I get now that we are wanting the option to lump as many coins into a single mixdepth so that the largest amount of coins are available to join with (since joins always come from a single depth), and then the maker can command a higher fee for the larger amount (since larger amounts are more scarce.) And where possible, to provide lower cost incentives to takers in order to get those coin amounts moved into the most clumped depth. I will look into this.

"my issues is I'm not understanding why there is a need for several orders"

Because higher amounts of coins command higher fees. The fees are distributed upward as well as the amount.

"could it just be replaced by a single relorder or absorder with a maxsize equal to your wallet's balance?"

Yes. Just change num_offers = 1

@raedah
Copy link
Contributor Author

raedah commented Nov 26, 2015

Just committed updates. "keep the maker's coins clumped together in one mixing depth" is now the default mode. See near line 244. This applies for all joins, regardless of what the offers are.

As far as incentives, I brought back 'bymixdepth' as a config option so offers can now be correlated with mixdepth sizes as they were before. I also brought back cjfee as custom_cjfees so that this can be defined easily by the user as well.

Anything else?

@raedah
Copy link
Contributor Author

raedah commented Dec 1, 2015

I have been using this for a week with no bugs and no more incoming updates.

@raedah
Copy link
Contributor Author

raedah commented Dec 3, 2015

My opinion is this can replace the old code, but I will go along with having this merged as a second yield generator. I have ideas for other yield generators, so there will be many anyways.

@raedah
Copy link
Contributor Author

raedah commented Dec 4, 2015

@chris-belcher . Regarding, "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."

Not following you. The old code forced the orders to correspond with mixdepths. The new code does not. In the new code, even if the user specifies that they want the order amounts to reflect the mixdepth amounts, that still does not determine which mix depth ends up filling the order.

@chris-belcher
Copy link
Collaborator

Yes you're absolutely right.

@raedah raedah mentioned this pull request Dec 6, 2015
@AdamISZ
Copy link
Member

AdamISZ commented Dec 7, 2015

So this has been superceded by #349 right? In any case I'll close it as we're not merging into master any more.

@AdamISZ AdamISZ closed this Dec 7, 2015
@raedah raedah deleted the new-yieldgen-algo branch December 9, 2015 20:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants