Repository navigation
Feature: txmodifypayee a txprepared transaction #3415
Description
Activity
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.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.
- 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 afterfundchannel_startis 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 havingtxprepareonly 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_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. - 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 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.- 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
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.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 ?
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.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 stringID
amounts, plus a separate field ofchangewith 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.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.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:- 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 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".- 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> .
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.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?
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.- Edit the PSBT I am holding locally to use the correct outputs.
- Derive the txid from the PSBT ourselves.
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 ?
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.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.Derive the txid from the PSBT ourselves.
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.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
Okay, I suppose I should let this settle down before planning any changes to
mutlifundchannel(which I intendfundchannelto be a thin wrapper around).populatepsbtto identify the utxos you want to spend and thenreserveinputsto mark them as reserved.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.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.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.
Worth mentioning that the 'idealized' version of
fundchannel_*accepts a PSBT and not the txid/output-index.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...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.
While studying the existing
fundchannelin preparation for fully implementing #1936 , I noticed that the currentfundchanneldoes this:connectto peer.txpreparea "dry-run" fundchannel using a dummy funding address.all.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, ortxprepareexecute between thetxdiscardof the dry-run transaction and the subsequenttxprepareas 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
txdiscardwith atxpreparethat uses the exact same inputs and outputs, just redirects the address of an output.So let me propose:
Where
modificationsis an array containing objects with fields:The
addresswould have to take up the same amount of space as the current address at thatoutnum.The command succeeds or fails "atomically". If it succeeds, then the old txid is no longer passable to
txdiscardortxsend. If it fails, then the old txid can still be passed totxdiscardortxsend.Thoughts @niftynei @cdecker @rustyrussell ?