Misc externally-visible cleanups (nothing urgent)

[rustyrussell]

(Based on #3843 see last 3 commits)

This adds some more info to our JSON outputs, and pushes transactions from sendpsbt into the wallet (otherwise they're not noticed, which confuses various tests once we port the other things onto it).

We could argue that signpsbt is where we should notify the wallet (and maybe, bump reservation on the inputs?), since they might not use sendpsbt through us, though we'll notice the tx when it's onchain. @niftynei @cdecker thoughts?

[ZmnSCPxj]

We could argue that signpsbt is where we should notify the wallet (and maybe, bump reservation on the inputs?)

+1 on bump reservations, but what should signpsbt notify to the wallet?

If you mean "move utxo from state 'reserved' to state 'spent'", I think neither signpsbt nor sendpsbt should do that, we should move that state on confirmation, especially if we support RBF now.

If you mean "look at the outputs of this transaction and add those we own as utxos" I think neither signpsbt nor sendpsbt should do this too, since if the transaction is later RBFed we cannot reliably determine if an unconfirmed UTXO can be spent: even if you broadcast a later version, there is an edge-case race condition where the older version is what miners mine before your replcement transaction reaches them.

We should really think more about the ramifications of supporting RBF.

[rustyrussell]

We could argue that signpsbt is where we should notify the wallet (and maybe, bump reservation on the inputs?)

+1 on bump reservations, but what should signpsbt notify to the wallet?

Currently, we tell the wallet about unconfirmed transactions. This is useful for spending unconfirmed outputs, and so users can
see their spends, but technically not a requirement.

If you mean "move utxo from state 'reserved' to state 'spent'", I think neither signpsbt nor sendpsbt should do that, we should move that state on confirmation, especially if we support RBF now.

No, later patches change that to happen only when we see the tx in the blockchain, as you say.

If you mean "look at the outputs of this transaction and add those we own as utxos" I think neither signpsbt nor sendpsbt should do this too, since if the transaction is later RBFed we cannot reliably determine if an unconfirmed UTXO can be spent: even if you broadcast a later version, there is an edge-case race condition where the older version is what miners mine before your replcement transaction reaches them.

We should really think more about the ramifications of supporting RBF.

Well, I have thought about it, and I agree!

[ZmnSCPxj]

This is useful for spending unconfirmed outputs, and so users can see their spends, but technically not a requirement.

Which is why blanket-switching to "RBF EVERYTHING!!!!" is a bit dangerous IMO. Users have been used to C-lightning allowing spending from unconfirmed transactions generated by C-lightning itself (though we do not support spending from unconfirmed transactions created by anyone else: we do not maintain a mempool). No longer allowing spending from unconfirmed change is arguably a feature degradation, and implies that withdraw and fundchannel cannot be built on top of signpsbt/sendpsbt without loss of this feature.

But even if we do not go "RBF EVERYTHING!!!!", if we have a PSBT that gets merged with a PSBT of somebody else, then the somebody else can double-spend the inputs and make the merged signed transaction invalid. Which means that, RBF or not. we cannot depend on change outputs existing until confirmed deeply. sigh.

Perhaps what we can do would be to add yet another API, gatherpsbt:

• Accepts a psbt.
• If the psbt is not completely signed (and on elements, have a fee output), successfully do nothing.
• If all inputs of the psbt are solely owned by this node then:
  • Go through all the outputs of the psbt.
    • If it goes to an address we control, add it to the utxo set as an unconfirmed change coin.
  • Permanently mark the input utxos of the psbt as never being able to be reserved again (reserved_til=UINT64_MAX), it can only be reserved or spent.

Then, near the last step of multiwthdraw / multifundchannel would be, after signpsbt and before sendpsbt, a gatherpsbt.

This would let change outputs from withdraw / fundchannel to be spendable even unconfirmed, even as we depreciate txprepare/txdiscard/txsend, and should be safe, assuming utxos they spend are deeply anchored in the blockchain, or are under.the node sole control AND are on a transaction with the same property on all its inputs.

(txsend thin wrapper should probably include gatherpsbt).

PSBTs that are merged which spend coins owned by anyone else will be rejected by gatherpsbt, as the outputs could be double-spent.

Basically, gatherpsbt is a promixe by the caller that it will never double-spend the transaction ever.

[ZmnSCPxj]

Affected User Story

You have a C-Lightning node without any onchain funds, and some funds in some other wallet you control.
You send a single large chunk of money to your C-Lightning node.

Using this chunk of money, you create a new channel by fundchannel.

fundchannel returns as soon as the channel fundincg transaction is broadcasted, but before it is confirmed.
You find at this point that miners, due to variance, have not managed to extend the chain, and that this condition persists for an hour or more.

Now, you want to create yet another channel to a different node.
As the previous transaction is unconfirmed, you cannot normally use the change of the previous transaction yet.
However, you know of the minconf argument, and thus do fundchannel with minconf=0.

A little while later, while miners are still not able to extend the chain and thus none of the transactions can be confirmed, you find out that you forgot to pay some onchain obligation.
So, you now do a withdraw to pay this onchain obligation, again using minconf=0 to use the previous change.

Then, a little while later, even while all those transactions are still unconfirmed, a friend requests a favor.
Your friend is launching its 100-Bitcoin business tomorrow, and it is badly in need of any incoming Lightning capacity it can get before the launch.
As you have some incoming capacity yourself, you can viably create a channel to it, and potentially earn routing fees for them.
Even if the funding transaction confirmation is delayed, as the launch will be tomorrow, by then your new channel will be deeply confirmed enough to be useful to your friend as it launches the new business tomorrow.
So you do yet another fundchannel with minconf=0 to use the previous change.

At this point, you realize that the onchain funds being held by the C-Lightning node will no longer be usefully utilized, so you decide to put all remaining funds into cold storage.
So you issue a withdraw command to the cold storage, with amount=all and minconf=0 in order to put all the remaining unconfirmed change output to your cold storage.

[rustyrussell]

Affected User Story

You have a C-Lightning node without any onchain funds, and some funds in some other wallet you control.
You send a single large chunk of money to your C-Lightning node.

Using this chunk of money, you create a new channel by fundchannel.

fundchannel returns as soon as the channel fundincg transaction is broadcasted, but before it is confirmed.
You find at this point that miners, due to variance, have not managed to extend the chain, and that this condition persists for an hour or more.

Old code: fundchannel tx is committed, we will never doublespend it.
New code: reservation is for ~12 hours (though I'm considering a variant where we bump reservation on signing, so 24 hours).

Now, you want to create yet another channel to a different node.
As the previous transaction is unconfirmed, you cannot normally use the change of the previous transaction yet.
However, you know of the minconf argument, and thus do fundchannel with minconf=0.

Old code: will spend change.
New code: will spend change.

A little while later, while miners are still not able to extend the chain and thus none of the transactions can be confirmed, you find out that you forgot to pay some onchain obligation.
So, you now do a withdraw to pay this onchain obligation, again using minconf=0 to use the previous change.

Old code: will spend change.
New code: will spend change, unless transaction still not confirmed after 12/24 hrs, then will double-spend and invalidate channel opens.

Then, a little while later, even while all those transactions are still unconfirmed, a friend requests a favor.
Your friend is launching its 100-Bitcoin business tomorrow, and it is badly in need of any incoming Lightning capacity it can get before the launch.
As you have some incoming capacity yourself, you can viably create a channel to it, and potentially earn routing fees for them.
Even if the funding transaction confirmation is delayed, as the launch will be tomorrow, by then your new channel will be deeply confirmed enough to be useful to your friend as it launches the new business tomorrow.
So you do yet another fundchannel with minconf=0 to use the previous change.

Old code: will spend change.
New code: will spend change, unless transaction still not confirmed after 12/24 hrs, then will double-spend and invalidate channel opens.

At this point, you realize that the onchain funds being held by the C-Lightning node will no longer be usefully utilized, so you decide to put all remaining funds into cold storage.
So you issue a withdraw command to the cold storage, with amount=all and minconf=0 in order to put all the remaining unconfirmed change output to your cold storage.

Old code: will spend change.
New code: will spend change, unless transaction still not confirmed after 12/24 hrs, then will double-spend and invalidate channel opens.

IOW, we should set the reservation should be long enough that this doesn't happen. But the ability to RBF is critical in modern bitcoin for fee efficiency.

[rustyrussell]

Basically, gatherpsbt is a promixe by the caller that it will never double-spend the transaction ever.

So, with this series sendpsbt kind of does this, but there are definitely future cases where you won't sendpsbt (dual funding in particular). There are two things here:

1. Extending the reservation now we're pretty sure this should be spent (I don't think indefinitely, at some point you should give up).
2. Marking the outputs (minconf=0) spendable.

I'm tempted to say we should leave the plugin to do #1 itself (perhaps add an blocks arg to reserveinputs?), and provide a new "unconfirmedpsbt" call to do the second one?

But we should also have code to detect that an unconfirmedpsbt is double-spent, and unreserve its inputs. We don't have that...

[ZmnSCPxj]

Indeed, but we need to be very careful about how we expose RBF to users.

I had some plans for RBF in the past, although only in the context of a new onchain-only wallet. Basically, the wallet maintains a set of "obligations", which are commands like "send x satoshi to y address", "cpfp incoming x satoshi from our address y", and periodically the wallet checks the blockchain and mempool for transacfions that it knows fulfill obligations. If it finds an obligation that is not fulfilled by any of the blockchain and mempool transactions, it looks for any mempool transaction it controls unilaterally, then mutates it to add unfulfilled obligations, and rebroadcasts it automatically with an RBFed transaction, then records that the transaction fulfills the obligations it added. If there is no such transaction, it creates a new one out of any coins it has control over.

This is a fairly high-level system, however, and it is not clear to me if PSBT-usage would be compatible with such a system (such a system can be impemented on top of PSBTs, but I am uncertain if it can support PSBTs itself; need more time to think of what we can do if a transaction with an obligation can be RBFed, or how we handle things if we want to make a PSBT that is an RBF of an existing transaction).

And if we allow obligations to be commands like "cpfp incoming funds", then we have to consider as well what we can do if there are multiple obligations, some of which are of the form "send funds" and others of the form "cpfp incoming funds" and then what happens if the sender of the funds to be cpfped RBFs the transaction from under us, and also what happens if we are sending out funds and the receiver cpfps it and now we want to RBF our own transaction and now we have to increase our fees enough to also evict the receiver CPFP transaction built on top. This means we need to start monitoring the mempool, which lightningd does not do now, at all, except implicitly via geutxout / bitcoin-cli gettxout, as well. And then this goes around and hurts #3858, particularly for BIP158 (Electrum SPV backends trust the mempool reported by the server, but a BIP158 SPV fullnode does not want to maintain a full UTXO set and does not wan to maintain a full mempool, so checking mempool becomes really really hard). Sigh.

[ZmnSCPxj]

So, with this series sendpsbt kind of does this, but there are definitely future cases where you won't sendpsbt (dual funding in particular)

We need to make either a separate sendpsbtwithoutdetectingchangeoutputs or add a parameter to sendpsbt that does not do this autodetection of change outputs. The backend can be changed, so plugins implementing dual-funding should perform transaction broadcasts via lightningd.

In particular, #3858 (comment) suggests to replace sendrawtransaction with sendpsbttomempool, because a BIP158 backend might want to maintain only a "local" mempool containing transactions we broadcast, and has to know if a new block has a transaction that double-spends a mempool transaction. But v1 backend plugins only have sendrawtransaction, so the lightningd frontend has to wrap the details of the backend plugin (meaning it has to provide a different command that is not sendrawtransaction, because lightningd has to be the one to handle backend v1 vs backend v2).

[cdecker]

Rebased on top of master now that #3843 was merged.

[cdecker]

@rustyrussell @ZmnSCPxj: Shall we move the discussion of how to properly add RBF support into a separate issue? To me it seems like the PR itself doesn't change much of the externally visible behavior and should be merged.

ACK 2ed392a

[ZmnSCPxj]

@cdecker seems ok.

[niftynei]

ACK 2ed392a