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]: missing itest for funding timeout case of 2016 blocks #9511

Open
Crypt-iQ opened this issue Feb 12, 2025 · 6 comments
Open

[itest]: missing itest for funding timeout case of 2016 blocks #9511

Crypt-iQ opened this issue Feb 12, 2025 · 6 comments
Labels
good first issue Issues suitable for first time contributors to LND intermediate Issues suitable for developers moderately familiar with the codebase and LN itests Issues related to integration tests. testing Improvements/modifications to the test suite

Comments

@Crypt-iQ
Copy link
Collaborator

Noticed this when looking through the code. We should be able to modify this constant

lnd/funding/manager.go

Lines 104 to 107 in 693b399

// MaxWaitNumBlocksFundingConf is the maximum number of blocks to wait
// for the funding transaction to be confirmed before forgetting
// channels that aren't initiated by us. 2016 blocks is ~2 weeks.
MaxWaitNumBlocksFundingConf = 2016
in the itests so that we can mine very few blocks and trigger the timeout case.

@guggero guggero added intermediate Issues suitable for developers moderately familiar with the codebase and LN testing Improvements/modifications to the test suite good first issue Issues suitable for first time contributors to LND itests Issues related to integration tests. and removed no-itest labels Feb 12, 2025
@morehouse
Copy link
Collaborator

I worked an an itest for this a few years ago: #7228

That work got stalled waiting on #7241, and I never picked it up again.

@yyforyongyu
Copy link
Member

Yeah would be nice to revive #7228, plus I don't think we need to mine that many blocks in the itest, this value can be changed using a build tag such that for integration this can be set to a much smaller value.

@Rakshitsinghhh
Copy link

Yeah would be nice to revive #7228, plus I don't think we need to mine that many blocks in the itest, this value can be changed using a build tag such that for integration this can be set to a much smaller value.

That makes sense! I was thinking of using a build tag to override MaxWaitNumBlocksFundingConf for integration tests, reducing it to a much lower value so we don’t have to mine too many blocks while still triggering the timeout case. Would this approach be a good way to minimize block mining?

@guggero
Copy link
Collaborator

guggero commented Feb 14, 2025

You can make it a configurable value, but expose the flag to actually configure it only in dev/integration test builds by adding it here:

type DevConfig struct {

@Rakshitsinghhh
Copy link

You can make it a configurable value, but expose the flag to actually configure it only in dev/integration test builds by adding it here:

lnd/lncfg/dev_integration.go

Line 23 in ab7aae0

type DevConfig struct {

Is this what you meant—making MaxWaitNumBlocksFundingConf configurable in DevConfig and exposing a flag only for dev/integration builds while keeping 2016 as the default for production?

@NishantBansal2003
Copy link
Contributor

Could I be assigned to this issue if it's available to work on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues suitable for first time contributors to LND intermediate Issues suitable for developers moderately familiar with the codebase and LN itests Issues related to integration tests. testing Improvements/modifications to the test suite
Projects
None yet
Development

No branches or pull requests

6 participants