Use new NoopAddHtlc TLV when sending assets with default above-dust anchor amt #1567

GeorgeTsagk · 2025-05-27T14:31:49Z

Description

When sending HTLCs that carry assets we now set the noop flag whenever the amount is the default anchor amount. Since that amount is mandatory for the anchoring of assets we want to keep it around while HTLCs are inflight, but not actually send it to the channel counter party when the HTLC settles. This is exactly what the noop flag triggers.

The feature is currently hidden behind the go:build dev flag, as it will break backwards compatibility if used by default. This will be the case until we introduce feature related peer messages for custom channels.

Closes #888
Closes #1305

coveralls · 2025-05-27T14:53:54Z

Pull Request Test Coverage Report for Build 15278104744

Details

1 of 28 (3.57%) changed or added relevant lines in 4 files are covered.
48 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.06%) to 37.112%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfqmsg/noop_add_active.go	0	3	0.0%
rfqmsg/records.go	1	4	25.0%
tapchannel/commitment.go	0	8	0.0%
tapchannel/aux_traffic_shaper.go	0	13	0.0%

Files with Coverage Reduction	New Missed Lines	%
asset/group_key.go	2	57.89%
tapchannel/aux_leaf_signer.go	2	43.08%
commitment/tap.go	3	71.59%
tapdb/universe.go	4	80.4%
asset/asset.go	5	45.27%
asset/mock.go	6	64.42%
address/address.go	8	67.47%
address/mock.go	18	86.93%

Totals
Change from base Build 15255103485:	-0.06%
Covered Lines:	26971
Relevant Lines:	72675

💛 - Coveralls

rfqmsg/noop_add_active.go

Roasbeef · 2025-05-28T00:19:16Z

rfqmsg/records.go

@@ -135,6 +143,11 @@ func (h *Htlc) Records() []tlv.Record {
 		records = append(records, r.Record())
 	})

+	if h.NoopAdd {
+		r := tlv.NewPrimitiveRecord[lnwallet.NoopAddHtlcType](true)


Another thing to update in the bLIPs.

Roasbeef · 2025-05-28T00:24:07Z

tapchannel/aux_traffic_shaper.go

-		return totalAmount, htlcCustomRecords, nil
+
+		htlc.SetNoopAdd()
+		updatedRecords, err := htlc.ToCustomRecords()


Seems the aspect related to the reserve that you mentioned isn't in this PR yet?

It's not updated yet and it's on the LND PR, here we don't really have access to the channel state so we only decide whether we flag the noop to LND or not

lightninglabs-deploy · 2025-06-25T11:36:46Z

@GeorgeTsagk, remember to re-request review from reviewers when ready

guggero

Looks good, surprised about the small diff. Will do another round once I've taken a look at the lnd PR.

guggero · 2025-06-25T12:03:26Z

rfqmsg/records.go

+	// normal HTLC except for the settlement step, where the satoshi amount
+	// is returned back to the sender, but the commitment blob is still
+	// updated to reflect the asset balance changes.
+	NoopAdd bool


Shouldn't this be an optional record directly? Then below we can simply do:

h.NoopAdd.WhenSome(func(r tlv.RecordT[NoopAddHtlcType, bool]) { records = append(records, r.Record()) })

Then decoding will also work correctly, which I think currently it doesn't (should also add unit test cases for this).

Hmm, looking at the rest of the diff, it looks like we never actually detect this flag in tapd. Going to circle back to this after taking a look at the lnd PR.

within tapd we check for the existence of this flag a bit further in this file, but only for the purpose of setting the record

The record is then only detected in LND, here we only care about whether we set it or not

guggero · 2025-06-25T12:09:24Z

tapchannel/aux_traffic_shaper.go

@@ -431,7 +431,14 @@ func (s *AuxTrafficShaper) ProduceHtlcExtraData(totalAmount lnwire.MilliSatoshi,
 	if htlc.Amounts.Val.Sum() > 0 {
 		log.Tracef("Already have asset amount (sum %d) in HTLC, not "+
 			"producing extra data", htlc.Amounts.Val.Sum())
-		return totalAmount, htlcCustomRecords, nil
+
+		htlc.SetNoopAdd()


Doing the noop build flag this way works, but tbh the rfqmsg package wouldn't necessarily be the place I'd look for a build flag switch.
Should we do a aux_features_dev.go and aux_features.go in this package that defines a UseNoOpHTLCs bool variable that we then set here? E.g.: htlc.SetNoopAdd(UseNoOpHTLCs)?

Agree the aux_features.go approach looks better, we can also bundle things up in there if we introduce more dev specific features.

ZZiigguurraatt · 2025-06-28T14:26:04Z

and this is dependent on lightningnetwork/lnd#9871 ?

ZZiigguurraatt · 2025-06-28T14:39:05Z

Are you going to revert #1252 as part of this or some separate PR? We need to remove the allow_overpay option (

taproot-assets/taprpc/tapchannelrpc/tapchannel.proto

Lines 144 to 152 in 723311b

    
           // If a small invoice should be paid that is below the amount that always 
        
           // needs to be sent out to carry a single asset unit, then by default the 
        
           // payment is rejected. If this flag is set, then the payment will be 
        
           // allowed to proceed, even if it is uneconomical, meaning that more sats 
        
           // are sent out to the network than the invoice amount plus routing fees 
        
           // require to be paid. 
        
           bool allow_overpay = 6;

).

We add a new noop flag to the HTLC which, when set, will produce the corresponding TLV record. We also hide this feature behind the dev flag, as we ultimately want to be backwards compatible via some feature bit related peer messages.

Since we now have two different candidate types for adding HTLCs, we use the helper method that checks against both of them.

Whenever we use the default above-dust anchor amount to send an HTLC with assets we also set the noop flag on the HTLC, which will produce the according noop HTLC record and signal to LND that the satoshi amount should be returned to the sender.

ZZiigguurraatt · 2025-07-02T14:21:25Z

Are you going to revert #1252 as part of this or some separate PR? We need to remove the allow_overpay option (

taproot-assets/taprpc/tapchannelrpc/tapchannel.proto

Lines 144 to 152 in 723311b

// If a small invoice should be paid that is below the amount that always

// needs to be sent out to carry a single asset unit, then by default the

// payment is rejected. If this flag is set, then the payment will be

// allowed to proceed, even if it is uneconomical, meaning that more sats

// are sent out to the network than the invoice amount plus routing fees

// require to be paid.

bool allow_overpay = 6;

).

In some discussions offline yesterday it sounds like we will be depreciating this eventually, but maybe wait a release or two for all nodes to upgrade. Otherwise we will get failures if both nodes aren't aware of the new protocol. So, I don't think it will be in this PR.

GeorgeTsagk · 2025-07-04T11:46:03Z

In some discussions offline yesterday it sounds like we will be depreciating this eventually, but maybe wait a release or two for all nodes to upgrade. Otherwise we will get failures if both nodes aren't aware of the new protocol. So, I don't think it will be in this PR.

I'm not sure this is the case, this is a valid sanity check that should apply per quote. If a quote violates the overpayment, it will be rejected therefore not going to be used by LND.

ZZiigguurraatt · 2025-07-04T15:55:00Z

In some discussions offline yesterday it sounds like we will be depreciating this eventually, but maybe wait a release or two for all nodes to upgrade. Otherwise we will get failures if both nodes aren't aware of the new protocol. So, I don't think it will be in this PR.

I'm not sure this is the case, this is a valid sanity check that should apply per quote. If a quote violates the overpayment, it will be rejected therefore not going to be used by LND.

I don't think this is generally about overpaying a quote, I think it is just about overpaying in order to meet the 354 sat minimum.

GeorgeTsagk self-assigned this May 27, 2025

Roasbeef requested changes May 28, 2025

View reviewed changes

GeorgeTsagk mentioned this pull request May 28, 2025

Noop HTLC itest fixes lightninglabs/lightning-terminal#1080

Open

levmi added payment-channel premature-exhaustion Related to btc exhaustion of an asset channel P0 htlc labels Jun 2, 2025

levmi added this to Taproot-Assets Project Board Jun 2, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Jun 2, 2025

levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Jun 2, 2025

guggero mentioned this pull request Jun 16, 2025

[bug]: The fee in the asset channel was not successfully collected. #1598

Closed

GeorgeTsagk force-pushed the noop-add-tlv branch from f6a1db8 to 723311b Compare June 16, 2025 11:38

levmi requested a review from guggero June 24, 2025 16:52

guggero reviewed Jun 25, 2025

View reviewed changes

GeorgeTsagk added 4 commits July 2, 2025 13:44

(temp)build: bump lnd

0485d56

rfqmsg: add noop option to HTLC

a647204

We add a new noop flag to the HTLC which, when set, will produce the corresponding TLV record. We also hide this feature behind the dev flag, as we ultimately want to be backwards compatible via some feature bit related peer messages.

tapchannel: detect both HTLC add types

72e654d

Since we now have two different candidate types for adding HTLCs, we use the helper method that checks against both of them.

GeorgeTsagk force-pushed the noop-add-tlv branch from 723311b to d963c73 Compare July 2, 2025 12:03

GeorgeTsagk requested review from Roasbeef and guggero July 2, 2025 12:15

Use new NoopAddHtlc TLV when sending assets with default above-dust anchor amt #1567

Are you sure you want to change the base?

Use new NoopAddHtlc TLV when sending assets with default above-dust anchor amt #1567

Uh oh!

Conversation

GeorgeTsagk commented May 27, 2025

Description

Uh oh!

coveralls commented May 27, 2025

Pull Request Test Coverage Report for Build 15278104744

Details

💛 - Coveralls

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Jun 25, 2025

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZZiigguurraatt commented Jun 28, 2025

Uh oh!

ZZiigguurraatt commented Jun 28, 2025

Uh oh!

ZZiigguurraatt commented Jul 2, 2025

Uh oh!

GeorgeTsagk commented Jul 4, 2025

Uh oh!

ZZiigguurraatt commented Jul 4, 2025

Uh oh!

Uh oh!