Add "manual" tx_broadcast method

[kristapsk]

Ensures that no attempts to broadcast transaction will be made by JoinMarket. This is needed for Lightning channel opening with c-lightning and it's fundchannel_start and fundchannel_complete commands.

Note that the funding transaction MUST NOT be broadcast until after
channel establishment has been successfully completed by running
fundchannel_complete, as the commitment transactions for this channel
are not secured until the complete command succeeds. Broadcasting
transaction before that can lead to unrecoverable loss of funds.

I plan to write scripts to automate channel opening/closing in the future, but this already allows to do it manually.

[chris-belcher]

I've reviewed the code and it looks fine to me.

How long might a coinjoin transaction be un-broadcast using this command?

[kristapsk]

Depends on a user. Workflow is to call lightning-cli fundchannel_start, which returns destination address. Then you do cj, then call lightning-cli fundchannel_complete, to which you give lightning node id, txid and output index as a parameters. When that command completes (should be seconds), you broadcast transaction (or call lightning-cli fundchannel_cancel and never broadcast it, if you changed your mind :)).

[kristapsk]

There is minor issue with qt, I will look at it. Just a message in output, otherwise it works. Tx does not appear in tx history also, but that's likely solved with #359.

Traceback (most recent call last):
  File "joinmarket-qt.py", line 630, in startSingle
    self.persistTxToHistory(destaddr, self.direct_send_amount, txid)
AttributeError: 'SpendTab' object has no attribute 'direct_send_amount'

[kristapsk]

I think I will not change anything here currently. Error happens because accept_callback is not called, I could hack around here with that, but I don't think it is right to add tx to history tab if we don't actually broadcast it ourselves. And it does not cause any other problems, just the message in console output. When #359 will be merged, as we will have auto-refresh for GUI, the right thing would be that it is also responsible for adding entries to history tab. It would also solve issue that user would except tx to appear in history if it's send using other wallet where user might have imported individual privkeys from JM wallet (or somebody might have stolen them). The same issue would be with "not-self" tx broadcast method when re-implemented (#368).

[kristapsk]

jmprint import was missing in taker.py, fixed.

[AdamISZ]

Having read through this, my comments:
Technically I can see the logic and I think the code is correct, except for the caveat that it may or may not be an issue that we have the callbacks registered just before the transaction is pushed, so if you actually push it manually via taking the tx hex and doing it somewhere else, then if you were doing serial processing (multiple coinjoins) you'd have to make sure to do it fast enough that the unconfirmed callback doesn't time out.
Clearly if you were trying to do this to make sure that a LN funding were not broadcast ahead of the correct time, the 'right' way to do that is with a callback or an inter-process communication, rather than this way; so I presume your current use case is for testing, which is fine.

In general, a manual option is useful, just it doesn't sit well with multi-join.

So for me utACK because it doesn't harm anything to add this option.

You do need to rebase though (this one should be easy).

[kristapsk]

Rebased

[AdamISZ]

Testing report: for command line sendpayment things work as expected, although it may not be obvious for users, along the lines of what I was saying above about callbacks:
sendpayment waits for the unconf callback to trigger before shutting down. This will happen, but only after the user has broadcast the transaction successfully. So the script is just hanging after showing the serialized tx hex.
This was no problem for me, and won't be for you, but if anyone uses it it may be slightly confusing. I'd tend to just let this slide, not sure what you think.

For Qt, something more important happened:
(1) It turns out the dialog box is just not big enough (those "OK" dialogs are not resizeable, either, at least as currently coded, I think it just hit max size). I copy-pasted the txhex and found it failed to broadcast, reason was that the txhex was just truncated (and this was a small transaction, too).
(2) Only cosmetic but marginally important: the message in the dialog is "Success" in the top bar, and "Tx sent: Broadcast manually:" before the txhex in the main dialog body. Obviously this isn't quite right.

I'd be tempted to suggest you change it to tell users not to use this option in Qt, rather than try to support it; but either way is fine with me; if you do want Qt support you'll have to look at the dialog box size issue. Istr i dealt with that already, for the commitments failure case, so I'd take a look at that dialog box, it's different somehow (I don't remember).

[AdamISZ]

Just to follow up on that last point, since I was curious:
The infoDirectSend callback uses JMQTMessageBox just like everything else; I think that all that's needed (if you want to do it) is to add some \n judiciously to get it to wrap. There is actually a 'detailed_text' feature in JMQTMessageBox but it is not currently being used; I think I discovered I couldn't get a "huge text box" to actually work, so just have to use \n. I think.

[kristapsk]

IIRC, I didn't have such problem when copy-pasted tx data from that dialog, could be related to the different Linux GUI's in our systems.

I think it will be good enough to just not allow tx_broadcast = manual for the GUI, at least for now. You will be using c-lightning in command line for this anyway.

[AdamISZ]

That's fine with me, just make sure users don't use it on GUI.

Tbh the more I look at this the less I like it, whenever we add options like this it makes it a little more complex to handle updates, and this one in particular doesn't fit well with most usage modes (multi-join definitely, single join it works but it behaves a little awkwardly). I think it's fine to merge it just for single join command line, though, if you have a strong enough need for it.

[kristapsk]

The end goal for me would be to have some script you could copy to JM scripts folder and use that for opening Lightning channels with JM CJ. It will most likely don't need this, this was just a way how to do it manually right now. Proper script would need some hook before tx broadcast, with ability to cancel broadcast, without caring much what is actual tx broadcast method afterwards. But haven't looked much into details yet.

Another thing - this was just a fun thing I wanted to do, but you don't gain much privacy currently as LN channel opening TX output is P2WPSH, not P2SH, so is distinguishable. And you can in most cases easy find out also inputs and change output of the taker, because of the cj fees.

So, let's not merge this in at least for now.

[kristapsk]

I'm closing this.

1. Returning PSBTs will be a better way to achive the same result. Original reason for me to code this was c-lightning's fundchannel_start / fundchannel_complete, which now changed from txid to PSBT.

2. Probably adding support of different broadcast method than Bitcoin node in generic would be useful in future, see https://github.com/sgeisler/btcbc-rs, for example.

[kristapsk]

2. Probably adding support of different broadcast method than Bitcoin node in generic would be useful in future, see https://github.com/sgeisler/btcbc-rs, for example.

https://github.com/alfred-hodler/pushtx