Yield generator can be launched in Qt

[AdamISZ]

A status bar widget allows access to configure,
and start/stop a yield generator (maker) bot
in the background. While running, taker functions
are disabled as well as wallet changing functions.
Other functionality is still available.

[AdamISZ]

The basic concept:
Wanting to offer market maker function to GUI-only users.
With this in mind, kept configuration options as simple as possible; yieldgen basic class is used for now.
To keep interface tidy and to reflect the fact this is a long-running service, rather than an action, use a widget in the status bar. Current icons are a red diamond and a green circle for not-running and running respectively (these are placeholders, feel free to help me choose better ones), and has a tooltip of course.
Clicking brings up a simple configuration box with the requisite parameters and a start button. Once running the same widget allows user to stop.

While running, other features are put into a disabled state: wallet load, generate and recover; the coinjoins tab and the settings tab.

Under the hood/technical: added a JM_SHUTDOWN message to the client-server communcation over AMP (not sure why I hadn't used that already). Attempted to make the shutdown clean, it seems to work fine when testing start then stop then restart. The yieldgenerator startup was slightly refactored to allow it to work without passing in an options parser object (see ygstart() and ygmain()). Other than that there is not really much change in jmclient at all.

In theory, I'm betting that it'll be very possible to actually allow Taker function while Maker is running, but don't want to think it through, seems wisest to disable it for now.
It's nice that now the wallet service arch is in, we don't have to worry about conflicting on wallet or closing reopening, also a user could fund their wallet, see the deposit in real time and have it contribute to joins without having to do any restart.

TODO:

I think it mostly needs (a) a bunch of testing and (b) some more polish in the UI, for example the MakerDialog should look different during running.
I won't go as far as adding fancy UI for things like the transaction log (yigen-statement) or history, that's too much. But as long as it's intelligible.

[chris-belcher]

Use the logger functions instead of print?

[AdamISZ]

Yeah debug leftover, mistake, sorry.

[AdamISZ]

I removed "script" from the title of the PR. That's dumb, it's not launching a separate process or something.

[AdamISZ]

A brief note on the TxHistory change in the last commit (rebased on master for the datadir option for testing):

I thought a bit about parsing transactions as they come in to see if they're coinjoins we participated, or deposits, or something strange. That's a bunch of extra code - especially if you were to do it right, i.e. fully.

So I set that aside, in line with #420 and a few comments in other places, I believe we should integrate and refactor the process of dealing with transaction history. So we currently have a history method of wallet-tool with a significantly more detailed processing, a bare bones recording in the TxHistory tab and its corresponding jm-tx-history.txt file (whose location is fixed in 0fad076), and a yigen-statement.csv to record maker coinjoins.

So the approach taken here is not to address either the UI concerns of the tab, nor the full correct transaction type accounting, but just to ensure that while running in maker mode we make an entry for each coinjoin that happens - note it doesn't bother to find the equal-size cjout, but just records the first utxo in the tx with its corresponding amount and txid, so the user can either look it up on the blockchain or look at yigen-statement.csv for more information. Deposits are also correctly recorded while the maker is running (although again - an exotic deposit with multiple utxos to this wallet will only show one, at random), as well as of course showing the updated information in the Wallet tab and the Coins tab.

Done some testing on both regtest and mainnet, seems to be quite functional.

Next job will be to change the information shown in the Maker tab while it's running to something more sensible than the starting parameters and a "Stop" button. Then I'll take off the "WIP" marker and propose it for merge.

[chris-belcher]

When you squish the commits make sure you add enough linebreak between the subject line and body. Right now git log --oneline is messed up:

0fad076 TxHistory tab in Qt records coinjoins while in maker mode, and deposits (although there is no sophisticated tx type parsing). The location of the corresponding history file is fixed to the datadir correctly. Also fixed Maker start/stop for non-Tor case.
52bf7f0 add icons directory
7d7989f Yield generator script can be launched in Qt
252a869 add datadir option to Qt script

[AdamISZ]

When you squish the commits make sure you add enough linebreak between the subject line and body.

Understood, will do.

[AdamISZ]

WIP status removed; I believe, after some more testing, this is mergeable.

I recognize that the UI choices are arguable; I personally find it elegant and easy to use; it's effectively just a button that's always available from anywhere in the UI, to switch this feature on/off.
I think the icons chosen are pretty rubbish but of course happy to change them. At least they're good enough for now; the tooltips tell the user what it means.
I'm not keen on doing a bunch more Qt coding work to put the same function somewhere else, especially because I really don't think this choice is particularly bad.
I do think that other widgets in the status bar make sense, obvious one being blockchain connectivity.

[kristapsk]

I do think that other widgets in the status bar make sense, obvious one being blockchain connectivity.

Agree on this. Another one could be a Tor indicator, where we can have three possible states - all IRC servers via Tor / some IRC servers via Tor / not using Tor at all.

[kristapsk]

Some issues I found:

• When wallet is not loaded, clicking on status bar icon should display error message about wallet not loaded instead of "Manage maker" dialog (or just hide icon before wallet is loaded);
• Icons should have transparent background instead of white, looks like this for me:
  
• Clicking some icon on status bar to start a maker isn't obvious, maybe worth duplicating this functionality in main menu (for example, "Tools > Start a maker...");
• Start-up errors of yield generator aren't handled, only printed in console, GUI pretends yg is running when it's not (noticed with [ERROR] do not have any coins left).

[chris-belcher]

I think having the yield-gen entry point as a button in the status bar is bad UI, because:

• I guess the vision behind making the yield-gen entry point so prominent is so that as many people as possible use it even if they're a bit casual about it? (e.g. running yieldgen on machines that aren't always-on, using slow machines or slow connections) If so, that may not fit with the plan of using fidelity bonds Design for improving JoinMarket's resistance to sybil attacks using fidelity bonds #371 which pretty much makes casual yield-generator operators uncompetitive (if people are locking up coins they'll want to get max possible benefit from them) . That vision also has flaws because JoinMarket's incentives are broken today with issue Duplication of offers from makers. Many makers one wallet joinmarket#693 so if we encourage more yield-gen operators without fixing it then we may end up with a bunch of sybils.
• It's separated from other coinjoin functions of single coinjoin and tumbler. Yield generator belongs with them as just another mode of operation.
• Yield generator doesnt belong on the status bar as its on-display there all the time. Yield-gen isn't of interest to users who are just recovering their wallet or using tumbler. This doesn't apply to other widgets like tor and blockchain connectivity which every JoinMarket user is interested in.

If you want to merge it anyway I won't oppose, but at least it's known that I was against this.

Regardless, I'll test a bit more shortly.

[AdamISZ]

I guess the vision behind making the yield-gen entry point so prominent is so that as many people as possible use it even if they're a bit casual about it?

Very much so, at least, a big part of it.

If so, that may not fit with the plan of using fidelity bonds #371 which pretty much makes casual yield-generator operators uncompetitive (if people are locking up coins they'll want to get max possible benefit from them) . That vision also has flaws because JoinMarket's incentives are broken today with issue JoinMarket-Org/joinmarket#693 so if we encourage more yield-gen operators without fixing it then we may end up with a bunch of sybils.

I think it's orthogonal.
If we take the fidelity bond aspect: this will have to be part of a protocol upgrade, which will inevitably involve other upgrades (native sw etc etc). It's not a given that such an upgrade will be successful; nobody has yet started using a fidelity bonds design; there are a few details about how it could or should work; we don't really have a good sense of how the community of users will react to it.
It will not happen quickly.
And ultimately, whether and when that gets implemented is orthogonal to the maker function existing in GUI.

So while 693 still exists, all we're really looking at here is a shift to a slightly broader population of people having access to the same function. A sybil attacker will use the command line anyway, it's far more convenient for that kind of user.

To get back to the first point - the reason I think making the maker function easier to use is helpful is that heterogeneous usage of Joinmarket greatly improves its obfuscation effect. I don't see how this point is at all debatable; in the absolute spherical cow case of a single Taker user and N makers with neither side ever budging from this role in any way, the obfuscation effect is more or less a crapshoot, there will be scenarios that completely re-link the starting and ending point amounts. The non-spherical cow is multiple takers, especially concurrent, and various other flies in the ointment for spies such as equal sized inputs. But imo the inability to unambiguously link the Maker or Taker role from transaction to transaction within the set is what turns Joinmarket from "kinda useful" to "pretty strong actually", which is why I think measures like this - not that this in itself is so amazing - really matter.

Meanwhile I understand that incentives are skewed to being maker, generally - as long as most people don't have urgent need to move their coins around. But here I don't think there's any logic in not giving people more optionality. The only case in which I agree with the logic of "don't let users do X" is where X has danger - obvious example, restrict printing of private keys (and yet we still have to allow even that!). This function has no danger at all, so even in cases where it's not effective (say, a user running without fidelity bonds when they are enabled and in wide use - and as per above, we are a long way from that ...), it still isn't an argument against having it.

[AdamISZ]

It's separated from other coinjoin functions of single coinjoin and tumbler. Yield generator belongs with them as just another mode of operation.

I disagree. Maker is a passive mode of operation, single and multiple join are active. But I don't disagree in a really profound way, I think this is simply arguable.

Yield generator doesnt belong on the status bar as its on-display there all the time. Yield-gen isn't of interest to users who are just recovering their wallet or using tumbler. This doesn't apply to other widgets like tor and blockchain connectivity which every JoinMarket user is interested in.

Both Tor and blockchain connection belong on the status bar, agreed there - PRs welcome etc etc :)

That's not a reason not to have this as an easy to press button from anywhere on the status bar. Having it as a toggle on/off makes sense; you can still deposit, freeze coins (and should be able to do at least non-coinjoin sends, but we'll have to check before enabling that).
The mere fact that a status bar might be representing something the user doesn't want to use is not a good reason not to have it. All I'm trying to represent is that there's free optionality here to a user any time they're not doing something - there's really no reason for them not to be running as maker, once the wallet is loaded.

[kristapsk]

@AdamISZ Do you plan to resume working on this at some point?

[AdamISZ]

@kristapsk i mean yes and no. First, I think it's a good thing to have this function in Qt (consider: as far as I was told, @openoms was even given a grant to work on a GUI for Joinmarket, specifically because this GUI doesn't support yieldgenerator; I'm sure there's more to the story that that, but still). Second, it's not really that hard.

But counter to that, I'm rather more interested in further developing #670 , combined with a javascript framework, because it would give several nicer outcomes: (1) out of the box binary cross platform distribution (2) allow client side code to be written independently of this codebase (3) a much more intuitive and reactive UI with more modern design that users are familiar with.

I did reach out to try to get people interested in that, but not so much luck so far. A couple expressed interest but not more than that.

That doesn't mean I don't want the existing GUI to continue to improve (hopefully the recent Windows one-click exe is helping some people, for example), just a question of, what should we work on.

[AdamISZ]

If you wanted to look into improving it @kristapsk , I could rebase this branch.

[kristapsk]

@AdamISZ Yeah, that would be cool. Seems like most of the job is already done, only UI/UX discussions and minor bugs. Mixing maker and taker roles is important for privacy. #670 and stuff on top of that could take long enough that it's worth getting this to the release.

[AdamISZ]

Rebased on master as of b57b70c

Note: I have not started testing this yet, and there were one or two tricky manual edits in the rebase, so I would not be amazed if there are a couple of bugs lurking, though I doubt anything serious.

As you said before, this was already operational back in January, it mostly needs some slightly better UI elements. Other than that it's probably quite viable.

[AdamISZ]

Have done a few tests and identified a couple of bugs, which are now fixed. Should be functional to use either Qt or command line yield generator now.

[openoms]

Very good to have this especially if it does not need a huge change!

Whether it will be used depends mainly on how people run their nodes. If someone has an always on (ideally dedicated) computer with a screen connected they can benefit from the ease of starting YG from the QT- GUI.

For the ones whose bitcoin node (and/or separate JoinMarket node) is a headless device (like most prebuilt/compiled/facilitated node packages) something like JoininBox is still useful to be able to run YG in the background.

[PulpCattel]

Couple nits.

[PulpCattel]

Suggested change

	self.makerDialog.close()
	def setMakerBtn(self):
	self.makerDialog.close()
	def setMakerBtn(self):

Extra blank line.

[PulpCattel]

This is just pedantic, but one tooltip ends with '.' the other does not. Since they are "symmetrical" it would be nice to have them consistent.

Update:
I'm testing this out a little bit, is there a reason why the first tooltip at init is Click to start maker but then changes to Maker not running: click to manage.? Shouldn't the tooltips be the same?
Seems like conceptually there should be no difference in the tooltip between first time and after maker stops.

m2c are: Click to start maker is better, Maker not running: click to manage seems more a message for e.g., when a maker should be running but it is not, not for when maker has been directly stopped.

[PulpCattel]

Suggested change

	TODO consolidate the multiple tx history functions.
	"""
	if not self.maker_running:
	TODO consolidate the multiple tx history functions.
	"""
	if not self.maker_running:

Extra blank line.

[kristapsk]

"Minimum CJ size (sats)" should be changed to 100000, which is default for cli since #570.

[kristapsk]

But in context of #727, should be read from the config in future, not being hardcoded here.

[kristapsk]

All sats inputs should now use BitcoinAmountEdit, change int to amount here and implement it below.

[kristapsk]

and should be able to do at least non-coinjoin sends, but we'll have to check before enabling that

All taker single sends (but not tumbler) could be freely allowed in GUI while maker is running. As taker can source coins only from one mixdepth at a time, just need to communicate to maker to not use ("mix from") certain mixdepth in moments when user is in a single send dialog. And also make sure for taker to not use himself as a cj counterparty.

[AdamISZ]

Yes, for sure it's possible. But since it's meaningful extra work, I didn't want to include it here.

[PulpCattel]

I tested this out a bit and I've managed to do maker CoinJoins through the GUI with the regtest setup (using a custom ygrunner that creates an extra seed).
I really like the maker functionalities into the GUI; seems natural for the QT to support both and this is a critical point:

Mixing maker and taker roles is important for privacy.

A couple of thoughts from using this:

• Could be nice to add a couple more extra icons for e.g., when a maker CoinJoin is happening and when there's some problem ongoing (IRC is disconnected, all coins are frozen, etc...)
  IDK if these are low enough hanging fruits, but would be useful.

• Since there are no icons for the "CoinJoin happening" status and no warning messages, I could imagine that this could, ceteris paribus, increase a tiny (?) bit the number of
  disconnection from makers while CoinJoins are been coordinated. QT users, without any explicit indication, could stop or close the QT without much thought about the console output.
  (It's also a bit weird that the "mirrored" console output in the CoinJoin tab, being greyed out, becomes really easy to miss and harder to follow in maker mode)
  Ideally would be nice to have a popup/warning that alerts you that you are closing the application in the middle of a mix. Wasabi for example does that.

[AdamISZ]

Yeah those are some good thoughts. The icons were clearly not in a ready state, they were more placeholders than anything.
Your other point about warning that join is in progress makes sense, this is linked to the more general need for better clarity with mode of operation. I'd probably think (a) status bar update/icon with more info as you suggest, and (b) a warning popup when close() is called, would cover us there.

About the log output that's in the greyed out coinjoin tab, I probably toyed with the idea of putting the yg log output in a similar box in the maker dialog, but put it to one side for now.

[kristapsk]

About "mode of operation" - there should be third status when tumbler is running, so you have three icons - default mode, maker is active (running), tumbler is running.