Skip to content

Add NoopAdd HTLCs #9871

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add NoopAdd HTLCs #9871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9697b18
lnwallet: add noop updateType to paymendDescriptor
GeorgeTsagk Jun 11, 2025
03cb342
lnwallet: add IsAdd helper to AuxHtlcDescriptor
GeorgeTsagk May 27, 2025
445ab1b
lnwallet: detect and handle noop HTLCs
GeorgeTsagk May 27, 2025
cc51a69
lnwallet: add noop HTLC tests
GeorgeTsagk May 27, 2025
2b2ffb6
lnwallet: add noop case to retransmit test
GeorgeTsagk Jun 25, 2025
9fedbd6
lnwallet: add table-driven test for evaluateNoOpHtlc helper
GeorgeTsagk Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions lnwallet/aux_signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,20 @@ import (
"github.com/lightningnetwork/lnd/tlv"
)

// htlcCustomSigType is the TLV type that is used to encode the custom HTLC
// signatures within the custom data for an existing HTLC.
var htlcCustomSigType tlv.TlvType65543
var (
// htlcCustomSigType is the TLV type that is used to encode the custom
// HTLC signatures within the custom data for an existing HTLC.
htlcCustomSigType tlv.TlvType65543

// NoOpHtlcType is the TLV that that's used in the update_add_htlc
// message to indicate the presence of a noop HTLC. This has no encoded
// value, its presence is used to indicate that the HTLC is a noop.
NoOpHtlcType tlv.TlvType65544
)

// NoopAddHtlcType is the (golang) type of the TLV record that's used to signal
// that an HTLC should be a noop HTLC.
type NoopAddHtlcType = tlv.TlvType65544
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we defining the same type twice?


// AuxHtlcView is a struct that contains a safe copy of an HTLC view that can
// be used by aux components.
Expand Down
143 changes: 127 additions & 16 deletions lnwallet/channel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,12 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
remoteOutputIndex = htlc.OutputIndex
}

customRecords := htlc.CustomRecords.Copy()

entryType := entryTypeForHtlc(
customRecords, lc.channelState.ChanType,
)

// With the scripts reconstructed (depending on if this is our commit
// vs theirs or a pending commit for the remote party), we can now
// re-create the original payment descriptor.
Expand All @@ -559,7 +565,7 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
RHash: htlc.RHash,
Timeout: htlc.RefundTimeout,
Amount: htlc.Amt,
EntryType: Add,
EntryType: entryType,
HtlcIndex: htlc.HtlcIndex,
LogIndex: htlc.LogIndex,
OnionBlob: htlc.OnionBlob,
Expand All @@ -570,7 +576,7 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
theirPkScript: theirP2WSH,
theirWitnessScript: theirWitnessScript,
BlindingPoint: htlc.BlindingPoint,
CustomRecords: htlc.CustomRecords.Copy(),
CustomRecords: customRecords,
}, nil
}

Expand Down Expand Up @@ -1100,6 +1106,10 @@ func (lc *LightningChannel) logUpdateToPayDesc(logUpdate *channeldb.LogUpdate,
},
}

pd.EntryType = entryTypeForHtlc(
pd.CustomRecords, lc.channelState.ChanType,
)

isDustRemote := HtlcIsDust(
lc.channelState.ChanType, false, lntypes.Remote,
feeRate, wireMsg.Amount.ToSatoshis(), remoteDustLimit,
Expand Down Expand Up @@ -1336,6 +1346,10 @@ func (lc *LightningChannel) remoteLogUpdateToPayDesc(logUpdate *channeldb.LogUpd
},
}

pd.EntryType = entryTypeForHtlc(
pd.CustomRecords, lc.channelState.ChanType,
)

// We don't need to generate an htlc script yet. This will be
// done once we sign our remote commitment.

Expand Down Expand Up @@ -1736,7 +1750,7 @@ func (lc *LightningChannel) restorePendingRemoteUpdates(
// but this Add restoration was a no-op as every single one of
// these Adds was already restored since they're all incoming
// htlcs on the local commitment.
if payDesc.EntryType == Add {
if payDesc.isAdd() {
continue
}

Expand Down Expand Up @@ -1881,7 +1895,7 @@ func (lc *LightningChannel) restorePendingLocalUpdates(
}

switch payDesc.EntryType {
case Add:
case Add, NoOpAdd:
// The HtlcIndex of the added HTLC _must_ be equal to
// the log's htlcCounter at this point. If it is not we
// panic to catch this.
Expand Down Expand Up @@ -2993,6 +3007,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView,
)
if rmvHeight == 0 {
switch {
// If this a noop add, then when we settle the
// HTLC, we actually credit the sender with the
// amount again, thus making it a noop. Noop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's accurate as we conditionally cancel the amount.

// HTLCs are only triggered by external software
// using the AuxComponents and only for channels
// that use the custom tapscript root.
case entry.EntryType == Settle &&
addEntry.EntryType == NoOpAdd:

lc.evaluateNoOpHtlc(
entry, party, &balanceDeltas,
)

// If an incoming HTLC is being settled, then
// this means that the preimage has been
// received by the settling party Therefore, we
Expand Down Expand Up @@ -3030,7 +3057,7 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView,
liveAdds := fn.Filter(
view.Updates.GetForParty(party),
func(pd *paymentDescriptor) bool {
isAdd := pd.EntryType == Add
isAdd := pd.isAdd()
shouldSkip := skip.GetForParty(party).
Contains(pd.HtlcIndex)

Expand Down Expand Up @@ -3069,7 +3096,7 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView,
// corresponding to whoseCommitmentChain.
isUncommitted := func(update *paymentDescriptor) bool {
switch update.EntryType {
case Add:
case Add, NoOpAdd:
return update.addCommitHeights.GetForParty(
whoseCommitChain,
) == 0
Expand Down Expand Up @@ -3145,6 +3172,70 @@ func (lc *LightningChannel) fetchParent(entry *paymentDescriptor,
return addEntry, nil
}

// balanceAboveReserve checks if the balance for the provided party is above the
// configured reserve. It also uses the balance delta for the party, to account
// for entry amounts that have been processed already.
func balanceAboveReserve(party lntypes.ChannelParty, delta int64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow this check - since it's only called when we are processing the Settle, and before that there must be an Add. To lock in that Add, we must have already checked that the channel balance wouldn't drop below the reserve, plus a few other checks such as the fee buffer. So this method should always return true? Unless we don't check for reserves when processing tap HTLCs?

channel *channeldb.OpenChannel) bool {

channel.RLock()
defer channel.RUnlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This usually means a method should exist on the struct itself.


c := channel

localReserve := lnwire.NewMSatFromSatoshis(c.LocalChanCfg.ChanReserve)
remoteReserve := lnwire.NewMSatFromSatoshis(c.RemoteChanCfg.ChanReserve)

switch {
case party.IsLocal():
totalLocal := c.LocalCommitment.LocalBalance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the balance should come from commitChain.tip().ourBalance as c.LocalCommitment is not updated yet?

if delta >= 0 {
totalLocal += lnwire.MilliSatoshi(delta)
} else {
totalLocal -= lnwire.MilliSatoshi(-1 * delta)
}

return totalLocal > localReserve

case party.IsRemote():
totalRemote := c.RemoteCommitment.RemoteBalance
if delta >= 0 {
totalRemote += lnwire.MilliSatoshi(delta)
} else {
totalRemote -= lnwire.MilliSatoshi(-1 * delta)
}

return totalRemote > remoteReserve
}

return false
}
Comment on lines +3178 to +3212
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There's a potential for an integer underflow here. totalLocal is a lnwire.MilliSatoshi (which is a uint64), and delta is an int64. If delta is negative and its absolute value is greater than totalLocal, the subtraction totalLocal -= lnwire.MilliSatoshi(-1 * delta) will underflow. This would cause incorrect balance calculations, potentially leading to channel failures.

To prevent this, all balance calculations involving deltas should be performed using signed integers before converting back to lnwire.MilliSatoshi. The same issue exists for the remote party's balance calculation.

	switch {
	case party.IsLocal():
		// To avoid underflows, we'll do the balance arithmetic using
		// signed integers.
		newBalance := int64(c.LocalCommitment.LocalBalance) + delta
		if newBalance < 0 {
			return false
		}

		return lnwire.MilliSatoshi(newBalance) > localReserve

	case party.IsRemote():
		// To avoid underflows, we'll do the balance arithmetic using
		// signed integers.
		newBalance := int64(c.RemoteCommitment.RemoteBalance) + delta
		if newBalance < 0 {
			return false
		}

		return lnwire.MilliSatoshi(newBalance) > remoteReserve
	}

Copy link
Collaborator Author

@GeorgeTsagk GeorgeTsagk Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we know that the negative delta can never be greater than the balance, as that would indicate a greater issue in the lightning channel state machine. We verify what the balance is before creating the add entry in the log update:

lnd/lnwallet/channel.go

Lines 6018 to 6030 in 4f0d187

func (lc *LightningChannel) addHTLC(htlc *lnwire.UpdateAddHTLC,
openKey *models.CircuitKey, buffer BufferType) (uint64, error) {
lc.Lock()
defer lc.Unlock()
pd := lc.htlcAddDescriptor(htlc, openKey)
if err := lc.validateAddHtlc(pd, buffer); err != nil {
return 0, err
}
lc.updateLogs.Local.appendHtlc(pd)

If that was the case we'd run into it here too:

lnd/lnwallet/channel.go

Lines 4743 to 4752 in 4f0d187

if deltas.Local >= 0 {
ourBalance += lnwire.MilliSatoshi(deltas.Local)
} else {
ourBalance -= lnwire.MilliSatoshi(-1 * deltas.Local)
}
if deltas.Remote >= 0 {
theirBalance += lnwire.MilliSatoshi(deltas.Remote)
} else {
theirBalance -= lnwire.MilliSatoshi(-1 * deltas.Remote)
}

I'm not against adding extra checks, but that would mean we'd have to change a few func signatures to return errors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we know that the negative delta can never be greater than the balance, as that would indicate a greater issue in the lightning channel state machine. We verify what the balance is before creating the add entry in the log update:

A comment that explains the assumption here would be appropriate.
Otherwise someone coming across this in the future will have to dig through a lot of stuff to find out why this is safe.


// evaluateNoOpHtlc applies the balance delta based on whether the NoOp HTLC is
// considered effective. This depends on whether the receiver is already above
// the channel reserve.
func (lc *LightningChannel) evaluateNoOpHtlc(entry *paymentDescriptor,
party lntypes.ChannelParty, balanceDeltas *lntypes.Dual[int64]) {

channel := lc.channelState
delta := balanceDeltas.GetForParty(party)

// If the receiver has existing balance above reserve then we go ahead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dual Local/Remote of a commitment has nothing to do with Sender/Receiver of an HTLC here - either we are receiving or sending HTLCs, there are always two rounds of commit_sig and revoke_and_ack being exchanged.

// with crediting the amount back to the sender. Otherwise we give the
// amount to the receiver. We do this because the receiver needs some
// above reserve balance to anchor the AuxBlob. We also pass in the so
// far calculated delta for the party, as that's effectively part of
// their balance within this view computation.
if balanceAboveReserve(party, delta, channel) {
party = party.CounterParty()
}

d := int64(entry.Amount)
balanceDeltas.ModifyForParty(party, func(acc int64) int64 {
return acc + d
})
}

// generateRemoteHtlcSigJobs generates a series of HTLC signature jobs for the
// sig pool, along with a channel that if closed, will cancel any jobs after
// they have been submitted to the sigPool. This method is to be used when
Expand Down Expand Up @@ -3833,7 +3924,7 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
// Go through all updates, checking that they don't violate the
// channel constraints.
for _, entry := range updates {
if entry.EntryType == Add {
if entry.isAdd() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update recordSettlement at L4762

// An HTLC is being added, this will add to the
// number and amount in flight.
amtInFlight += entry.Amount
Expand Down Expand Up @@ -5712,7 +5803,7 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) (
// don't re-forward any already processed HTLC's after a
// restart.
switch {
case pd.EntryType == Add && committedAdd && shouldFwdAdd:
case pd.isAdd() && committedAdd && shouldFwdAdd:
// Construct a reference specifying the location that
// this forwarded Add will be written in the forwarding
// package constructed at this remote height.
Expand All @@ -5731,7 +5822,7 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) (
addUpdatesToForward, pd.toLogUpdate(),
)

case pd.EntryType != Add && committedRmv && shouldFwdRmv:
case !pd.isAdd() && committedRmv && shouldFwdRmv:
// Construct a reference specifying the location that
// this forwarded Settle/Fail will be written in the
// forwarding package constructed at this remote height.
Expand Down Expand Up @@ -5970,7 +6061,7 @@ func (lc *LightningChannel) GetDustSum(whoseCommit lntypes.ChannelParty,
// Grab all of our HTLCs and evaluate against the dust limit.
for e := lc.updateLogs.Local.Front(); e != nil; e = e.Next() {
pd := e.Value
if pd.EntryType != Add {
if !pd.isAdd() {
continue
}

Expand All @@ -5989,7 +6080,7 @@ func (lc *LightningChannel) GetDustSum(whoseCommit lntypes.ChannelParty,
// Grab all of their HTLCs and evaluate against the dust limit.
for e := lc.updateLogs.Remote.Front(); e != nil; e = e.Next() {
pd := e.Value
if pd.EntryType != Add {
if !pd.isAdd() {
continue
}

Expand Down Expand Up @@ -6062,9 +6153,12 @@ func (lc *LightningChannel) MayAddOutgoingHtlc(amt lnwire.MilliSatoshi) error {
func (lc *LightningChannel) htlcAddDescriptor(htlc *lnwire.UpdateAddHTLC,
openKey *models.CircuitKey) *paymentDescriptor {

customRecords := htlc.CustomRecords.Copy()
entryType := entryTypeForHtlc(customRecords, lc.channelState.ChanType)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't an error path here yet, but we should return an error if this type is used for any channel type other than the aux channels. This'll serve to contain the logic a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why error out instead of no-op'ing the noop?

i.e

		if lc.ChanType().HasTapscriptRoot() {
			entryType = NoopAdd
		}

if this for any reason is happening over a normal channel, the TLV field will be ignored and won't change any state whatsoever

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that happens to a normal channel which does not understand the TLV, you propose just treating it as a normal Add ? But I think we should error the channel if a peer tries to trick us into adding a NOOPADD for a normal channel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that happens to a normal channel which does not understand the TLV, you propose just treating it as a normal Add

Yes, if for any reason (maybe even by accident) the noop gets signaled then we should simply revert to old behavior.

But I think we should error the channel if a peer tries to trick us into adding a NOOPADD for a normal channel.

Need to account for potential bugs in software. If for any reason the flag "escapes" the scope of being set only over custom channels we wouldn't want to cause force-closes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why error out instead of no-op'ing the noop?

To make the failure explicit, as it should never happen. If we silently downgrade an unexpected noop HTLC (say from a legacy static key channel), then it may be harder to figure out what happened after the fact.

We can put this check in validateCommitmentSanity, as it's already possible that we'll error out if a peer or the switch sends us a bad/invalid HTLC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to account for potential bugs in software. If for any reason the flag "escapes" the scope of being set only over custom channels we wouldn't want to cause force-closes.

If that's the case we should let the FC happen so we know something is wrong, otherwise the bug will be sitting there silently forever.

return &paymentDescriptor{
ChanID: htlc.ChanID,
EntryType: Add,
EntryType: entryType,
RHash: PaymentHash(htlc.PaymentHash),
Timeout: htlc.Expiry,
Amount: htlc.Amount,
Expand All @@ -6073,7 +6167,7 @@ func (lc *LightningChannel) htlcAddDescriptor(htlc *lnwire.UpdateAddHTLC,
OnionBlob: htlc.OnionBlob,
OpenCircuitKey: openKey,
BlindingPoint: htlc.BlindingPoint,
CustomRecords: htlc.CustomRecords.Copy(),
CustomRecords: customRecords,
}
}

Expand Down Expand Up @@ -6126,17 +6220,20 @@ func (lc *LightningChannel) ReceiveHTLC(htlc *lnwire.UpdateAddHTLC) (uint64,
lc.updateLogs.Remote.htlcCounter)
}

customRecords := htlc.CustomRecords.Copy()
entryType := entryTypeForHtlc(customRecords, lc.channelState.ChanType)

pd := &paymentDescriptor{
ChanID: htlc.ChanID,
EntryType: Add,
EntryType: entryType,
RHash: PaymentHash(htlc.PaymentHash),
Timeout: htlc.Expiry,
Amount: htlc.Amount,
LogIndex: lc.updateLogs.Remote.logIndex,
HtlcIndex: lc.updateLogs.Remote.htlcCounter,
OnionBlob: htlc.OnionBlob,
BlindingPoint: htlc.BlindingPoint,
CustomRecords: htlc.CustomRecords.Copy(),
CustomRecords: customRecords,
}

localACKedIndex := lc.commitChains.Remote.tail().messageIndices.Local
Expand Down Expand Up @@ -9825,7 +9922,7 @@ func (lc *LightningChannel) unsignedLocalUpdates(remoteMessageIndex,

// We don't save add updates as they are restored from the
// remote commitment in restoreStateLogs.
if pd.EntryType == Add {
if pd.isAdd() {
continue
}

Expand Down Expand Up @@ -9999,3 +10096,17 @@ func (lc *LightningChannel) ZeroConfRealScid() fn.Option[lnwire.ShortChannelID]

return fn.None[lnwire.ShortChannelID]()
}

// entryTypeForHtlc returns the add type that should be used for adding this
// HTLC to the channel. If the channel has a tapscript root and the HTLC carries
// the NoOp bit in the custom records then we'll convert this to a NoOp add.
func entryTypeForHtlc(records lnwire.CustomRecords,
chanType channeldb.ChannelType) updateType {

noopTLV := uint64(NoOpHtlcType.TypeVal())
if _, ok := records[noopTLV]; ok && chanType.HasTapscriptRoot() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least log an error here when there is such a tlv record, yet the channel isn't aux chan.

return NoOpAdd
}

return Add
}
5 changes: 5 additions & 0 deletions lnwallet/payment_descriptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,3 +319,8 @@ func (pd *paymentDescriptor) setCommitHeight(
)
}
}

// isAdd returns true if the paymentDescriptor is of type Add.
func (pd *paymentDescriptor) isAdd() bool {
return pd.EntryType == Add || pd.EntryType == NoOpAdd
}