Beat [0/4]: improve itest miner by yyforyongyu · Pull Request #8892 · lightningnetwork/lnd

yyforyongyu · 2024-07-04T12:50:30Z

This PR refactors the mining methods to make them easier to use. In specific,

Strictly defined MineBlocks, MineEmptyBlocks and MineBlocksAndAssertNumTxes methods, and instructions on how to use them are now shown in the returned error message.
Downgrade HarnessMiner to be a private instance, as previously we've seen a mixed usage of ht.MineBlocksAndAssertNumTxes(1, 1) and ht.Miner.MineBlocksAndAssertNumTxes(1, 1), causing confusion for devs. All the miner-related methods are now moved into a single file, and can only be accessed via ht.
Start tracking the best height in ht and it can be accessed via ht.CurrentHeight(), which is helpful in tests that heavily rely on the block height, e.g., the multi-hop tests.

coderabbitai · 2024-07-04T12:50:35Z

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>.
- 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 generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @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 as 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 resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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

Very nice refactor 💯

Would be nice to get a green run for Neutrino, since we change some of the behavior of that backend in this PR.

yyforyongyu · 2024-07-11T09:28:11Z

~~Shuffled the test cases to validate an assumption, let's see how it goes.~~

ellemouton

Really great work! 🔥

Pretty much LGTM - agreed with Oli re trying to catch the rest of the neutrino Mempool errors here though

ellemouton · 2024-07-12T08:28:01Z

-// NOTE: this differs from miner's `MineBlocks` as it requires the nodes to be
-// synced.
-func (h *HarnessTest) MineBlocks(num uint32) []*wire.MsgBlock {
-	require.Less(h, num, uint32(maxBlocksAllowed),
-		"too many blocks to mine")
+// NOTE: Use `MineBlocksAndAssertNumTxes` if you expect txns in the blocks. Use
+// `MineEmptyBlocks` if you want to keep the txns stay unconfirmed.


nice 🙏

wow yeah "this differs from miner's `MineBlocks" is very confusing 😂

nit: s/"if you want to keep the txns stay unconfirmed"/"if you want to make sure that txns stay unconfirmed".

unaddressed :)

ellemouton · 2024-07-12T08:31:55Z

+
+		desc += "Consider using `MineBlocksAndAssertNumTxes` if you " +
+			"expect txns, or `MineEmptyBlocks` if you want to " +
+			"keep the txns stay unconfirmed."


nit: "if you want to keep the txns unconfirmed" or "if you want to ensure that the txns stay unconfirmed"

ellemouton · 2024-07-12T08:56:13Z

+
+// CurrentHeight returns the current block height.
+func (h *HarnessTest) CurrentHeight() uint32 {
+	return h.currentHeight


should we guard this or do we expect the Mine* methods to always be called synchronously with other calls?

yeah it should always be called in the same goroutine.

yyforyongyu · 2024-07-18T05:49:51Z

We now are seeing two failed tests,

The first happens in bitcoind-postgres, which gives a new error I've never seen before,

2024-07-18 01:48:45.890 UTC [1186] LOG:  unexpected EOF on client connection with an open transaction
2024-07-18 01:48:45.890 UTC [1161] LOG:  unexpected EOF on client connection with an open transaction
2024-07-18 01:48:45.892 UTC [1197] LOG:  unexpected EOF on client connection with an open transaction
    harness_rpc.go:97: 
        	Error Trace:	/home/runner/work/lnd/lnd/lntest/rpc/harness_rpc.go:97
        	            				/home/runner/work/lnd/lnd/lntest/rpc/lnd.go:84
        	            				/home/runner/work/lnd/lnd/lntest/harness_assertion.go:50
        	            				/home/runner/work/lnd/lnd/lntest/wait/wait.go:51
        	            				/home/runner/work/lnd/lnd/lntest/wait/wait.go:27
        	            				/opt/hostedtoolcache/go/1.22.3/x64/src/runtime/asm_amd64.s:1695
        	Error:      	Received unexpected error:
        	            	rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing: dial tcp 127.0.0.1:10198: connect: connection refused"
        	Messages:   	Bob: failed to call GetInfo

Will check the logs and create an issue page to track it.

The second is in neutrino, which is exactly what the blockbeat aims to fix - we get this error because the blocks are not synced, causing one sweeping tx happens before another, while they are supposed to happen in the same block.

lnd_channel_force_close_test.go:743: 
        	Error Trace:	/home/runner/work/lnd/lnd/itest/lnd_channel_force_close_test.go:743
        	            				/home/runner/work/lnd/lnd/itest/lnd_channel_force_close_test.go:84
        	Error:      	"[0xc0011da8a0]" should have 6 item(s), but has 1
        	Test:       	TestLightningNetworkDaemon/tranche02/53-of-156/neutrino/channel_force_closure/committype=SIMPLE_TAPROOT
        	Messages:   	htlc transaction has wrong num of inputs
    harness.go:440: finished test: channel_force_closure, start height=689, end height=785, mined blocks=96
    harness.go:446: test failed, skipped cleanup

ellemouton

LGTM 🧹 🔥

one unaddressed tiny nit :)

guggero · 2024-07-18T07:09:45Z

For the postgres one, seems like Bob isn't starting up anymore because it's crashing:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x65b0b2]

goroutine 3685 [running]:
github.com/btcsuite/btcd/wire.(*MsgTx).Copy(0xc00270f1c0)
	/home/runner/go/pkg/mod/github.com/btcsuite/btcd@v0.24.2-beta.rc1.0.20240625142744-cc26860b4026/wire/msgtx.go:455 +0x512
github.com/lightninglabs/neutrino/pushtx.(*Broadcaster).broadcastHandler.func1()
	/home/runner/go/pkg/mod/github.com/lightninglabs/neutrino@v0.16.1-0.20240425105051-602843d34ffd/pushtx/broadcaster.go:160 +0xf0
github.com/lightninglabs/neutrino/pushtx.(*Broadcaster).broadcastHandler(0xc0006084e0, 0xc001a5ee90)
	/home/runner/go/pkg/mod/github.com/lightninglabs/neutrino@v0.16.1-0.20240425105051-602843d34ffd/pushtx/broadcaster.go:211 +0x2e6
created by github.com/lightninglabs/neutrino/pushtx.(*Broadcaster).Start.func1 in goroutine 2611
	/home/runner/go/pkg/mod/github.com/lightninglabs/neutrino@v0.16.1-0.20240425105051-602843d34ffd/pushtx/broadcaster.go:109 +0xfd

guggero

Nice, the Neutrino test is now green 🟢

Although I have one question about a skipped test... Other than that, LGTM 🎉

guggero · 2024-07-23T07:15:35Z

 // testUnconfirmedChannelFunding tests that our unconfirmed change outputs can
 // be used to fund channels.
 func testUnconfirmedChannelFunding(ht *lntest.HarnessTest) {
+	if ht.IsNeutrinoBackend() {


Are we planning on resurrecting this test after some of the other changes in the PR series?
I understand that it's a bit harder to set up for Neutrino since we don't see any unconfirmed balance.

But couldn't we send some coins to the wallet, confirm those, then do a self-transfer that is unconfirmed (that way even Neutrino nodes will see the TX as they've created it themselves), then open a channel from that?

I think it might be good to keep the test if possible since this scenario is one that seems to happen some times in the wild.

I like it! Updated as suggested!

I don't see a push of new commits since your comment?

This commit moves the `HarnessMiner` into a new package to avoid confusion about incoming changes.

This commit adds more assertion to `MineBlocks` so the caller won't misuse it.

in harness Prepare to make `HarnessTest.Miner` a private instance to sync height.

`MineBlocksAndAssertNumTxes`

The nodes are already shut down in the `Cleanup` in `ht.Subtest` so there's no need to shutdown them again.

This commit makes sure `missionControlStore` catches the shutdown signal when draining the ticker. A few debug logs are added to aid the process.

This commit makes sure the sweep requests are received before mining blocks to trigger the actual sweeping. In addition, `testFundingExpiryBlocksOnPending` is updated to deal with the old `channel link not found` issue.

This test was previously working because we'd mine an extra block to confirm the coins inside `FundCoinsUnconfirmed` when it's a neutrino backend, as shown in https://github.com/lightningnetwork/lnd/blob/fdd28c8d888792ea8fde3c557ba9f2594e0a6ec8/lntest/harness.go#L1431 Since neutrino has trouble seeing unconfirmed balance, we now send some coins to the wallet, confirm those, then do a self-transfer so the node will have unconfirmed outputs to perform the test.

guggero

Very nice, LGTM 🎉

Will merge once CI run is complete.

guggero · 2024-07-23T18:12:26Z

Moooar green CI ❤️
The remaining unit test failure (send coverage) will be fixed by #8927.

yyforyongyu added utxo sweeping itests Issues related to integration tests. labels Jul 4, 2024

yyforyongyu added this to the v0.18.2 milestone Jul 4, 2024

yyforyongyu self-assigned this Jul 4, 2024

yyforyongyu force-pushed the yy-itest-miner branch from 2be0eb1 to 0339928 Compare July 4, 2024 12:51

yyforyongyu changed the title ~~Beat: [0/3]: improve itest miner~~ Beat [0/3]: improve itest miner Jul 4, 2024

yyforyongyu mentioned this pull request Jul 4, 2024

Beat [1/4]: handle sweeper's broadcast error #8893

Merged

guggero reviewed Jul 8, 2024

View reviewed changes

Comment thread itest/lnd_multi-hop_test.go Outdated

yyforyongyu force-pushed the yy-itest-miner branch from 0339928 to 263006e Compare July 11, 2024 09:27

yyforyongyu force-pushed the yy-itest-miner branch from 263006e to 4630d4f Compare July 11, 2024 09:39

saubyk requested a review from ellemouton July 11, 2024 15:17

ellemouton reviewed Jul 12, 2024

View reviewed changes

saubyk added the P1 MUST be fixed or reviewed label Jul 15, 2024

yyforyongyu force-pushed the yy-itest-miner branch 2 times, most recently from 388981d to adca500 Compare July 15, 2024 18:02

yyforyongyu mentioned this pull request Jul 15, 2024

routing: lnd cannot shutdown due to MissionControl #8912

Closed

yyforyongyu force-pushed the yy-itest-miner branch 3 times, most recently from 0d6d233 to aa7013c Compare July 17, 2024 02:50

guggero mentioned this pull request Jul 17, 2024

chanbackup, server, rpcserver: put close unsigned tx, remote signature and commit height to SCB #8183

Merged

14 tasks

yyforyongyu force-pushed the yy-itest-miner branch 2 times, most recently from 1e898e2 to 4548971 Compare July 18, 2024 01:36

yyforyongyu changed the title ~~Beat [0/3]: improve itest miner~~ Beat [0/4]: improve itest miner Jul 18, 2024

yyforyongyu requested review from ellemouton and guggero July 18, 2024 05:50

ellemouton approved these changes Jul 18, 2024

View reviewed changes

yyforyongyu force-pushed the yy-itest-miner branch from 4548971 to e0ec893 Compare July 22, 2024 14:10

guggero approved these changes Jul 23, 2024

View reviewed changes

yyforyongyu and others added 15 commits July 23, 2024 21:30

lntest: create new package lntest/miner for itest miner

a881477

This commit moves the `HarnessMiner` into a new package to avoid confusion about incoming changes.

lntest: move mining methods into one file

91b20e6

lntest+itest: strictly define the behavior of MineBlocks

e553895

This commit adds more assertion to `MineBlocks` so the caller won't misuse it.

lntest+itest: add method AssertNumTxsInMempool and AssertTxInBlock

976bb37

in harness Prepare to make `HarnessTest.Miner` a private instance to sync height.

lntest+itest: continue removing direct call to Miner

6bd8bae

lntest+itest: finalize moving miner methods

be4dba5

lntest+itest: make Miner a private instance

14e7b13

lntest+itest: add new method CurrentHeight

f1f3410

itest: fix misuse of MineBlocks and replace it with

8240a87

`MineBlocksAndAssertNumTxes`

lntest: remove redundant nodes shutdown

623f816

The nodes are already shut down in the `Cleanup` in `ht.Subtest` so there's no need to shutdown them again.

itest: remove unused param in chanRestoreViaRPC

5027946

multi: make sure missionControlStore catches done signal

2608c08

This commit makes sure `missionControlStore` catches the shutdown signal when draining the ticker. A few debug logs are added to aid the process.

lntest+itest: fix flakes found using neutrino backend

4dcce9d

This commit makes sure the sweep requests are received before mining blocks to trigger the actual sweeping. In addition, `testFundingExpiryBlocksOnPending` is updated to deal with the old `channel link not found` issue.

docs: update release notes

2d21aa3

yyforyongyu requested a review from guggero July 23, 2024 16:44

yyforyongyu force-pushed the yy-itest-miner branch from e0ec893 to 9180762 Compare July 23, 2024 16:49

guggero approved these changes Jul 23, 2024

View reviewed changes

guggero merged commit f27f9f2 into master Jul 23, 2024

guggero deleted the yy-itest-miner branch July 23, 2024 18:12

guggero restored the yy-itest-miner branch July 23, 2024 18:13

guggero deleted the yy-itest-miner branch July 23, 2024 18:13

guggero restored the yy-itest-miner branch July 23, 2024 18:14

yyforyongyu deleted the yy-itest-miner branch July 23, 2024 18:14

yyforyongyu restored the yy-itest-miner branch July 23, 2024 18:17

yyforyongyu deleted the yy-itest-miner branch July 23, 2024 18:19

Conversation

yyforyongyu commented Jul 4, 2024

Uh oh!

coderabbitai Bot commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yyforyongyu commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yyforyongyu commented Jul 18, 2024

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

guggero commented Jul 18, 2024

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!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

guggero commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai Bot commented Jul 4, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

yyforyongyu commented Jul 11, 2024 •

edited

Loading