[ghost]

openingd/json_fund_channel:
- result fundchannel command now depends on successful or failed broadcast of the funding tx
- failure returns error code FUNDING_BROADCAST_FAIL
- don't fail the channel when broadcast failed, but keep in CHANNELD_AWAITING_LOCKIN
- after fixing the initial broadcast failure, the user could manually rebroadcast the tx and
keep the channel

openingd/opening_funder_finished:
- broadcast_tx callback function now handles both success and failure

jsonrpc: added error code FUNDING_BROADCAST_FAIL
manpage: added error code returned by fundchannel command

This makes the user more aware of broadcast failure, so it hopefully doesn't
try to broadcast new tx's that depend on its change_outputs. Some users have reported (see
issue #2171) a whole sequence of fundings failing, because each funding was using the change
output of the previous one, which would not confirm.

[ghost]

I think this PR is enough for now.

Maybe further improvements could be:

• Make the fundchannel command atomic (is that the correct word here?), i.e. channel is forgotten when broadcast fails, together with all state introduced by it (in wallet, watches, tx_filter etc) ... but this sound like a lot of work.
• Introduce new output states in the wallet db, like: output_state_available_awaiting_mempool and output_state_spent_awaiting_mempool. New change_outputs created-by, or spent-by funding tx's are then marked into these intermediate states (so they are not used for other tx's) until successful broadcast.

[rustyrussell]

OK, rebased this on top of a different fix, and changed the test. Still needs a couple of cosmetic changes...

[cdecker]

Really like the way this is shaping up. It gives more control to the user. We might want to go a step further and move the following things into funding_broadcast_success as well:

lightning/lightningd/opening_control.c

Lines 408 to 409 in f96cdd9

	/* Extract the change output and add it to the DB */
	wallet_extract_owned_outputs(ld->wallet, fundingtx, NULL, &change_satoshi);

lightning/lightningd/opening_control.c

Line 425 in f96cdd9

wallet_confirm_utxos(ld->wallet, fc->wtx.utxos);

The rationale behind this is that some users have reported a whole sequence of fundings failing, because each funding was using the change output of the previous one, which would not confirm. If we move those two parts into the success callback we avoid adding outputs that are unlikely to confirm, and we also delay marking the reserved outputs as spent (which is something that we might want to move to the output confirmation path anyway, instead of the funding path), making it easier to reset them should the funding fail indefinitely.

Should the funding eventually confirm wallet_extract_owned_outputs will be called anyway when the tx gets confirmed, we just call it to avoid people asking where their funds went.

[rustyrussell]

Should the funding eventually confirm wallet_extract_owned_outputs will be called anyway when the tx gets confirmed, we just call it to avoid people asking where their funds went.

Let's make sure that this works if we die between telling bitcoind to add the tx to the mempool and getting the response. As far as I can tell, we'll then try to doublespend the output, which will fail: do we catch that case correctly if it's a wallet withdraw?

IOW, I think that's a different change, which we should definitely do as a separate PR.

[ghost]

Hmmm, hope I did this correctly ... rebased my local branch on 37c276d then --force pushed it back

[cdecker]

Let's make sure that this works if we die between telling bitcoind to add the tx to the mempool and getting the response. As far as I can tell, we'll then try to doublespend the output, which will fail: do we catch that case correctly if it's a wallet withdraw?

The UTXO is set to reserved after selecting it, and before passing it to openingd, and the confirm call just goes from reserved to spent (which is a useless state anyway since we should really be tracking confirmations). Selection ignores both reserved and spent outputs, so that shouldn't cause us to double spend.

If we don't manually add the funding TX, and therefore the change output, we will grab it from the block once the funding confirms, so this really is just an optimization.

I'm happy to move this to another PR, if we want to discuss it further, and unblock this PR 😉

[cdecker]

Hmmm, hope I did this correctly ... rebased my local branch on 37c276d then --force pushed it back

Seems that you rebased on something that was not master which can pick up some additional commits when comparing to master afterwards. No problem though since it'll automatically be solved once the extra commits are also merged into master.

[ghost]

Really like the way this is shaping up. It gives more control to the user. We might want to go a step further and move the following things into funding_broadcast_success as well:

We could indeed make it further fail/fool-proof, but I don't see a way to do that without deeper modifications and new infrastructure.

Let's make sure that this works if we die between telling bitcoind to add the tx to the mempool and getting the response.

Indeed something for a new PR.

[rustyrussell]

Ack 0aee7fc