Takers: filter out makers with many offers

[AlexCato]

Takers: filter out makers with more than X offers (default 5), configurable.

Tested and works: sendpayment

Since I've never used the tumbler or patientsendpayment, I did not test those. Please have an extra-careful look at the changes there.

[coveralls]

Coverage decreased (-0.04%) to 77.275% when pulling 692a960 on AlexCato:limit_maker_max_offers into 0c919d2 on JoinMarket-Org:develop.

[chris-belcher]

Some points:

It would be better to put the remove_makers_with_too_many_offers() function calls in one of the superclasses like OrderbookWatch or Taker. That way it only needs to be called once instead of in every application script, avoiding code-reuse is a benefit of OOP and we should use it.

Recommend to reword the 'potentially malicious makers' phrasing in joinmarket.cfg, from what I see these makers don't do any harm except make the sqlite database big.

The SQL query of checking the oid value isn't secure. Makers choose their own oid values, today the oid starts from zero and counts up but it can be anything. Makers could set their oid=9001, only have a single offer and get banned anyway. Recommend instead to use an SQL query like 'SELECT oid, counterparty FROM orderbook WHERE COUNT(oid) > ?; which should return all the (oid, counterparty) that have more than the threshold count of offers. You could probably also do it in one statement using the sql DELELE but it's been a while since I did sql so don't remember how.

Perhaps the threshold of 5 offers per maker is too low? There might be some nice algorithms that involve a few more offers than that. For example I remember yield-generator-mixdepth announced 6 offers, one for each mixdepth and an absoffer from zero to the balance of its lowest balance mixdepth.

[AdamISZ]

Perhaps the threshold of 5 offers per maker is too low? There might be some nice algorithms that involve a few more offers than that. For example I remember yield-generator-mixdepth announced 6 offers, one for each mixdepth and an absoffer from zero to the balance of its lowest balance mixdepth.

Yes, in my reddit post I referred to 20 with this history in mind. If it's user configurable then the maker just takes his chances, but in any case I agree we can probably be fairly lax.

[chris-belcher]

Though it's user configurable, the default will be the case for almost all users, especially with the joinmarket.cfg file having so many options now.

I think I got that above SQL query wrong btw, make sure you try it before putting it in, COUNT(oid) might always be 1

[AlexCato]

Thanks for checking this out so quickly!

It would be better to put the remove_makers_with_too_many_offers() function calls in one of the superclasses like OrderbookWatch or Taker. That way it only needs to be called once instead of in every application script, avoiding code-reuse is a benefit of OOP and we should use it.

I've tried to find a suitable place in OrderbookWatch or Taker, but didnt see any. I especially wanted to make sure that this is only done when necessary, i.e. not every time a new offer is announced (only if someone actually wants to take one). Open for suggestions!

Recommend to reword the 'potentially malicious makers' phrasing in joinmarket.cfg, from what I see these makers don't do any harm except make the sqlite database big.

Can do. Though I meant "potentially" as in "hints that this is not necessarily the case". Will be changed.

The SQL query of checking the oid value isn't secure. Makers choose their own oid values, today the oid starts from zero and counts up but it can be anything. Makers could set their oid=9001, only have a single offer and get banned anyway. Recommend instead to use an SQL query like 'SELECT oid, counterparty FROM orderbook WHERE COUNT(oid) > ?; which should return all the (oid, counterparty) that have more than the threshold count of offers. You could probably also do it in one statement using the sql DELELE but it's been a while since I did sql so don't remember how.

I did indeed think about this. Yes, makers can choose any OID they want, but why should they? This statement allows exactly OID 0 through 4 to be used; I can change this so that any combination of 5 OIDs works, but I really dont see what this is good for. I'd not consider it "not secure" though.
Will do if you prefer that still.

Perhaps the threshold of 5 offers per maker is too low? There might be some nice algorithms that involve a few more offers than that. For example I remember yield-generator-mixdepth announced 6 offers, one for each mixdepth and an absoffer from zero to the balance of its lowest balance mixdepth.

Again, I'd prefer lower over higher, and even if it's only meant to make the orderbook more human-readable. >10 makers with 20 offers each violate this goal imho.

Though it's user configurable, the default will be the case for almost all users, especially with the joinmarket.cfg file having so many options now.

Strongly agree. The importance of defaults can hardly be overstated.

[chris-belcher]

I've tried to find a suitable place in OrderbookWatch or Taker, but didnt see any. I especially wanted to make sure that this is only done when necessary, i.e. not every time a new offer is announced (only if someone actually wants to take one). Open for suggestions!

in taker.create_cj() perhaps

I did indeed think about this. Yes, makers can choose any OID they want, but why should they? This statement allows exactly OID 0 through 4 to be used; I can change this so that any combination of 5 OIDs works, but I really dont see what this is good for. I'd not consider it "not secure" though.
Will do if you prefer that still.

Fair point, it just looked wrong on first impression. Maybe put a comment with a short explanation for anyone who makes the same assumption as me.

The aim of a human-readable orderbook probably can't happen long term if the number of makers goes up a lot. Exchange orderbook visuals like http://bitcoinwisdom.com/ collapse their orderbook into a depth chart.

[AdamISZ]

Re: moving to superclass, yes, clearly desirable. I was wondering whether we couldn't hook it into OrderbookWatch.on_order_seen, but it seems tricky; you'd need in that case to keep track of banned counterparties within that class. Still, that might be the right way, since it would then completely avoid any need to modify calling code. I'm sort of warming to that idea, although I've probably missed something.

Makers also subclass OrderbookWatch now, but I don't think that matters too much; they're only using it to cross-broadcast !hp2s, so it's not a problem if they don't talk to banned Makers.

If not that, then it does seem logical to add a function in Taker. But unless I missed something, that still requires added calls from user-level scripts.

Re: restricting oid values or not, I too can see both sides of that argument. I guess I agree with @chris-belcher 's comments, it's OK but needs to be mentioned somewhere as a new rule.

[chris-belcher]

If not that, then it does seem logical to add a function in Taker. But unless I missed something, that still requires added calls from user-level scripts.

I meant adding it to taker.start_cj() not taker.create_cj() (which doesn't exist)

joinmarket/joinmarket/taker.py

Line 639 in 78470c3

def start_cj(self,

The only downside would be that it never gets called in the ob-watcher.py script, although that script could add it to the loop that periodically says !orderbook

In a situation like this it would be nice to use socket timeouts to implement a heartbeat, which already exists in the p2p network code

joinmarket/joinmarket/peertopeer.py

Line 297 in 3756cc4

self.p2p_message_handler.on_heartbeat(self)

and also in this experimental IRC code https://gist.github.com/chris-belcher/39033530fe550506b833e71c6f19f82d If we had that then this remove_makers_with_too_many_offers() function could go in the heartbeat function.

[AdamISZ]

Yeah, start_cj makes more sense. Architecturally it does fit into OBW class though, do you see anything wrong with the on_order_seen suggestion? That way they would never make it into the db in the first place.

We already have a delete call into the db in that function, I guess an extra query in there is not an issue. Just need to have a variable like ignored_makers there so as to remember to ignore all after ban.

[AlexCato]

Thanks for the comments! I hope I'll find some time this weekend to build a solution closer to these ideas.

[AlexCato]

So, had a look at it now:

At the point in time when start_cj() is called, the maker-counterparties are already chosen. Sure, I could remove those that do not fit certain criteria (e.g. too many offers or overlapping offers) by cross-checking their other offers in the orderbook-db, but then the CJ would be performed with fewer counterparties than the taker expected/preferred.
I think thats not much of an improvement. Did I miss something?

on_order_seen() might work, if I keep add a blacklist of "bad" makers in its state.
Objections to me implementing it there?

[AlexCato]

Implemented as discussed in #712