Fix unit test flake `TestHistoricalConfDetailsTxIndex` by yyforyongyu · Pull Request #9549 · lightningnetwork/lnd

yyforyongyu · 2025-02-25T09:09:51Z

Fix a long standing flake found in our unit test,

--- FAIL: TestHistoricalConfDetailsTxIndex (0.00s)
    --- FAIL: TestHistoricalConfDetailsTxIndex/rpc_polling_disabled (11.66s)
        bitcoind_test.go:143: miner height=430, bitcoind height=0
        ...
        bitcoind_test.go:143: miner height=430, bitcoind height=0
        bitcoind_test.go:143: timed out in syncNotifierWithMiner, got err=<nil>, minerHeight=430, bitcoindHeight=0
FAIL
FAIL	github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify	13.353s
FAIL

It turns out the peer connection between the bitcoind node and the btcd node (miner) is broken, causing the chain backend to never be synced. This unexpected behavior only happens when the test runs multiple times (ranging from 50-ish to 200-ish). To reproduce, remove the second-to-last commit and run,

pkg=chainntnfs/bitcoindnotify
case=TestHistoricalConfDetailsNoTxIndex/rpc_polling_enabled
make flakehunter-unit pkg=$pkg case=$case

coderabbitai · 2025-02-25T09:09:59Z

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.
@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.

guggero

Thanks a lot for investigating!
The changes look good to me 💯

Interestingly enough it looks like TestFilteredChainView in routing/chainview now fails constantly. Perhaps we were able to turn a flake into an error and with that be able to fix two things at once? Which would be a huge win.

guggero · 2025-02-25T11:40:52Z


-#? flakehunter-parallel: Run the integration tests continuously until one fails, running up to ITEST_PARALLELISM test tranches in parallel (default 4)
-flakehunter-parallel:
+	# while [ $$? -eq 0 ]; do GOTRACEBACK=all $(UNIT) -count=1; done


nit: commented out code.

guggero · 2025-02-25T11:43:29Z

+counter=0
+
+# Run the command in a loop until it fails.
+while output=$(go clean -testcache | make unit-debug log="stdlog trace" pkg=$pkg case=$case timeout=$timeout 2>&1); do


Why do we pipe the output of go clean into the make command? Shouldn't that just be &&?

guggero · 2025-02-25T11:45:58Z


 	var conn *chain.BitcoindConn
-	err := wait.NoError(func() error {
+	err = wait.NoError(func() error {


nit: belongs to previous commit.

guggero · 2025-02-25T11:48:27Z

 	"fmt"
 	"os/exec"
 	"path/filepath"
+	"strings"


nit: belongs into the next commit.

This commit adds a script to hunt flakes for a specific unit test with trace logs. Also rename the make commands to make them more clear on whether it's a unit test, itest, or paralleled itest.

The config used for the miner is updated to skip banning and debug. For bitcoind, the config is updated to use a unique port for P2P conn.

Make sure the shutdown of `bitcoind` is finished without any errors.

We change how the `bitcoind` node connects to the miner by creating a temp RPC client and use it to connect to the miner. In addition we also assert the connection is made.

yyforyongyu · 2025-02-25T14:20:17Z

Interestingly enough it looks like TestFilteredChainView in routing/chainview now fails constantly. Perhaps we were able to turn a flake into an error and with that be able to fix two things at once? Which would be a huge win.

Yeah glad they are now at least deterministic - pushed a fix, let's see if it passes.

guggero

Awesome fixed, thanks a lot!

guggero · 2025-02-26T09:35:27Z

+	// dedicated miner to generate blocks, cause re-orgs, etc. We'll set up
+	// this node with a chain length of 125, so we have plenty of BTC to
+	// play around with.
+	node := unittest.NewMiner(t, netParams, []string{"--txindex"}, true, 25)


nit: extract this code block into a function?

ellemouton

Epic epic epic!!! Very nice investigation & beautiful PR ✨
lgtm after linter fix

ellemouton · 2025-02-26T11:40:33Z

-	@$(call print, "Flake hunting unit tests.")
-	while [ $$? -eq 0 ]; do GOTRACEBACK=all $(UNIT) -count=1; done
+#? flakehunter-unit: Run the unit tests continuously until one fails
+flakehunter-unit:


so nice 🔥

ellemouton · 2025-02-26T11:43:40Z

 	}
 	t.Cleanup(conn.Stop)

+	// Assert that the connection with the miner is made.


ellemouton · 2025-02-26T11:45:23Z

+		// Reconnect the miner and the chain backend.
+		//
+		// NOTE: The connection should have been made before we perform
+		// the `syncNotifierWithMiner`. However, due unknown reason, the


nit: s/due unknown reason/due to an unknown reason

In this commit we document an unexpected behavior found when connecting a bitcoind node to a btcd node. We mitigate this in our test by reconnecting the nodes when the connection is broken. We also limit the connection made from `bitcoind` to be v1 only.

Prepares for the following commit where we assert the chain backend is synced to the miner.

This commit makes sure the bitcoind node is synced to the miner when initialized and returned from `NewBitcoindBackend`.

So each test has its own miner and chainView.

yyforyongyu added flake fix size/micro small bug fix or feature, less than 15 mins of review, less than 250 labels Feb 25, 2025

yyforyongyu added this to the v0.19.0 milestone Feb 25, 2025

yyforyongyu self-assigned this Feb 25, 2025

yyforyongyu force-pushed the fix-bitcond-test branch from 5eecae6 to 0efa79e Compare February 25, 2025 09:11

guggero reviewed Feb 25, 2025

View reviewed changes

yyforyongyu added 4 commits February 25, 2025 21:10

Makefile+scripts: add unit test flake hunter

fa8527a

This commit adds a script to hunt flakes for a specific unit test with trace logs. Also rename the make commands to make them more clear on whether it's a unit test, itest, or paralleled itest.

lntest/unittest: update config for miner and bitcoind

7666d62

The config used for the miner is updated to skip banning and debug. For bitcoind, the config is updated to use a unique port for P2P conn.

lntest/unittest: assert bitcoind is shut down

245ea85

Make sure the shutdown of `bitcoind` is finished without any errors.

lntest/unittest: make sure miner is connected to bitcoind

1753016

We change how the `bitcoind` node connects to the miner by creating a temp RPC client and use it to connect to the miner. In addition we also assert the connection is made.

yyforyongyu force-pushed the fix-bitcond-test branch from 0efa79e to 7ec907a Compare February 25, 2025 14:16

yyforyongyu force-pushed the fix-bitcond-test branch from 46a52b6 to c1062c2 Compare February 25, 2025 21:28

yyforyongyu requested review from ellemouton and guggero February 26, 2025 08:49

guggero approved these changes Feb 26, 2025

View reviewed changes

yyforyongyu force-pushed the fix-bitcond-test branch from c1062c2 to e2c9e58 Compare February 26, 2025 10:20

ellemouton approved these changes Feb 26, 2025

View reviewed changes

yyforyongyu added 6 commits February 26, 2025 19:51

docs: update release notes re flake fix

c725ba9

multi: change NewBitcoindBackend to take a miner as its param

99d49de

Prepares for the following commit where we assert the chain backend is synced to the miner.

lntest+chainntnfs: make sure bitcoind node is synced

4bfcfea

This commit makes sure the bitcoind node is synced to the miner when initialized and returned from `NewBitcoindBackend`.

routing/chainview: refactor TestFilteredChainView

56fa3aa

So each test has its own miner and chainView.

routing: fix flake in TestFilteredChainView/bitcoind_polling

cfa4341

yyforyongyu force-pushed the fix-bitcond-test branch from e2c9e58 to cfa4341 Compare February 26, 2025 11:51

guggero merged commit e3d9fcb into lightningnetwork:master Feb 26, 2025

yyforyongyu deleted the fix-bitcond-test branch February 26, 2025 13:33

This was referenced Feb 27, 2025

References to known test flakes and planned fixes #8842

Closed

chainntfn: unable to get missed blocks: starting height is greater than ending height #8108

Closed

Uh oh!

Conversation

yyforyongyu commented Feb 25, 2025

Uh oh!

coderabbitai Bot commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

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!

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!

yyforyongyu commented Feb 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!

ellemouton 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Feb 25, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)