Beat itest [2/3]: document and fix itest flakes #9307

yyforyongyu · 2024-11-26T09:40:19Z

Check #9306 for context, and #9260 for the final result.

Changes in each commit is pretty small, major changes are,

flatten some tests so it's easier to debug.
We encountered issue [bug]: listunspent shows spent outputs #8786 - which should have been caught way easier if we didn't use the standby nodes, and is now temporarily hacked. This will be my high-priority item to fix.
when checking for the sweeping tx, if the tx is RBFed, the old txid won't work.
connect to an already connected node can result in an error.
increase payment timeout for sql.
assert channel is in the grapb before sending payments.
assert outgoing and incoming HTLCs separately.
increase rpcmaxwebsockets for btcd.
document the MPP flake - we now have a much better understanding of how it happened.
some other minor refactors.

coderabbitai · 2024-11-26T09:40:25Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

github-actions · 2024-11-26T09:53:34Z

Pull reviewers stats

Stats of the last 30 days for lnd:

User	Total reviews	Time to review	Total comments
guggero 🥇	29 ▀▀▀▀	13h 1m	31 ▀▀
yyforyongyu 🥈	11 ▀	1d 1h 32m	28 ▀▀
ellemouton 🥉	11 ▀	16h 30m	21 ▀
ziggie1984	8 ▀	12h 47m	9 ▀
Roasbeef	7 ▀	4d 13h 56m ▀▀	47 ▀▀▀
ProofOfKeags	3	5d 4h 23m ▀▀	10 ▀
Abdulkbk	3	13h 37m	2
ViktorTigerstrom	2	2d 14h 26m ▀	5
alexbosworth	1	4d 11h 58m ▀▀	1
bhandras	1	12h	0
bitromortac	1	16h 16m	0

guggero

Amazing bug fixes, thank you so much! Crazy how many flakes you were to investigate and fix! You're my absolute hero 💯

itest/lnd_estimate_route_fee_test.go

ziggie1984

Thanks for this very laborious PR 👏 It is very time-expensive to study all these itest failures and fix them 🧹

ziggie1984 · 2024-12-18T11:11:33Z

itest/lnd_psbt_test.go

-// multisig funding output.
-func runPsbtChanFunding(ht *lntest.HarnessTest, carol, dave *node.HarnessNode,
-	private bool, commitType lnrpc.CommitmentType) {
+func runPsbtChanFundingWithNodes(ht *lntest.HarnessTest, carol,


ziggie1984 · 2024-12-18T11:37:57Z

itest/lnd_rest_api_test.go

@@ -506,6 +505,7 @@ func wsTestCaseBiDirectionalSubscription(ht *lntest.HarnessTest) {
 				}
 				return
 			}
+			ht.Log("Finish writing message")


Nit: Finished writing message maybe also more detail which message ?

ziggie1984 · 2024-12-18T11:43:43Z

itest/lnd_estimate_route_fee_test.go

@@ -376,7 +377,7 @@ func runFeeEstimationTestCase(ht *lntest.HarnessTest,
 		)
 		feeReq = &routerrpc.RouteFeeRequest{
 			PaymentRequest: payReqs[0],
-			Timeout:        10,
+			Timeout:        uint32(wait.PaymentTimeout.Seconds()),


what is the flake here tho, we are just increasing the timeout to 60s ?

yeah the previous 10s will cause the payment to time out, updated the commit msg

ziggie1984 · 2024-12-18T11:45:58Z

itest/lnd_channel_graph_test.go

@@ -237,17 +237,17 @@ func testUnannouncedChannels(ht *lntest.HarnessTest) {
 	ht.WaitForChannelOpenEvent(chanOpenUpdate)

 	// Alice should have 1 edge in her graph.
-	ht.AssertNumActiveEdges(alice, 1, true)
+	ht.AssertNumEdges(alice, 1, true)


Why does the windows build have no problem with this call tho, why are we not catching this nil case ?

yeah when running in windows the behavior is quite different, which is why we skip some tests for the window build in the final PR, that being said, we don't really know why windows has this issue, and we now have somewhat a giant TODO list for the windows build.

ziggie1984 · 2024-12-18T11:48:11Z

itest/lnd_psbt_test.go

@@ -269,6 +269,9 @@ func runPsbtChanFundingWithNodes(ht *lntest.HarnessTest, carol,
 	txHash := finalTx.TxHash()
 	block := ht.MineBlocksAndAssertNumTxes(6, 1)[0]
 	ht.AssertTxInBlock(block, txHash)
+
+	ht.AssertChannelActive(carol, chanPoint)


I wonder if we should pack the AssertChannelActive and AssertChannelInGraph in a new call so we never forget these two ?

good q - I think we need to do a proper cleanup of the assertion methods in lntest, to make them clear that which one is asserting the channeldb, and the graphdb. But think that will be something far in the future...

ziggie1984 · 2024-12-18T11:54:09Z

itest/lnd_mpp_test.go

+// `SendPaymentV2` case, where a payment amount of 300,000 sats is used and it
+// tests sending three attempts: the first has 150,000 sats, the rest two have
+// 75,000 sats. It returns the payment amt.
+func (c *mppTestScenario) setupSendPaymentCase() btcutil.Amount {


ziggie1984 · 2024-12-18T11:55:11Z

itest/lnd_mpp_test.go

@@ -378,6 +378,33 @@ func (c *mppTestScenario) setupSendPaymentCase() btcutil.Amount {
 	// - 2nd attempt(75,000 sats): Alice->Dave->Bob: 155,000 sats.
 	// - 3rd attempt(75,000 sats): Alice->Carol->Eve->Bob: 155,000 sats.
 	//
+	// There is a case where the payment will fail due to the channel
+	// capacity not being updated in the graph, which has been seen many


I think the channel capacity is a constant value so how can it be updated in the graph ?

updated to be bandwidth

ziggie1984 · 2024-12-18T11:57:08Z

lntest/harness.go

-	// Make sure the nodes know each other's channels if they are public.
-	if !p.Private {
+	// If the channels are private, make sure the channel participants know
+	// the relevant channel.


Nit: channel => channels ?

ziggie1984 · 2024-12-18T11:59:45Z

itest/lnd_multi-hop_force_close_test.go

+	// - the local one will be swept.
+	// - the remote one will be marked as failed due to `testmempoolaccept`
+	//   check.
+	// - the pending remote one will not be attempted due to it being


are you here talking about hte case after the commitment is confirmed (dangling anchor) ?

nope - before the FC is confirmed, the uneconomical inputs will not be attempted since we check them when making the input sets.

ziggie1984 · 2024-12-18T12:01:18Z

itest/lnd_mpp_test.go

+	//
+	// TODO(yy): Remove the following wait and use the direct call, then
+	// investigate the bug in the edge unifier.
+	var route *lnrpc.Route


so eventually it shows up after some time the missing edge ?

yeah, which is weird, think it's most likely due to graph being cached and not updated in time.

So it's easier to get the logs and debug.

We previously didn't see this issue because we always have nodes being over-funded.

So we know which open channel operation failed.

We need to mine an empty block as the tx may already have entered the mempool. This should be fixed once we start using the sweeper to handle the justice tx.

The reconnection will happen automatically when the nodes have a channel, so we just ensure the connection instead of reconnecting directly.

The test used 10s as the timeout value, which can easily cause a timeout in a slow build so we increase it to 60s.

So we won't forget to assert the topology after opening a chain of channels.

This is no longer needed since we don't have standby nodes, plus it's causing panic in windows build due to `edge.Policy` being nil.

This has been seen in the itest which can lead to the node startup failure, ``` 2024-11-20 18:55:15.727 [INF] RPCS: Max websocket clients exceeded [25] - disconnecting client 127.0.0.1:57224 ```

This is needed so we can have one place to fix the flakes found in the MPP-related tests, which is fixed in the following commit.

We now make sure the channel participants have heard their private channel when opening channels.

yyforyongyu added flake fix no-changelog labels Nov 26, 2024

yyforyongyu added this to the v0.19.0 milestone Nov 26, 2024

yyforyongyu self-assigned this Nov 26, 2024

yyforyongyu requested a review from guggero November 26, 2024 09:58

yyforyongyu added the no-itest label Nov 26, 2024

yyforyongyu force-pushed the yy-beat-itest-flakes branch from b1011ac to d8d2a54 Compare November 26, 2024 12:02

guggero approved these changes Dec 2, 2024

View reviewed changes

itest/lnd_estimate_route_fee_test.go Outdated Show resolved Hide resolved

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from c6819c0 to 3c60868 Compare December 3, 2024 22:08

yyforyongyu force-pushed the yy-beat-itest-flakes branch 2 times, most recently from a558fe3 to 3ac3b53 Compare December 3, 2024 22:13

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from 3c60868 to abda3ce Compare December 4, 2024 06:36

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 3ac3b53 to 8abca8c Compare December 4, 2024 06:37

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from abda3ce to 21082c5 Compare December 5, 2024 05:15

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 8abca8c to 6f59050 Compare December 5, 2024 05:15

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from 21082c5 to 9374711 Compare December 5, 2024 15:09

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 6f59050 to 9c24fc0 Compare December 5, 2024 15:09

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from 9374711 to e7beb0a Compare December 10, 2024 07:18

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 9c24fc0 to a6b616e Compare December 10, 2024 07:19

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from e7beb0a to d03a8b2 Compare December 12, 2024 12:14

yyforyongyu force-pushed the yy-beat-itest-flakes branch from a6b616e to 598f2a0 Compare December 12, 2024 12:14

ziggie1984 self-requested a review December 16, 2024 23:35

yyforyongyu force-pushed the yy-beat-itest-shuffle branch from d03a8b2 to dfa75eb Compare December 17, 2024 09:49

yyforyongyu mentioned this pull request Dec 17, 2024

Beat itest [3/3]: fix all itest flakes #9260

Merged

1 task

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 598f2a0 to e5cbddd Compare December 17, 2024 09:58

yyforyongyu changed the base branch from yy-beat-itest-shuffle to yy-waiting-on-merge December 17, 2024 10:45

yyforyongyu force-pushed the yy-waiting-on-merge branch from 8cfd618 to bde86dc Compare December 17, 2024 11:55

yyforyongyu mentioned this pull request Dec 17, 2024

Fix itest re new behaviors introduced by blockbeat #9368

Merged

yyforyongyu force-pushed the yy-beat-itest-flakes branch from e5cbddd to 064f494 Compare December 17, 2024 18:19

yyforyongyu force-pushed the yy-waiting-on-merge branch from bde86dc to 8e08948 Compare December 17, 2024 19:22

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 064f494 to 4e1822c Compare December 17, 2024 19:26

yyforyongyu force-pushed the yy-waiting-on-merge branch from 8e08948 to 890b841 Compare December 18, 2024 11:37

yyforyongyu added 2 commits December 18, 2024 19:38

itest: fix spawning temp miner

d72a0d5

itest: fix flake for neutrino backend

fdddd54

yyforyongyu force-pushed the yy-beat-itest-flakes branch from 4e1822c to d36937e Compare December 18, 2024 11:38

ziggie1984 approved these changes Dec 18, 2024

View reviewed changes

yyforyongyu added 18 commits December 19, 2024 00:02

itest: flatten PSBT funding test cases

c3cee82

So it's easier to get the logs and debug.

itest: fix and document flake in sweeping tests

6d1eb98

We previously didn't see this issue because we always have nodes being over-funded.

itest: remove loop in wsTestCaseBiDirectionalSubscription

8e171ce

So we know which open channel operation failed.

itest: fix flake in testRevokedCloseRetributionZeroValueRemoteOutput

220c71d

We need to mine an empty block as the tx may already have entered the mempool. This should be fixed once we start using the sweeper to handle the justice tx.

itest: fix flake in testSwitchOfflineDelivery

9d79d8d

The reconnection will happen automatically when the nodes have a channel, so we just ensure the connection instead of reconnecting directly.

itest+routing: fix flake in runFeeEstimationTestCase

b73fecc

The test used 10s as the timeout value, which can easily cause a timeout in a slow build so we increase it to 60s.

itest: use ht.CreateSimpleNetwork whenever applicable

46ec0bc

So we won't forget to assert the topology after opening a chain of channels.

itest: put mpp tests in one file

dc1d0c7

lntest+itest: remove AssertNumActiveEdges

1ba41fc

This is no longer needed since we don't have standby nodes, plus it's causing panic in windows build due to `edge.Policy` being nil.

itest: fix flake in runPsbtChanFundingWithNodes

4d89b40

itest: fix flake in testPrivateUpdateAlias

cb47db2

itest: fix flake in update_pending_open_channels

eea6671

lntest: increase rpcmaxwebsockets for btcd

da77c84

This has been seen in the itest which can lead to the node startup failure, ``` 2024-11-20 18:55:15.727 [INF] RPCS: Max websocket clients exceeded [25] - disconnecting client 127.0.0.1:57224 ```

itest: document details about MPP-related tests

76dab6a

This is needed so we can have one place to fix the flakes found in the MPP-related tests, which is fixed in the following commit.

itest+lntest: fix flake in MPP-related tests

dadceec

lntest: fix flakeness in openChannelsForNodes

097239e

We now make sure the channel participants have heard their private channel when opening channels.

itest: document a rare flake found in macOS

86365c2

itest: document a flake found in SendToRoute

55b40e2

yyforyongyu force-pushed the yy-beat-itest-flakes branch from d36937e to 55b40e2 Compare December 18, 2024 16:21

yyforyongyu merged commit 4db3060 into yy-waiting-on-merge Dec 18, 2024
21 of 23 checks passed

yyforyongyu deleted the yy-beat-itest-flakes branch December 18, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beat itest [2/3]: document and fix itest flakes #9307

Beat itest [2/3]: document and fix itest flakes #9307

yyforyongyu commented Nov 26, 2024

coderabbitai bot commented Nov 26, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

github-actions bot commented Nov 26, 2024

guggero left a comment

ziggie1984 left a comment

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

ziggie1984 Dec 18, 2024

yyforyongyu Dec 18, 2024

Beat itest [2/3]: document and fix itest flakes #9307

Beat itest [2/3]: document and fix itest flakes #9307

Conversation

yyforyongyu commented Nov 26, 2024

coderabbitai bot commented Nov 26, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

github-actions bot commented Nov 26, 2024

Pull reviewers stats

guggero left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot commented Nov 26, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)