-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[psbt saga part 3/4]: taprpc: add CommitVirtualPsbts and PublishAndLogTransfer RPC methods #787
Conversation
@GeorgeTsagk it's still in draft, needs a lot of cleanup and some decisions. But should have what you need to build the sighash swap itest. |
0603206
to
0fe1702
Compare
@Roasbeef I have the following flow in mind for driving an asset send mostly from outside of the daemon (for the
My current question is mostly around step 4: To transport the unfinished proof suffixes and optional proof courier addresses from step 3 to step 4 we would need to place them somewhere. I would argue that the vPSBT outputs would be a good place for that. But I distinctly remember that you were against storing proofs or proof courier addresses in the vPSBTs. Do you remember why you argued that way? Mostly due to the size of the vPSBTs? |
} | ||
|
||
lndWallet := r.cfg.Lnd.WalletKit | ||
fundedPacket, changeIndex, lockedUTXO, err := lndWallet.FundPsbt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC at this point there is a strict check that the inputs amount must equal the outputs amount.
If I want to create an incomplete anchorTx, could we somehow signal that there may be an input or output missing, hence no need to perform strict checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the input and output sum doesn't have to be equal here. That's exactly the task for lndWallet.FundPsbt
to figure out if additional inputs are required to fund the outputs.
But that does mean that all outputs need to be specified here, otherwise we wouldn't know what to fund.
Yeah I think that makes sense. Alternatively, the user may wish to just have the set of participants side load the proofs as well. Though either way you'd want to make sure that the sender's DB is updated to reflect that those outputs are now spent. In theory, we could also just watch all the outpoints we believe to be unspent for spends. In that case though, we'd still want to know things like if a change output was created that does still store outputs.
Agreed, as at that point the user (channel daemon) is managing all those outputs. Alternatively, this could go into a sort of "watch only" mode, where the daemon keeps track of the outputs for accounting purposes, but understands it can't unilaterally sign for them itself.
Hmm, IIRC I think it was about the potential size of the vPSBTs assuming it was actually storing the entire proof. If it's just the proof suffix, then I think that's ok, as their sort of bounded, in that the largest item is usually the Bitcoin block header merkle proof. |
408a0f4
to
0f67be5
Compare
2a2d072
to
a4627e5
Compare
a4627e5
to
c3051f7
Compare
1f1fa26
to
9aead7d
Compare
c3051f7
to
1d9c31a
Compare
1d9c31a
to
132290e
Compare
72944ca
to
c22283d
Compare
132290e
to
783fa63
Compare
c22283d
to
c6d31e2
Compare
783fa63
to
4795a21
Compare
Since we'll want to be able to sign passive assets through RPC as well, we need the full input proofs to be present on passive assets.
In the new RPC flow that's going to be added in the following commits, the user will need to sign the passive assets manually. We return the passive assets in the funding call so the user can actually do that. For any existing flow these passive assets can just be ignored as the signing still happens automatically under the hood.
With this commit we refactor the code for generating the pk script for an anchor output and re-use it for both creating new outputs as well as validating input scripts. We also remove some unnecessary code and fix a typo.
Because we now give users way more control over the construction and commitment of virtual packets into a BTC anchor transaction, we cannot rely on everything being set correctly anymore. To avoid things being committed incorrectly, we create rigorous virtual packet input and output validation functions that make sure everything is ready to be committed to the chain.
Deleting an asset from an empty commitment should be a no-op and not result in a panic. This commit avoids the panic by returning early because the tree for an empty commitment is nil.
If we want to have the ability to encumber the BTC level outputs by a MuSig2 internal key and/or have a Tapscript sibling, we need to allow for the internal key to be unknown to the wallet. Otherwise we can't choose the NUMS pub key as the internal key if we wanted, for example, to only allow a script spend on the BTC level.
We no longer prune any tombstone or burn assets from the input commitment, since we'll need them later when committing all assets on-chain to validate that we can fully reconstruct the input TAP commitment tree. So we keep the purgeable assets in the input commitment and only filter them before creating the passive assets (which operates on a copy anyway, so they stay in the original input commitment).
With this commit we simplify, unify and clean up the virtual packet creation. Now we can use the same logic for both active and passive packets.
We'll want to support multiple virtual transactions as part of a transfer within the porter at some point. For now we just prepare the struct to hold a slice instead of a single packet. And we add a new parcel type that will allow us to publish, log and transfer proofs for a fully externally prepared and anchored transaction.
Now that we have a full validation logic for making sure all assets of the input anchors are properly accounted for and re-created in appropriate outputs, we want to turn on that validation during the normal send logic of the freighter as well to catch potential bugs before publishing any on-chain transaction.
In case we are sure we can't confirm or even publish an on-chain transaction, we release the UTXOs that were locked in lnd's wallet to avoid locking up balance with re-tries.
Since we reduced the node startup time delay during itests in a previous commit (a2a95e7) by removing a one second wait, we can now also decrease the wait time for a receiver to send an ACK after a restart as well. This shaves off around 15 seconds of our itests.
We fix some additional issues of long-running (>10 seconds) integration tests.
f51c888
to
8280b00
Compare
I was able to re-enable the re-org itest by bumping to the latest version of |
@guggero, remember to re-request review from reviewers when ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had another look due to the many force updates, still LGTM 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 30 of 30 files at r25, 4 of 4 files at r26, 2 of 2 files at r27, 2 of 2 files at r28, 2 of 2 files at r29, 5 of 5 files at r30, 3 of 3 files at r31, 3 of 3 files at r32, 2 of 2 files at r33, 2 of 2 files at r34, 3 of 3 files at r35, 3 of 3 files at r36, 11 of 13 files at r37, 22 of 22 files at r38, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @GeorgeTsagk and @guggero)
Depends on #805.
This is a first draft towards a new series of RPCs that allows us to drive the send logic completely from outside of the daemon.
That means we'll likely want to bypass the freighter partially or completely, since for TAP channels not all state and/or proofs of all state updates should be reflected in the database.
We'll likely want an additional RPC that allows us to store the output of the
CommitVirtualPsbts
call in the database, realizing the actual entries in the transfers and assets tables.This change is