Closed ZmnSCPxj closed 4 years ago
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.
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.
fundchannel_start
, however we don't know the output address for the funding transaction until after fundchannel_start
is completed.Here are some options:
txprepare
. this fixes the race condition by having txprepare
only called once, with all valid information.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.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.
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.
In https://github.com/ElementsProject/lightning/pull/3763#pullrequestreview-430739538 @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 ?
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.
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.
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
.
The point as I understood it was to specifically not handle a valid transaction that could be broadcast with scriptPubKey
s 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:
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"
.
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 < notifications@github.com> 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 https://github.com/ElementsProject/lightning/issues/3415#issuecomment-645087549, 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 reserveinputs
command. Is my understanding correct that I can do something like this?
reserveinputs
with dummy outputs -> keep PSBT.fundchannel_start
.fundchannel_complete
.signpsbt
<- the command exists but is not documented in doc/
.sendpsbt
exists, and is undocumented, and is (I assume) signpsbt
followed by sendrawtransaction
.signpsbt
-> save PSBT with signatures.sendpsbt
the saved PSBT from signpsbt
. Yeah you need to document the new APIs.sendrawtransaction
the finalized transaction from PSBT.bcli
sendrawtransaction
command is accessible by plugins via the lightning-rpc
, right? This is useful so that we use the same transaction broadcast policy if the user replaces bcli
with 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.
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 populatepsbt
to identify the utxos you want to spend and then reserveinputs
to 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 signpsbt
after 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 the fundchannel_*
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 intend fundchannel
to be a thin wrapper around).
populatepsbt
to identify the utxos you want to spend and thenreserveinputs
to mark them as reserved.
Should that not be a single atomic operation? Like I point out in https://github.com/ElementsProject/lightning/pull/3798#discussion_r446807623 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 populatepsbt
in parallel. You could add an option later to just identify inputs without reserving them.
One way around this would be to call
signpsbt
after editing the outputs, which will produce the finalized tx object, and taking the txid from there.
Welllllll that is mildly undesirable, as before fundchannel_complete
returns 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_start
only 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
/txsend
in terms of PSBTs.
txprepare
: reserveinputs
, signpsbt
-> extract txid, put in txid->PSBT map.txdiscard
: look up txid and get PSBT, unreserveinputs
and remove txid from map.txsend
: look up txid and get PSBT, sendpsbt
and remove txid from map.
While studying the existing
fundchannel
in preparation for fully implementing #1936 , I noticed that the currentfundchannel
does this:connect
to peer.txprepare
a "dry-run" fundchannel using a dummy funding address.all
.fundchannel_start
to peer using the extracted amount, receiving a target funding addresstxdiscard
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
, ortxprepare
execute between thetxdiscard
of the dry-run transaction and the subsequenttxprepare
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 atxprepare
that uses the exact same inputs and outputs, just redirects the address of an output.So let me propose:
Where
modifications
is an array containing objects with fields:The
address
would 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
txdiscard
ortxsend
. If it fails, then the old txid can still be passed totxdiscard
ortxsend
.Thoughts @niftynei @cdecker @rustyrussell ?