Updates offers automatically on any new deposits

[AdamISZ]

Fixes #1021. Also applies to withdrawals. This commit adds a registration of a callback for all transactions, which then follows up with a callback on the confirmation event, after which we send the AnnounceOffers message to the daemon (after modifying our orders in line with current balance). Minor changes: WalletService.active_txids is a set, not a list, to avoid reannouncements, and the unused argument to on_tx_confirmed is removed.

[AdamISZ]

Tests out fine on the most basic test on regtest: start yg, send funds, see the new offers sent with the correct updated values. But it will need broad testing across other functions to sanity check nothing is wrong.

So, background:

The main job the WalletService.transaction_monitor function is doing, is translating between the return from the RPC call gettransaction, to a callback to every function that subscribes to the events: "new transaction of any kind", "new transaction with a specific txid, unconfirmed", "new transaction with a specific txid/output set, confirmed".

Those latter two are the "normal" case for Joinmarket: we're asking the wallet to tell us when the coinjoin that we negotiated, appears on the blockchain/network. The "all" callbacks are for any other case, and were already being used in Qt to update the balance tab in real time.

Because the stuff that comes out of the RPC call has a bunch of repeats (events per address, for example, not just each transaction once), we do some temporary caching of lists (in particular see active_txids in that function). We use this to help us make sure we (a) only process each transaction but also somewhat contradictory: (b) make sure we keep a txid "in play" if it's been processed as "unconfirmed" but not yet as "confirmed" (which we must do, to update offers; the first, reduces the available balance, and the second, increases it, remembering that yield generators only offer confirmed coins).

Now this new scenario: update the offers list on a deposit or withdrawal is a bit different. We create an "all" callback to check for every new transaction. Then, we ignore it if it's part of our coinjoin negotiation (because that's already being dealt with by the above logic; we don't want to do it twice). Then, once the callback triggers, we add a new "confirmed" callback for this new txid, and we have to add it to the active_txids set (not list! see commit comment), because otherwise the confirmed callback would get ignored (because we already processed this transaction once, and that's the general rule).

I think this covers the general points. There is also a minor refactoring with the function create_new_orders which separates that function from whatever is specific to a coinjoin-specific update (basically the logging in YieldGenerator(Basic).on_tx_(un)confirmed).

[AdamISZ]

OK thanks for the report. I want to double check that:

• the normal offer update work as before (i.e. the offer update that happens when the coinjoin happens)
• that normal wallet refreshes work as before on Qt.
• payjoin

that stuff might seem irrelevant, but this is a change in the guts of the transaction-monitoring engine, so to speak.

Also if anyone has an opinion about making offers in the pit every time we receive a deposit, feel free to mention it. Right now we very often (not necessarily always) make an offer in the pit when we participate in a join. This would add in deposits.

[kristapsk]

Also if anyone has an opinion about making offers in the pit every time we receive a deposit, feel free to mention it.

Cannot this be used to additionally deanonymize specific UTXO(s) of a maker? I mean, you will 99% know the block in which deposit tx were included for a specific maker. Later just need to see which of UTXOs from a specific block participate in any JM coinjoin as a maker.

[AdamISZ]

Yes that was the kind of comment I was expecting :) I even envisaged someone describing an attack where you send coins (because even if you send to a reused address and it gets frozen, in this code, it could still trigger a reannounce).

My first thought was, simplest solution, though it won't be what users want, is: reactor.callLater with a random delay of, maybe several blocks' worth of time. Pretty awkward but also simple.

You're right to raise the "single utxo" thing; offer reannouncing after coinjoins is already giving some info to attackers, this, in certain particular cases, might give even clearer info. At least that's my general feeling.

[AdamISZ]

I could add into the logging here:

https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1042/files#diff-dbe9e61f92c1d62ead3957720feca1a4678576a88d40d1d1e2a5693ea57cc046R437

... that the "offers will be updated after a random delay of 14 minutes 28 seconds" (or whatever comes out of rand()).

[AdamISZ]

Since it seems (to me) a clear improvement, I added the random delay as discussed above, with a INFO logging message. I tested again and included testing with Qt and doing a coinjoin after to check sanity. All seems OK.

I'm more or less happy with merging as-is, but it is still open to opinions.

[kristapsk]

Randomization is obviously the first idea and useful, but also I think we should randomize in both in wall time and block time. But there are more smart things we likely could do. How about checking previous offer against confirmed maximum balance in a single mixdepth and don't announce changed offers immediately if new potentially increased amount is below existing_amount + some_threshold (could be configurable, default to something like 10% or 20%)?

[AdamISZ]

Randomization is obviously the first idea and useful, but also I think we should randomize in both in wall time and block time

Very good point. Certainly what I put so far; 10min-100minutes doesn't really achieve the intended goal considering blocks are sometimes slow.

How about checking previous offer against confirmed maximum balance in a single mixdepth and don't announce changed offers immediately if new potentially increased amount is below existing_amount + some_threshold (could be configurable, default to something like 10% or 20%)?

Yeah I guess we could add that in, though not sure it's worth the bother. Also of course we have the amount randomization in the offers themselves.

I tend towards making it wait 50-500 minutes perhaps, albeit (2-5 blocks + 30-100 minutes, for example, might be better; but I don't want to write tons more code.

[kristapsk]

Yeah I guess we could add that in, though not sure it's worth the bother. Also of course we have the amount randomization in the offers themselves.

I think it's worth, also should not be so hard. Idea here is to reduce number of times offers by a single maker changes that can be correlated to Bitcoin block numbers. Amount randomization does not help here at all.

[AdamISZ]

Yeah I guess we could add that in, though not sure it's worth the bother. Also of course we have the amount randomization in the offers themselves.

I think it's worth, also should not be so hard. Idea here is to reduce number of times offers by a single maker changes that can be correlated to Bitcoin block numbers. Amount randomization does not help here at all.

So coming back to this: I think it's certainly doable, it bothers me a little though is that, if I'm thinking about it correctly, it will only change the situation quite infrequently.

I think that there will be many situations where a deposit does not change our offer. The variables to_cancel, to_announce will both be [] and the daemon will not send any message on the message channel. In the case where it does increase the balance of our biggest mixdepth, then we get a new offer to announce on the message channel, but it being only a 20% bump in size may not be that frequent.

So on balance, sure I can add that logic, but it seems like the timing might be the more important thing.

Amount randomization does not help here at all.

I take your main point (that the attacker is filtering inbound utxos by coinjoin activity in the future). But I do think it's more complex.

One complicating element is the extent to which other makers are changing their offer (or making a new one) in the same time interval, but not coming out of a coinjoin (those offers are kinda directly tyable to that coinjoin event, so can be discarded) - on a timescale of an hours this should be very likely, but on a timescale of minutes, probably not. With both amounts and timings randomized, as long as it's sufficient, it shouldn't be obvious (a) which block the utxo came from (b) what the amount is and (c) how to tie inbound-to-JM utxos to specific makers (not that it's a complete catastrophe if such a connection is made, anyway).

Overall I think the main thing that comes out of the discussion is to make sure it's a multi-block delay, not just some number of minutes. Adding your extra 'no reannounce if small delta', I guess I'll do that too.

[kristapsk]

Just got crazy new idea - how about every privacyenhanced maker changing and reannouncing their offers after each new mined Bitcoin block by default, regardless of coinjoin activity?

[AdamISZ]

Yeah I had a similar thought, variants on this idea are certainly viable, but more work.

[AdamISZ]

One limitation with "everyone re-announces regularly" is the bandwidth limitation. But, for reasonable sizes, as long as the frequency isn't very high (let's say it's per block, so 1 per 10 minutes on average, but not more frequent than that), then it should be no problem at all.

Another consideration is, doesn't this too heavily rely on randomness to be effective. If reannouncements of offers vary randomly in a fixed 20% range, they can reasonably be inferred to be not based on new coins. But if in a sequence of 1 per 20 minutes reannouncements it goes 10.0, 11.2, 10.9, 10.4, 11.4, 10.6, 11.1,10.1,15, 15.6 ... then it's pretty obvious what happened right?

I think the unfortunate reality is that the only thing obfuscating that a deposit has been made is:

• Not reannouncing - because it's not in the biggest mixdepth and doesn't create the biggest mixdepth (or, as today, the software just ignores it!)
• Reannouncing but on a timescale that mixes it in with other announcers making it look like you took part in a CJ
• You restart the bot so that it's not obviously the same (but: fidelity bonds, and relevant to next point to)
• The deposit is within the randomness range like 20% that we already were using (but which arguably, ironically, is not worth much nowadays if you use a FB). Then indeed it's fine but we can hardly expect people to mostly deposit only sub 20% of their largest mixdepth.

[kristapsk]

I was thinking about that bandwidth limitation yesterday too. In rare cases you can have something like three blocks in a less than a minute. That could be a problem.

[AdamISZ]

OK. Looks like we're all agreed that probably the best is for everyone to reannounce regularly, once per block (but we have to remember it isn't a panacea, as per second paragraph of above). Then we have a burst issue (as to be fair, we already have, with the responses to !fill). The IRC server won't much appreciate a pubmsg from every maker in the pit at the same time. We could put a random delay of a few seconds in there and that's probably fine.

It's not a particularly trivial change though so I'd like to hear other reviewers' opinions before doing it.

(Also if people end up switching to #1000 it's the type of thing that works better in that model (our directory nodes will have no specific aversion to bursty traffic, as opposed to traffic in general, though we might at some point need to have banscores, chaumian cash or fidelity bond checks or whatever, but that's down the line).)

[AdamISZ]

If this was obviously added for the reason of not bursting the IRC servers with messages, a quarter of an hour seems very reserved, especially given that the unthrottled onion channels have now been integrated in the meantime, so that this artificial delay is no longer needed seems to? Via the onion channel, every maker can easily re-announce their offer after each individual block?

Probably, at least I hope this will prove reasonable. I realise this is a somewhat desired feature, but given that it's not trivial as discussed ad nauseam in this thread, I think we're better off waiting a bit, seeing the onion message channel thing stabilize after release, and then coming back and seeing if it's reasonable to add this in, with something like 'reannounce after every block', probably.