lnwallet: add new NoopAdd payDesc entry type #9430
Conversation
In this commit, we add a new NoopAdd payDesc entry type. This type is meant to be used primarily by taproot overlay channels. When we go to settle this HTLC, rather than credit the settler for the funds, we just give the funds back to the sender. This results in an add that when settled, doesn't affect the balance in the channel. This new HTLC type is intended to be used alongside a push amt, to ensure the remote party has a non-dust balance from the start. With that in place, then this new add type can be used for special overlay HTLCs.
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Okay, so we get an in flight HTLC for +354 sat, then when we settle with a new commitment transaction, HTLC is taken out of flight and we just update the new commitment to include the new taproot asset data anchored to the other outputs that are much larger. As long as each party has more than 354 sat in their balance, it's fine? Basically, on the sats side of the channel this isn't much different than canceling a HOLD invoice, except we add some new taproot assets data anchored to the updated commitment? If the HTLC must go on chain, the sender just lost 354 sat as sort of an extra penalty, but the incentive for the receiver to go on chain is normally not going to motivate them to do that because they will have to pay much more than 354 sat to do a force close of an in flight HTLC? |
|
So this solution is more efficient than sending a matching 354 sat in the opposite direction (mentioned in lightninglabs/taproot-assets#888 (comment)), because it doesn't require an extra HTLC to be in flight and therefor results in lower on chain footprint in the event that the HTLC does need to go on chain? |
Yep, or any amount really. An easy way to think about it is: we cancel the HTLC, while also revealing the preimage. This is the simplest solution I've thought of so far. |
|
What is the point of revealing the preimage? That isn't needed for the sats if you aren't changing the sats amount in the channel. I guess you need it to encourage updating of the commitment with the new taproot asset anchor? Otherwise they have no motivation to update the commitment with the new taproot assets anchor if they don't get the preimage? Also, once the preimage is revealed, what prevents someone from wanting to settle a new commitment transaction with the channel state after the 354 sats were put into an HTLC (instead of before)? Is it the same incentive model if for dust with sats, if you start to disagree over this petty thing, then the other party will just close the channel because you are not worth doing business with anymore? |
|
Updated to account for persistence. |
You may still need to go on-chain to claim the HTLC. Also there might be an incoming HTLC, so you need to reveal that so the payment circuit can be closed (all HTLCs removed from all commitments).
Can you elaborate on this? |
|
Yep, exactly. Also if there's an outgoing HTLC (multi-hop), they may be able to secure an off-chain profit from the succesful HTLC forward. |
| @@ -6043,9 +6088,25 @@ func (lc *LightningChannel) MayAddOutgoingHtlc(amt lnwire.MilliSatoshi) error { | |||
| func (lc *LightningChannel) htlcAddDescriptor(htlc *lnwire.UpdateAddHTLC, | |||
| openKey *models.CircuitKey) *paymentDescriptor { | |||
|
|
|||
| // TODO(roasbeef): can use push amt to simplify logic, not have to | |||
There was a problem hiding this comment.
wdym in this todo: always push some sats amount on channel open, to make sure remote side can always anchor? if that's the case
a) this todo shouldn't be here, as lnd doesn't know anything about custom channel funding details
b) for custom channels with large btc capacity, it may be uneconomical to push enough sats to the other side to get them over the reserve
There was a problem hiding this comment.
it may be uneconomical to push enough sats to the other side to get them over the reserve
Would need to double check, but I think the starting value of a push_amt is actually exempt from the reserve constraints.
wdym in this todo: always push some sats amount on channel open, to make sure remote side can always anchor? if that's the case
Yes, so we'd have the APIs set up to always push an amount over to ensure they can anchor for as starting state.
Otherwise, we'll need to make sure that we only ever push for the "first" HTLC, which can be a bit tricky if you have many pending HTLCs in a new commitment transaction.
|
|
||
| customRecords := htlc.CustomRecords.Copy() | ||
| if lc.channelState.ChanType.HasTapscriptRoot() { | ||
| entryType = NoopAdd |
There was a problem hiding this comment.
this is a nice way to bootstrap things, but I think HasTapscriptRoot() is insufficient as the sole criteria here
We could be sending raw btc HTLCs over the channel, or we could be sending HTLCs carrying custom data and also carry a significant amount of btc. In both of the previous cases we definitely don't want to give the amount back to sender upon settlement
From the top of my head: we could also check for this HTLC's custom records, plus the actual amount of the btc delta and then decide whether we're in a NoopAdd case
There was a problem hiding this comment.
Agreed. I think instead we may want to add a new aux interface, or re-use an existing one, to know when we should be using this or not.
| // The NoopAdd HTLC is an internal construct, and isn't meant to show up | ||
| // on the wire. So we'll remove the special element from the set of | ||
| // custom records. | ||
| delete(customRecords, noopTLV) |
There was a problem hiding this comment.
IIUC this wouldn't really matter, as the receiver of the wire message would just ignore this unknown type, are we just saving wire space here?
There was a problem hiding this comment.
Ended up doing this as we have some tests that assert what an HTLC should look like for transmission. If we continue to thread these over, then those start to fail.
On the other hand, maybe it makes sense to expose this, as then the receiver just accepts the noop if the sender does, vs the current logic of assuming that it's always noop when the tapscript bit is set.
| balanceDeltas.ModifyForParty( | ||
| party.CounterParty(), | ||
| func(acc int64) int64 { | ||
| return acc + delta |
There was a problem hiding this comment.
👍 So by default the custom aux blob is being updated / preserved in the background, so the only diff required is the one that pays back the sats, didn't expect this to be that lean 💯
There was a problem hiding this comment.
Yep, was great how small it turned out to be in the end. This area of the code has been refactored a bit also, which served to simplify the addition of this new feature.
| addEntry.EntryType == NoopAdd: | ||
|
|
||
| delta := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( |
There was a problem hiding this comment.
Also important to mention that parties could disagree here, with receiver potentially trying out malicious things (like going to chain, like @ZZiigguurraatt mentioned), and try to "grab" both the custom data and the btc amount
That's a good reason why I believe we should not convert the entry type to NoopAdd for HTLCs that carry a btc amount above a certain threshold, or let the user explicitly mark it somehow as noop
There was a problem hiding this comment.
Not sure I follow, but if they disagree, a force close occurs. Which doesn't benefit either party, as they need to pay for chain fees, and now the receiver will likely end up with nothing as it'll cost more to spend the output than it's actually worth.
There was a problem hiding this comment.
I mean that we should not allow significant btc amounts being carried by HTLCs of type NoopAdd
That breaks the incentive alignment and someone could actually want to misbehave and force-close/pay fees
In this commit, we add a new NoopAdd payDesc entry type. This type is meant to be used primarily by taproot overlay channels. When we go to settle this HTLC, rather than credit the settler for the funds, we just give the funds back to the sender. This results in an add that when settled, doesn't affect the balance in the channel.
This new HTLC type is intended to be used alongside a push amt, to ensure the remote party has a non-dust balance from the start. With that in place, then this new add type can be used for special overlay HTLCs.