Repository navigation
Feature: txmodifypayee a txprepared transaction #3415
Description
While studying the existing fundchannel in preparation for fully implementing #1936 , I noticed that the current fundchannel does this:
connectto peer.txpreparea "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.
- Extract the funding output amount in the returned transaction --- presumably this is how we determine the actual amount when given
fundchannel_startto peer using the extracted amount, receiving a target funding addresstxdiscardthe previous "dry-run" transaction.txprepare, this time with the now-filled-in funding address.fundchannel_completewith the transaction ID.txsendthe 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 ?
Activity
ZmnSCPxj commentedon Jan 17, 2020
Another potential race condition is that if we are using
allfor theamountbut not indicating thefeerate, then in the firsttxpreparewe are able to get a specific amount forallwhich we use forfundchannel_start, then feerate estimation changes to become higher, then the redonetxprepare(which will use an exact amount instead ofall, extracting the amount from the dryruntxprepare) may fail due to lack of funds.niftynei commentedon Jan 20, 2020
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
fundchanneldocumentation. hehehe. dusts hands offthere's a few problems here that we can tease apart.
fundchannel_start, however we don't know the output address for the funding transaction until afterfundchannel_startis completed.Here are some options:
txprepare. this fixes the race condition by havingtxprepareonly called once, with all valid information.fundchannel_startand, then,txprepare(which we call with the specified feerate). we may even be able to use the existing RPC callslistfundsandfeeratesto patch this data together today, without modifying c-lightning. this fixes the problem by allowing us to calltxprepareonce, with all valid information.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 calltxprepareonce 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_startis 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 commentedon Jan 20, 2020
1 avoids the problem but brings back #3402, thus not a solution. 2 does not avoid the problem: Between the time we get
alland the time that we actuallytxprepare, onchain funds (and thus the value ofall) could be modified by a separate process talking tolightningd, which is the root race problem anyway.ZmnSCPxj commentedon Jun 16, 2020
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
nLockTimeargument totxprepare. Then for "dummy" transactions which are not intended to be actually broadcast, use a far-future date fornLockTime. 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
nLockTimeis 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 commentedon Jun 16, 2020
Ah, I forgot about the fee computation, but maybe we could set the
prevout_indexfor one of the inputs (or all) to maxint. That'd still have the desired effect of reserving the outputs usingtxpreparebut 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 commentedon Jun 16, 2020
We could split up
txprepareeven further, to atxreservethat reserves some amount(s) and supportsall(so thatallcomputation and coin reservation are atomic), and atxpreparereservedwhich creates a transaction that uses the reserved coins.txreservewould return a UUID (could be just a hex representation of a 64-bit number) which is then used bytxpreparereservedto actually prepare the funds.So
txreservemight accept an object of stringID-amounts (key is a string unique within the object, value is anything parsable byparam_amount_sat_or_all). Then it returns areservation_code(the aforementioned UUID) and a corresponding object of stringIDamounts, plus a separate field of
changewith astruct amount_sat.Then a later
txpreparereservedaccepts thereservation_codeand a mapping from the old stringID to actual addresses, and returnstxidandtx, with thetxidbeing something that can later betxsendortxdiscard.How does that sound?
The lifetime of a reservation would be indefinite, but only while the
lightningdsurvives. Not sure how to handle plugins dying before they can free up reserved coins though. Sigh. Maybetxreservecan also accept aprocess_id, whichlightningdwill poll, and if that process ID is no longer running,lightningdautomatically unreserves the reserved output? Object lifetimes across process boundaries how do they work LOLAlternately 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 usetxreservevialightning-cli, becauselightning-clicloses the connection as soon as possible. Object lifetimes....Because
txreservewould not yet know what the actual addresses are, we would assume the largestscriptPubKey, 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 commentedon Jun 16, 2020
The RPC calls in #3775 should help with this, particularly
reserveinputsandunreserveinputs.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 commentedon Jun 17, 2020
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 likereserveinputsconstructs 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
reserveinputsaccepting 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: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 newtxpreparereservedcommand (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/unreserveinputsit 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 inlightningdand we cannot cannot cannot uselistfundsin a plugin to implement"all".niftynei commentedon Jun 17, 2020
ZmnSCPxj commentedon Jun 17, 2020
Well, I suppose we need to ask @cdecker just what we are protecting from. Obviously it cannot protect against someone who gets access to db + hsmd, or RPC. So this is basically a belt-and-suspenders thing here in programmer-land; what would be sufficient protection against programmer mistakes here?
ZmnSCPxj commentedon Jun 30, 2020
Okay, I just took a look over at the new
reserveinputscommand. Is my understanding correct that I can do something like this?reserveinputswith dummy outputs -> keep PSBT.fundchannel_start.fundchannel_complete.signpsbt<- the command exists but is not documented indoc/.sendpsbtexists, and is undocumented, and is (I assume)signpsbtfollowed bysendrawtransaction.signpsbt-> save PSBT with signatures.sendpsbtthe saved PSBT fromsignpsbt. Yeah you need to document the new APIs.sendrawtransactionthe finalized transaction from PSBT.Thebclisendrawtransactioncommand is accessible by plugins via thelightning-rpc, right? This is useful so that we use the same transaction broadcast policy if the user replacesbcliwith their own. @darosior ?@niftynei ?
This approximates what I need for
txmodify; I need to reserve the inputs, then ensure the inputs remain continuously reserved until the transaction is finalized.niftynei commentedon Jun 30, 2020
Yes that looks correct. I should warn you that @rustyrussell got wind of some changes I was making to the utxo infrastructure and is changing the interface a bit, so that you'd need to call something like
populatepsbtto identify the utxos you want to spend and thenreserveinputsto mark them as reserved.This is a bit trickier than you'd think it should be, since the global_tx specifies that all sigScript fields must be blank. This means that for any tx spending a P2SH- wrapped UTXO the txid of the PSBT's global tx is 'incorrect'. One way around this would be to call
signpsbtafter editing the outputs, which will produce the finalized tx object, and taking the txid from there.Alternatively, calculate the 'filled in but not signed tx/txid' from a PSBT via a new method.
Worth mentioning that the 'idealized' version of
fundchannel_*accepts a PSBT and not the txid/output-index. The interface for dual funding will include this update, and will eventually replace thefundchannel_*set.ZmnSCPxj commentedon Jul 1, 2020
Okay, I suppose I should let this settle down before planning any changes to
mutlifundchannel(which I intendfundchannelto be a thin wrapper around).Should that not be a single atomic operation? Like I point out in #3798 (comment) it would be better if this is a single atomic operation otherwise a robust plugin has to do a livelockable loop in case other plugins call
populatepsbtin parallel. You could add an option later to just identify inputs without reserving them.Welllllll that is mildly undesirable, as before
fundchannel_completereturns the signed PSBT is a liability, and removes belt-and-suspenders.But this is the by far simplest solution.
I suppose you mean
fundchannel_complete?fundchannel_startonly cares about the amount and the peer. Then it should accept the finalized PSBT with all the inputs signed? Even more dangerous...ZmnSCPxj commentedon Jul 3, 2020
It strikes me as well that we can implement
txprepare/txdiscard/txsendin terms of PSBTs.txprepare:reserveinputs,signpsbt-> extract txid, put in txid->PSBT map.txdiscard: look up txid and get PSBT,unreserveinputsand remove txid from map.txsend: look up txid and get PSBT,sendpsbtand remove txid from map.