Feature: `txmodifypayee` a `txprepare`d transaction

[ZmnSCPxj]

While studying the existing fundchannel in preparation for fully implementing #1936 , I noticed that the current fundchannel does this:

• connect to peer.
• txprepare a "dry-run" fundchannel using a dummy funding address.
  • Extract the funding output amount in the returned transaction --- presumably this is how we determine the actual amount when given all.
• fundchannel_start to peer using the extracted amount, receiving a target funding address
• txdiscard the previous "dry-run" transaction.
• txprepare, this time with the now-filled-in funding address.
• fundchannel_complete with the transaction ID.
• txsend the transaction.

What I want to point is that in theory, it would be possible for a sufficiently heavily-used server to have some other withdraw, fundchannel, or txprepare execute between the txdiscard of the dry-run transaction and the subsequent txprepare as a multithreaded race condition.

As locking of the RPC is not implemented (and would be dangerous as well --- consider that a plugin can easily deadlock itself with an incorrect lock behavior), we might instead want to have an atomic operation that combines a txdiscard with a txprepare that uses the exact same inputs and outputs, just redirects the address of an output.

So let me propose:

txmodifypayee txid modifications

Where modifications is an array containing objects with fields:

{
    'outnum' : 0,
    'address': "3LXPWUpTGKmeatx6NpyQ8A8TZr2GCXkK4u"
}

The address would have to take up the same amount of space as the current address at that outnum.

The command succeeds or fails "atomically". If it succeeds, then the old txid is no longer passable to txdiscard or txsend. If it fails, then the old txid can still be passed to txdiscard or txsend.

Thoughts @niftynei @cdecker @rustyrussell ?

[ZmnSCPxj]

Another potential race condition is that if we are using all for the amount but not indicating the feerate, then in the first txprepare we are able to get a specific amount for all which we use for fundchannel_start, then feerate estimation changes to become higher, then the redone txprepare (which will use an exact amount instead of all, extracting the amount from the dryrun txprepare) may fail due to lack of funds.

[niftynei]

we could take a page from the Javadoc convention of merely labeling an API "not thread safe"; i.e. we add this language to the fundchannel documentation. hehehe. dusts hands off

there's a few problems here that we can tease apart.

• first, we need to know "how much money is all the money" so that we can continue to support the 'all' keyword for fundchannel. the key issue with 'all' as it stands is that we need to know the amount of sats for the funding tx in order to communicate this to the peer in fundchannel_start, however we don't know the output address for the funding transaction until after fundchannel_start is completed.
• second, we need a way to mark utxos as reserved, or at least a way to update or delay specifying the output script for a composed transaction.

Here are some options:

• deprecate the 'all' keyword for fundchannel. this will allow us to remove the 'dry-run' attempt with txprepare, as we can wait until we've gotten the funding output's script before calling txprepare. this fixes the race condition by having txprepare only called once, with all valid information.
• add an RPC call which, given a feerate and estimated transaction size, will return the value of 'all' which we then use for fundchannel_start and, then, txprepare (which we call with the specified feerate). we may even be able to use the existing RPC calls listfunds and feerates to patch this data together today, without modifying c-lightning. this fixes the problem by allowing us to call txprepare once, with all valid information.
• add an RPC call which allows for updating an in-flight tx, txmodify. ideally this would encompass changing the feerate as well as altering the outputs; i.e. drop this output, add this other one. this fixes the problem by allowing us to call txprepare once upfront, and then updating with valid output information after we've received it (while keeping the feerate the same)

As it is, I like the third option the best. The first two solve the race problem by allowing us to delay reserving the funds until after fundchannel_start is called. In my opinion, this is sub-ideal as it allows us to initiate with a peer before confirming that we can cover the requested amounts from our wallet.

[ZmnSCPxj]

1 avoids the problem but brings back #3402, thus not a solution. 2 does not avoid the problem: Between the time we get all and the time that we actually txprepare, onchain funds (and thus the value of all) could be modified by a separate process talking to lightningd, which is the root race problem anyway.

[ZmnSCPxj]

In #3763 (review) @cdecker mentions concern about us handling transactions that can be extracted from db, sent to our hsmd, then broadcast.

Note that the same thing already exists in current fundchannel.

What we could do would be to add an nLockTime argument to txprepare. Then for "dummy" transactions which are not intended to be actually broadcast, use a far-future date for nLockTime. That allows us to create a transaction that is still validly parseable by libwally and other support code, but which is not practically broadcastable, or at least, we are likely to be either nonexistent, or vastly different sentiences by the time they are broadcastable.

(The suggestion to add an invalid input leads to either overpaying fees, or fudging around the fee computation code; I think a far-future nLockTime is a practical prevention of accidentally broadcasting a dummy tx; in theory we would never make that mistake anyway, so a practical way to block a mistake that could occur in practice should be sufficient.)

What do you think @cdecker ?

[cdecker]

Ah, I forgot about the fee computation, but maybe we could set the prevout_index for one of the inputs (or all) to maxint. That'd still have the desired effect of reserving the outputs using txprepare but make the returned tx unusable. The timelock solution is a close second, but if we can avoid handling dangerous transactions altogether we should probably go that route instead.

[ZmnSCPxj]

We could split up txprepare even further, to a txreserve that reserves some amount(s) and supports all (so that all computation and coin reservation are atomic), and a txpreparereserved which creates a transaction that uses the reserved coins.

txreserve would return a UUID (could be just a hex representation of a 64-bit number) which is then used by txpreparereserved to actually prepare the funds.

So txreserve might accept an object of stringID-amounts (key is a string unique within the object, value is anything parsable by param_amount_sat_or_all). Then it returns a reservation_code (the aforementioned UUID) and a corresponding object of stringID
amounts, plus a separate field of change with a struct amount_sat.

Then a later txpreparereserved accepts the reservation_code and a mapping from the old stringID to actual addresses, and returns txid and tx, with the txid being something that can later be txsend or txdiscard.

How does that sound?

The lifetime of a reservation would be indefinite, but only while the lightningd survives. Not sure how to handle plugins dying before they can free up reserved coins though. Sigh. Maybe txreserve can also accept a process_id, which lightningd will poll, and if that process ID is no longer running, lightningd automatically unreserves the reserved output? Object lifetimes across process boundaries how do they work LOL

Alternately we can make the lifetime of reservations (and unbroadcasted transactions) the lifetime of the RPC connection that initially did the txreserve. That should let us work even if we somehow expose the RPC remotely as well. Drawback is you cannot then use txreserve via lightning-cli, because lightning-cli closes the connection as soon as possible. Object lifetimes....

Because txreserve would not yet know what the actual addresses are, we would assume the largest scriptPubKey, which I think is P2SH (or is it P2WSH?). This gives suboptimal fees if we later put a P2KH or P2WKH address. Or maybe we can add a mapping of stringID to expected address type as well.

[niftynei]

The RPC calls in #3775 should help with this, particularly reserveinputs and unreserveinputs.

As noted, these reservations last for the duration of the node's runtime (i.e. restarting the node will remove all pending reservations). There's a series of commits that moves to a time based reservation system in #3418 that should more amicably resolve this problem.

You'll still have the 'need to attach an output for an unknown script' problem with reserveinputs.

[ZmnSCPxj]

The point as I understood it was to specifically not handle a valid transaction that could be broadcast with scriptPubKeys that would not allow the owner to recover funds. It looks like reserveinputs constructs a transaction, which is what we would like to avoid in the first place. It would be nice if we could do reservations without a transaction being created, just reserve some utxos.

For example, instead of reserveinputs accepting a list of addr-amount pairs, maybe it should just get a list of amounts, or a single-entry list with the string "all". Then it does coin selection, returning:

• A list of reserved utxos.
• A reservation code, which is used later to transform the set of reserved utxos into an input of the transaction.
• The total amount.
• The sipa-weight of the inputs that were reserved.

This could result in a transaction that has no outputs, which will never confirm because Bitcoin consensus code specifically checks that a transaction has at least one output.

We can unreserve the set by passing the reservation code to unreserveinputs, or we can transform the reserved set into a transaction by passing the reservation code, plus expected fee, to a new txpreparereserved command (which also gets the actual output set).

We need an atomic method to move reserved inputs to a "real" transaction. From what I understand of reserveinputs/unreserveinputs it looks like you are planning to reserve inputs then unreserve then prepare tx? If so, please remember, race conditions exist, we need to design proper uncuttable atomic operations. This is the reason why "all" has to be implemented in lightningd and we cannot cannot cannot use listfunds in a plugin to implement "all".

[niftynei]

Technically it constructs a PSBT, but point taken. You need to specify a desired out amount and the change amount needs to be stored somewhere —a transaction’s outputs is exactly the data struct which encodes this data. One idea I had was to substitute in a to-us address for the output’s scriptPubkey — that way you’d definitely end up with the funds if it accidentally gets signed + sent. The problem with this is that p2wpkh and p2wsh scripts differ by 12? bytes. So your feerate would be undershooting if you swapped in a p2wsh. p2sh-p2wpkh is overshooting though — maybe over is better than under.

…

On Tue, Jun 16, 2020 at 20:06 ZmnSCPxj, ZmnSCPxj jxPCSmnZ < ***@***.***> wrote: The point as I understood it was to specifically not handle a valid transaction that could be broadcast with scriptPubKeys that would not allow the owner to recover funds. It looks like reserveinputs constructs a transaction, which is what we would like to avoid in the first place. It would be nice if we could do reservations *without* a transaction being created in the first place, just reserve some utxos. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3415 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIMAKJDHF2L7ZURS62OSQTRXAJKFANCNFSM4KG7TRUA> .