Skip to content
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

itest: expect failure on direct rfq peer btc invoices #916

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Dec 11, 2024

Description

Previously we'd consider it acceptable to settle direct rfq peer
invoices, which included no rfq scid, with asset HTLCs. This behavior
has been updated on the tapd invoice manager and we no longer accept
asset HTLCs on invoices that do not expect assets. This commit updates
such payments in our itests to instead expect a failure.

Tapd dependency: lightninglabs/taproot-assets#1244
Lnd dependency: lightningnetwork/lnd#9357

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.

itest seems to fail.

itest/litd_custom_channels_test.go Outdated Show resolved Hide resolved
@GeorgeTsagk
Copy link
Member Author

itest seems to fail.

interesting, could be flaky, didn't hit this on local tests

@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented Dec 11, 2024

also linter seems to fail here with
https://github.com/lightninglabs/lightning-terminal/actions/runs/12275164256/job/34257220772?pr=916#step:5:293

go: github.com/lightninglabs/lightning-terminal imports
	github.com/lightningnetwork/lnd/kvdb imports
	github.com/lightningnetwork/lnd/kvdb/etcd imports
	go.etcd.io/etcd/server/v3/embed: mkdir /home/runner/go/pkg/mod/cache/download/go.etcd.io/etcd/server: permission denied

passes locally

Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

👍 need to remove the temp commit (waiting on dep I guess?), otherwise looks good.

@GeorgeTsagk
Copy link
Member Author

thanks @ffranr , currently working on resolving an error with the htlc force-close test, will push once it has been resolved

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.

LGTM pending dependant PRs and nit mentioned in earlier review.

@guggero
Copy link
Member

guggero commented Dec 16, 2024

Can be updated to lnd commit 04767fe which will remain stable, even after tagging the RC.

guggero and others added 10 commits December 16, 2024 13:22
This commit represents the main integration between lnd running in
integrated mode and tapd providing auxiliary components for custom
channels.
This change will speed up integration tests by not waiting a full 5
seconds before re-trying the connection to lnd if it fails the first
time.
Co-authored-by: Olaoluwa Osuntokun <[email protected]>
Co-authored-by: Gijs van Dam <[email protected]>
Co-authored-by: George Tsagkarelis <[email protected]>
Add the `priceoraclerpc`, `rfqrpc`, and the `tapchannelrpc` JSON
callbacks to the litclient's `Registrations` array. This allows the
litclient to use the rpc functions contained in these JSON callbacks.
Previously we'd consider it acceptable to settle direct rfq peer
invoices, which included no rfq scid, with asset HTLCs. This behavior
has been updated on the tapd invoice manager and we no longer accept
asset HTLCs on invoices that do not expect assets. This commit updates
such payments in our itests to instead expect a failure.
@GeorgeTsagk GeorgeTsagk force-pushed the strict-forwarding-p2-itest branch from 19fd405 to ec7a3e6 Compare December 16, 2024 14:22
@GeorgeTsagk
Copy link
Member Author

Updated go.mod deps & rebased
Waiting for CI

@guggero
Copy link
Member

guggero commented Dec 16, 2024

Just FYI: The itests broke due to the update to the latest tapd version on the base branch (update-to-lnd-18-4), will take a look. So the CI on this will likely also fail.

@guggero
Copy link
Member

guggero commented Dec 16, 2024

Ah, actually, this PR fixes the CI (which makes sense). So I cherry picked the relevant commit into the base branch (update-to-lnd-18-4) itself, which means this can be closed.

@guggero guggero closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants