Skip to content

Noop HTLC itest fixes #1080

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

Open
wants to merge 3 commits into
base: tapd-main-branch
Choose a base branch
from

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented May 28, 2025

Description

Modifies some of our existing custom channels itests to now include some manual sats keysends to provide enough liquidity on some nodes to carry out the test case.

Our previous expectation was that asset HTLCs would always pay some sats to the receiver, but now this only happens up to the chan reserve amount.

Based on: lightningnetwork/lnd#9871 , lightninglabs/taproot-assets#1567

Extra note

Since we introduced NoOp HTLCs on LND/Tapd that means we are no longer compatible with older versions. That leads to some itest failures, specifically the test cases that declared a backwards compatibility version for some of the nodes in the network topology.

[Update]
We updated tapd to guard the feature behind a cfg option, so it's off by default. This PR turns NoOp HTLCs on for a sub-set of itests, which do not define a backwards-compatibility version:
testCustomChannelsGroupTranchesForceClose
testCustomChannelsBreach
testCustomChannelsLiquidityEdgeCasesCore (assetID & groupKey)
testCustomChannelsStrictForwarding
testCustomChannelsHtlcForceCloseMpp
testCustomChannelsSelfPayment

@GeorgeTsagk GeorgeTsagk self-assigned this May 28, 2025
@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented May 28, 2025

The backwards compatibility itest is expected to fail, as the sender of the HTLCs expects a different commitment view than what the receiver is constructing.

Currently the tapd feature is hidden behind the dev build flag, but we will eventually also support lightninglabs/taproot-assets#1573

@GeorgeTsagk GeorgeTsagk changed the title Noop add itest Noop HTLC itest fixes May 28, 2025
@levmi levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Jun 2, 2025
@GeorgeTsagk GeorgeTsagk requested review from guggero and Roasbeef July 2, 2025 12:16
@GeorgeTsagk
Copy link
Member Author

Bumped lnd & tapd to the commits that address all latest feedback

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks like the backward compatibility itest causes a force close, due to the NoOp HTLC option.
We probably need to set that to false at the start of a backward compat test (and restore it to true with a defer or t.Cleanup()).

Other than that looks good to me.

@levmi levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Aug 4, 2025
@GeorgeTsagk GeorgeTsagk force-pushed the noop-add-itest branch 2 times, most recently from 399a498 to a854870 Compare August 13, 2025 16:15
@GeorgeTsagk GeorgeTsagk changed the base branch from master to tapd-main-branch August 13, 2025 16:20
@lightninglabs-deploy
Copy link

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

@GeorgeTsagk GeorgeTsagk added the no-changelog This PR is does not require a release notes entry label Aug 18, 2025
@GeorgeTsagk GeorgeTsagk requested a review from guggero August 18, 2025 16:27
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice work with the flag, looks good to me 🎉

Custom channel itests seem to be a bit flakey lately.
Re-ran it again, can merge assuming it eventually passes. But another round of flake hunting would probably be good.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 📭

I see this as a failure right now, which looks like maybe a file system issue, will rerun the failing test:

=== RUN   TestLightningTerminal/custom_channels_self-payment_backward_compatibility
    harness_setup.go:25: Setting up HarnessTest...
    harness_setup.go:38: Prepare the miner and mine blocks to activate segwit...
    harness_setup.go:46: Connecting the miner at 127.0.0.1:10694 with the chain backend...
    litd_node.go:495: Using bbolt database backend
    litd_node.go:522: Created new node Alice with p2p port 10703
Starting node=Alice, pid=29132
    litd_custom_channels_test.go:4762: 
        	Error Trace:	/home/runner/work/lightning-terminal/lightning-terminal/itest/litd_custom_channels_test.go:4762
        	            				/home/runner/work/lightning-terminal/lightning-terminal/itest/test_harness.go:103
        	            				/home/runner/work/lightning-terminal/lightning-terminal/itest/litd_test.go:88
        	Error:      	Received unexpected error:
        	            	error reading macaroon file: open /tmp/litdtest-node3060423016/data/chain/bitcoin/regtest/admin.macaroon: no such file or directory
        	Test:       	TestLightningTerminal/custom_channels_self-payment_backward_compatibility
    harness.go:393: finished test: , start height=438, end height=438, mined blocks=

@guggero
Copy link
Member

guggero commented Aug 21, 2025

LGTM 📭

I see this as a failure right now, which looks like maybe a file system issue, will rerun the failing test:

=== RUN   TestLightningTerminal/custom_channels_self-payment_backward_compatibility
    harness_setup.go:25: Setting up HarnessTest...
    harness_setup.go:38: Prepare the miner and mine blocks to activate segwit...
    harness_setup.go:46: Connecting the miner at 127.0.0.1:10694 with the chain backend...
    litd_node.go:495: Using bbolt database backend
    litd_node.go:522: Created new node Alice with p2p port 10703
Starting node=Alice, pid=29132
    litd_custom_channels_test.go:4762: 
        	Error Trace:	/home/runner/work/lightning-terminal/lightning-terminal/itest/litd_custom_channels_test.go:4762
        	            				/home/runner/work/lightning-terminal/lightning-terminal/itest/test_harness.go:103
        	            				/home/runner/work/lightning-terminal/lightning-terminal/itest/litd_test.go:88
        	Error:      	Received unexpected error:
        	            	error reading macaroon file: open /tmp/litdtest-node3060423016/data/chain/bitcoin/regtest/admin.macaroon: no such file or directory
        	Test:       	TestLightningTerminal/custom_channels_self-payment_backward_compatibility
    harness.go:393: finished test: , start height=438, end height=438, mined blocks=

I think we need to make the use of the noop-htlcs conditional. Alice didn't start up here because of:

unknown flag `taproot-assets.channel.noop-htlcs'
could not load config: error parsing flags: unknown flag `taproot-assets.channel.noop-htlcs'

@@ -4717,6 +4752,8 @@ func testCustomChannelsSelfPayment(ctx context.Context, net *NetworkHarness,
fmt.Sprintf(node.ListenerFormat, alicePort),
))

litdArgs = append(litdArgs, litdArgNoopHTLCs...)
Copy link
Member

Choose a reason for hiding this comment

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

This is executed in a backward compatibility test as well, so we have to do this conditionally. Or perhaps it's easier to just add it conditionally in HarnessNode.overrideFlagsAndBinary(), then the diff of this commit also becomes smaller and we don't have to think to add it to new tests either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah.. sneaky. Thanks for the catch @guggero!

We add a new litd argument that triggers the usage of NoOp HTLCs. We may
not use that argument in itests where backwards compatibility is
enabled, as currently this leads to a channel force-closure.
@GeorgeTsagk
Copy link
Member Author

So now we automatically append the new NoOpHTLCs argument to all itests that do not use backwards compatibility.

When feature bits get implemented we'll want to go one step further and add the flag to those itests too, but only on the nodes that do not use an older version.

@guggero
Copy link
Member

guggero commented Aug 21, 2025

Seems to fail quite early now. Probably need to adjust some additional balance checks?

@GeorgeTsagk
Copy link
Member Author

Seems to fail quite early now. Probably need to adjust some additional balance checks?

Yeah some balances are off in the closing tx, already looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR is does not require a release notes entry
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

4 participants