[ghost]

WIP: This is a follow up of #2274. In addition to returning an error when a json_withdraw or json_fundchannel broadcast failed, it now also prohibits users from creating new dependent wallet transactions. Not sure how often this happens and by what cause, but this PR protects users from creating more potentially stuck transactions (#2300, #2171).

json_withdraw and json_fundchannel now use similar procedure:

• Owned change outputs are marked as _reserved until successful broadcast
  OR until confirmation in block
• Owned outputs consumed by tx are marked as _spent before broadcast
• UTXO/coin selection fails when any output in state _reserved

In case broadcast failed:

1. user should fix the problem that caused failure (bitcoind dead?)
2. user should manually rebroadcast, for example by copy-paste rawtx from log and call
   bitcoin-cli sendrawtransaction <rawtx> and wait for confirmation. At confirmation,
   owned_txfilter will detect it and update owned output from _reserved to _available.
3. continue normal operation

Considerations:

• Assume that, once tx is signed and valid, it may/will be (manually) broadcast.
• That makes cleanly aborting a fundchannel very hard, so better go ahead.
• If user really wants to abort, it should use dev-rescan and dev-forget-channel
  idea: maybe a mutual closing_tx can be negotiated and stored as backup without any broadcast?

TODO:

• make listfunds show outputs in _reserved also, so users see funds after a failed broadcast
• json_withdraw and json_fundchannel should return same error code in case of failed broadcast
• same for json_close ?
• add some test vectors to make sure our outputs are eventually picked up when broadcast
  manually

[cdecker]

I'm really tempted to just have UTXO selection code ignore unconfirmed (confirmation_height is NULL). That way we don't pick up stuff that results from failed broadcasts, with the downside of forcing users with few outputs to wait for confirmation before opening new channels (which is a small price to pay for a more stable solution imho).

[ghost]

The aim here is also to force the user to solve a failed broadcast before creating any new ones. The presence of an output in _reserved state act as a flag that tx is not (yet) successfully broadcast.

I'm really tempted to just have UTXO selection code ignore unconfirmed (confirmation_height is NULL).

Agreed, that it is simpler and robust and prevents user creating txs dependent on failed ones. But they can still create new txs until it runs out of confirmed outputs. In another test I tried to use a global wallet lock (in vars table in db) that is set after a failed broadcast, but couldn't find easy condition to unset it.

Thanks for the quick response, first I must say this exercise was a learning experience for me (took me quite some time to understand all the filters and watches).

EDIT: Consider this single commit as proof-of-concept. I can split it up in smaller chunks if desired, but will first wait for a Concept ACK/NACK.

[ghost]

Split up into multiple commits, improved comments, same semantics.

[ghost]

After slept over this a few nights, perhaps the added complexity is not worth the small benefits. Using _reserved outputs states as a general flag for previous broadcast failure also doesn't work when that tx didn't have any change outputs. So I will close this PR.

Hopefully users are alerted enough by the returned errors introduced in #2274.