Prepare tap channels for v1 proofs #1779
Conversation
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
6b5fffb to
6fdc32e
Compare
GeorgeTsagk
force-pushed
the
feature-bits
branch
from
74d98bf to
611a9fc
Compare
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
6fdc32e to
87174c6
Compare
|
LiT tests are expected to fail, the related LiT PR is here |
tapsend/send.go
Outdated
|
|
||
| // Verify that the new set of alt leaves has | ||
| // unique keys. | ||
| err := asset.AddLeafKeysVerifyUnique( |
There was a problem hiding this comment.
What about adding this protection, or assertion at a higher level?
When can this happen (non-unique STXO leaves passed in) within the context of the protocol?
There was a problem hiding this comment.
When funding a channel we need to "prematurely" create and include the alt leaves on the commitment because LND will cache the tapscript for future reference. That's the only case where we pre-generate the alt leaves, and that's why we added the option here for this "soft" check.
Generally we want to error out on alt leaf key conflict, but for this specific case we want to let this tool know that they may already exist -- in which case we only append the new leaves if no conflict occurs.
There was a problem hiding this comment.
The rationale behind adding the option is to be able to re-use the commitPacket helper even from the chan funding call site, where the above special conditions are met.
| // for the server to be started before being able to use it. | ||
| return tapchannel.ApplyHtlcView(s.chainParams, in) | ||
| return tapchannel.ApplyHtlcView( | ||
| s.chainParams, in, s.cfg.AuxChanNegotiator, |
There was a problem hiding this comment.
First thought at seeing this change: do we actually need to propagate this all the way here just to do something like fetch the set of aux leaves, or to apply an HTLC view?
Even before a channel is created, we know if we're going to use the STXO feature and also noop feature (for new channels). Therefore, when preparing the funding and commitment blobs, we can just store this information right there and there. So then we don't need to query for feature bits each time we want to make a new commitment or validate one from the remote peer.
The other dependnacy that pops up here is: these methods needs to always work, even if we're not currently connected to a peer. IIUC, this new aux negotiator interface can only query live peer state (what did they send us in init, etc).
There was a problem hiding this comment.
Therefore, when preparing the funding and commitment blobs, we can just store this information right there and there
Where exactly are you referring to? If you're referring to the funding/commit blobs then we can't really rely on those values for future decisions. Given that (at least for a short time window) downgrading from v1 will be allowed, then we need to know what the latest state of our peer is. Last commitment could have been using v1, but then upon reconnection our peer could stop supporting it.
The other dependnacy that pops up here is: these methods needs to always work, even if we're not currently connected to a peer. IIUC, this new aux negotiator interface can only query live peer state (what did they send us in init, etc).
I think for ApplyHTLCView we know that the channel must be active for the method to be called, which means we have communicated/agreed upon feature bits with corresponding peer. You're right for some of these cases that we could skip using the AuxChanNegotiator and rely on the commit blob, will change those hooks accordingly.
Generally, if the method affects the creation of the next commitment we'll want to consult the negotiator. If it just cares about some existing commitment, we could look up the flag in the blob.
There was a problem hiding this comment.
Where exactly are you referring to? If you're referring to the funding/commit blobs then we can't really rely on those values for future decisions. Given that (at least for a short time window) downgrading from v1 will be allowed, then we need to know what the latest state of our peer is.
Why not? We generate that blob ourselves. If say someone changes that information to say that there's only 5 beefbux in a channel but we had 10, then we'd act on that information. You're correct that atm it isn't directly cryptographically authenticated, but we assume that the information we wrote is correct.
We can protect against downgrading if we store the current negotiated feature bit information in the funding blob. If we come online, and the peer tells us we're not using the new channel type, then we can fail to proceed.
There was a problem hiding this comment.
I think for ApplyHTLCView we know that the channel must be active for the method to be called, which means we have communicated/agreed upon feature bits with corresponding peer.
Yep, you're correct about that. We only call that when accepting or creating a new commitment.
tapchannel/aux_leaf_creator.go
Outdated
| "commit state: %w", err)) | ||
| } | ||
|
|
||
| supportsSTXO := bitFetcher.GetChannelFeatures( |
There was a problem hiding this comment.
In this case, we have asccess to the blobs that we have lnd store (funding + commitment), so we can just read that state.
There was a problem hiding this comment.
Same goes for all the other instances in this commit.
IMO, we only need to query this the very first time when we create a channel.
There was a problem hiding this comment.
Thinking about things anew, I think the ideal implementation here would actually set a feature bit in the channel type message. Otherwise, we're falling back to the old implicit channel type negotiation, which we wanted to get away from by making the type of the channel being negotiated explicit.
With this route, an extension point would actually go near the funding manager in lnd, which is what decides if we understand a feature bit combo to open a new channel type.
There was a problem hiding this comment.
See #1779 (comment), focusing on closing remark
|
|
||
| // STXO is a flag indicating whether this commitment supports stxo | ||
| // proofs. | ||
| STXO tlv.RecordT[tlv.TlvType5, bool] |
There was a problem hiding this comment.
Cool, so if we're using this here, then I think we can remove many of the perior instances in the earlier commits where we're doing queries on feature bits after channel funding.
There was a problem hiding this comment.
Not really. This entry is juts a historical entry.
The fact that the last commit used STXO doesn't mean that the next commit will use STXO too. Our peer could downgrade in the meantime.
There was a problem hiding this comment.
The above entry was only really introduced for our contract resolver
taproot-assets/tapchannel/aux_sweeper.go
Line 2498 in 87174c6
specifically the part where we import the commit tx:
taproot-assets/tapchannel/aux_sweeper.go
Line 1588 in 87174c6
There was a problem hiding this comment.
The fact that the last commit used STXO doesn't mean that the next commit will use STXO too. Our peer could downgrade in the meantime.
We must continue to use it IMO. We prevent downgrading simply by refusing to continue the channel if they downgrade and want to go back to using the old version.
github-project-automation
bot
moved this from 🏗 In progress
to 👀 In review
in Taproot-Assets Project Board
GeorgeTsagk
force-pushed
the
feature-bits
branch
2 times, most recently
from
3d4c18f to
f9e10d7
Compare
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
87174c6 to
24347b7
Compare
There was a problem hiding this comment.
Did a first pass and mad comments throughout. Starts to look good! This PR depends on the litd itest working, right? (lightninglabs/lightning-terminal#1138)
tapsend/send.go
Outdated
| // existingSTXOLeaves indicates whether we should be aware that alt | ||
| // leaves that correspond to stxo assets may already exist in asset | ||
| // outputs of the vpsbt. | ||
| existingSTXOLeaves bool |
There was a problem hiding this comment.
I understand the reason of this boolean and premature tapscript generation that is referenced by lnd, but I don't like it.
This complexity makes the code harder to reason about and could be fragile. Isn't there a way to establishes a single point of responsibility for adding STXO?
There was a problem hiding this comment.
Thanks for this comment.
I was getting ready to write a long explanation on how this isn't a quirk of the commitment related toolkit but a characteristic of the chan funding flow -- i.e we need to craft the correct commitment even though we haven't yet finalized the vpsbt that creates it.
Then I realized that in my latest push, where I simplified a lot of code, I also omitted the (now unnecessary) part of actually appending the alt leaves to the vOut. This means that even though we do collect STXOs prematurely, we don't really attach it to the vOut, leaving it "clean" for the toolkit to handle it. This means that we actually never hit a case where there's duplicate alt leaves, as we'd never really persist them between preparation & finalization phase.
Will just drop the commit that adds WithExistingSTXOLeaves altogether.
There was a problem hiding this comment.
Interesting. But if we don't append them to the vOut why do we still collect the STXOs prematurely? Where do we use them in the chan funding flow?
There was a problem hiding this comment.
We directly merge them into the commitment that will be used to derive the tapscript for LND. The vOut alt leaves are just the placeholder for keeping the leaves somewhere, until we'd eventually want to merge them into the tap commitment.
So now we simply skip the step of adding the alt leaves to the vOut and later when the helper functions get the vPSBT they'll be free to re-collect/append the alt leaves without conflicts or extra ticks needed
There was a problem hiding this comment.
Had some back and forth on this with @GeorgeTsagk and an LLM who will remain anonymous. I'm posting it here for future reference.
- Premature Stage: Funding Transaction Construction (
addToFundingCommitment)
This stage computes the correctTapscriptRootfor the on-chain P2TR funding output script. LND needs this root early, before the final proofs are generated.
Call Flow:
FundingController.processFundingReq->pendingAssetFunding.addToFundingCommitment
Action:addToFundingCommitmentcallsasset.CollectSTXOand merges the resulting leaves directly into an in-memorycommitment.TapCommitment. This allows for the correctTapscriptRootto be provided to LND. This process does not modify anyVPacket'sAltLeaves. - Finalization Stage: Final Proof Generation (
commitPacket)
This stage generates the final Taproot Asset proofs for the assets being created in the channel after the funding transaction is fully known.
Call Flow:
FundingController.processFundingReq->FundingController.completeChannelFunding->FundingController.anchorVPackets->tapsend.CreateOutputCommitments->commitPacket
Action: ThecommitPacketfunction is called on theVPackets representing the channel assets. It callsasset.CollectSTXOagain and appends the resulting leaves to thevOut.AltLeavesslice before creating the final commitment for the proof.
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
24347b7 to
be7349d
Compare
Yeap. Will update the LiT PR soon to reflect all of recent changes made here |
| outCommitments, err := tapsend.CreateOutputCommitments( | ||
| directPkts, tapsend.WithNoSTXOProofs(), | ||
| ) | ||
| outCommitments, err := tapsend.CreateOutputCommitments(directPkts) |
There was a problem hiding this comment.
For my understanding: From now on we always create a sweeping Tx with stxo proofs.
Why didn't we do so before? Oli needed the WithNoSTXOProofs to make the channel commitments work, but it seems to me that the sweeping Tx could always have had stxo proofs. (This is more a question about Oli's PR than about this PR, but I'm still wondering.)
There was a problem hiding this comment.
Yeah I suppose we defaulted to using WithNoSTXOProofs everywhere in the tapchannel code, regardless of if it could have been using them in the first place. I guess it would require explaining why it would work in some places but not in others -- which is what we're doing now in this PR.
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
be7349d to
623d31d
Compare
Yeap I can write a short summary of how we actually achieve downgrade protection: The v1 proofs will become active over a channel if both peers support the feature, the negotiation for that happens during connection. We have two types of data keeping track of v1 proof presence: In our various call sites, we'll consult a) if the on-going action produces a new commitment, or a new overall state (like the chan funding tx). When referring to existing, already created commitments we'll consult their respective blob flag to find out whether they used v1 proofs or not. Restart & DowngradeIf a peer of the channel restarts and downgrades, dropping support for v1, then the reconnection negotiation will result to v1 not being active over the channel. This means that any future transactions will not use v1 proofs (commitments / co-op closing -- value acquired from We'll have a grace-period for which the v1 feature bit will be marked as optional, but eventually we'll want to switch to it being a required feature. The time window until the feature switches to a required one is the only one within which a peer may downgrade and still keep the channel active. When the switch happens the channel won't become active again until v1 is negotiated on connection. Sweeping & BreachesWithin the lifecycle of a channel it is totally expected to only have some of the commitments using v1. Given that we have this notion of a "graceful window" we also accept that there isn't a single switch from non-v1 to v1 usage over the channel. This was the main motivation behind storing the v1 flag in the commit blob. When a commitment makes it on-chain, our sweeper will consult the commit blob to see whether v1 is used, then reconstruct the tap commitment accordingly, and sweep the funds. The sweep transaction itself can be v1 regardless of what the remote party supports, as that's a transaction paying directly to the local party. |
But that isn't part of this PR, right? We should probably make a note of this on the tracking issue: https://github.com/lightninglabs/taproot-assets-private/issues/38 |
Correct. Will update the tracking issue. |
GeorgeTsagk
force-pushed
the
feature-bits
branch
from
f9e10d7 to
cf4c996
Compare
We change the commitment related generations to now include stxo proofs if the setting allows so. Previously we would always exclude stxo proofs, but now we add them if both parties understand & expect them.
This commit updates the aux funding controller to conditionally use stxo proofs, depending on whether the feature bit was negotiated or not with the remote peer. Our funding commitment will now include the alt leaves for stxos if the flag is set. The funder needs to construct the correct tapscript early on, as LND will query the tapscript root before we reach the finalization phase. That's why we manually calculate and merge the stxo alt leaves in the funding process. The fundee now also needs to calculate and merge the alt leaves that result from the asset outputs, in order to arrive to the same tapscript root.
We extend the commitment blob to also store a flag, indicating whether STXO was active when that commitment was created. This can be useful for future sweeps that need to know whether that commitment used stxo alt leaves, which affects the reconstruction of the tap commitment.
We also add stxo support for the aux leaf creation. This is crucial and needs to be the same across both channel parties. We rely on the consistency of the feature bit for whether we'll include the stxo alt leaves or not. A disagreement here could lead to a force close.
For the cooperative channel closing we also need to check if the parties have agreed upon the usage of stxo proofs. If that is the case, we'll include those commitments in the coop channel closing transaction & proof.
This commit adds stxo support to the aux sweeper. When importing a commit tx to the wallet we now consult the commit blob flag in order to find out whether that commitment was using stxo proofs or not.
GeorgeTsagk
force-pushed
the
channels-v1
branch
from
623d31d to
4f37787
Compare
Description
Adds support for v1 proofs for all channel related transactions (funding/commitments/closing/sweeping).
We also guard this feature behind a feature bit, which must be set and supported by both channel peers in order to use v1 proofs.
For some cases during sweeping, we don't really care about the feature being supported by the remote party as we're sweeping locally to our own address.
Fixes #1582