GitHub: use bitcoind v26.0 for CI #8273

guggero · 2023-12-12T19:58:58Z

Depends on #8555.

Change Description

See if things break in the CI if we update to bitcoind v26.0.

.github/workflows/main.yml

guggero · 2023-12-18T17:41:54Z

Okay, so the failed unit tests were due to a number of reasons:

bitcoind v26.0 doesn't have support for Unix Sockets (ipc://) in ZMQ anymore. Broken by Validate port-options bitcoin/bitcoin#22087, will be fixed by ZMQ: Support UNIX domain sockets bitcoin/bitcoin#27679
We had frequent TCP listening port collisions, at least when running the tests locally
We didn't clean up temporary directories for bitcoind sometimes

I added a couple of commits to address these issues.

yyforyongyu

Thanks for the cleanup! No major comments, just need to merge the dependent PR first. And the unit tests need to be fixed.

scripts/install_bitcoind.sh

chainntnfs/test_utils.go

lnwallet/test/test_interface.go

Roasbeef

LGTM 🎨

go.mod

chainntnfs/test_utils.go

lnwallet/test/test_interface.go

yyforyongyu · 2023-12-21T23:45:12Z

Unit tests still failing. One of the itest failures has uncleaned shutdown that still occupying a port, werid

 unable to create server: listen tcp 127.0.0.1:6563: bind: address already in use

guggero · 2023-12-22T07:13:11Z

Yeah, it seems like I have to do some more digging into failures.

yyforyongyu · 2024-01-16T11:43:58Z

scripts/install_bitcoind.sh

@@ -2,9 +2,9 @@

 set -ev

-BITCOIND_VERSION=${BITCOIN_VERSION:-25.0}
+BITCOIND_VERSION=${BITCOIN_VERSION:-25}


I think we should use one place to manage the version, to avoid mistakes such as 994a142

Maybe this is a dumb question but why isn't this number 26?

This is just the fallback version in case no environment variable is set. We set the version we actually want here:

lnd/.github/workflows/main.yml

Line 24 in ff3e8da

BITCOIN_VERSION: "26"

But I guess we shouldn't do that to avoid confusion...

coderabbitai · 2024-02-01T15:40:28Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

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.

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-tests 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 tests 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 tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

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

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ProofOfKeags

I pulled this down and tested it and it looks like everything passes except a wtclient test (possibly a flake?). It doesn't seem related to this as best I can tell. I think that test will pass if you rebase this.

ProofOfKeags · 2024-02-01T18:12:48Z

scripts/install_bitcoind.sh

@@ -2,9 +2,9 @@

 set -ev

-BITCOIND_VERSION=${BITCOIN_VERSION:-25.0}
+BITCOIND_VERSION=${BITCOIN_VERSION:-25}


Maybe this is a dumb question but why isn't this number 26?

guggero · 2024-02-02T17:38:34Z

@Roasbeef can you see the logs of the failed Travis build?
I only get this error message (not sure if we need to re-auth the app somehow?):

To make it less confusing what version of bitcoind is actually installed, we now require the version to be specified as a command line argument. Because we tag the version in the docker image tag as bitcoin-core:XX but the binary internally is located under /opt/bitcoin-XX.Y/, we manually set Y to 0.

With this commit we create a new function that returns system wide unique ports by using a single file to keep track of previously used ports. We'll want to use this everywhere whenever we need to listen on a new, random port during unit or integration tests. Because we now have a unique source, we don't need to apply the port offset that was used for the different tranches of parallel running integration tests before.

Make sure we wait for the initial connection long enough.

This commit fixes a number of issues with our temporary bitcoind nodes that we spin up for unit tests: - We didn't remove the node's temporary data dir after tests finished. - We used random ports which lead to unwanted port collisions. - We used ipc:// unix sockets for ZMQ which currently isn't supported in bitcoind v26.0 due to a regression. Since we can reliably create new non-colliding ports now, we should use TCP for ZMQ anyway.

With the chainntnfs.NewMiner now being optimized for not creating nodes with colliding ports, we use it in all unit tests that spin up temporary miners.

To avoid circular dependency issues between packages, we move the unit test backend creation function to a new package in the lntest parent package.

Since we fixed a number of issues in chainntnfs.NewBitcoindBackend that makes it compatible with bitcoind v26.0, we now want to use that function in all our unit tests.

To make it easy to add "-test.v" and "-test.count=1" to the unit tests, we add two new make flags.

This is a commit to attempt to fix Travis which runs on ARM64.

Because we pass in the same config into multiple notifiers, we sometimes get a data race in the unit tests. By creating a copy of the config we avoid that.

guggero · 2024-03-18T20:32:47Z

Okay, this is as green as it will probably ever get. There are still a couple of (pre-existing!) flakes in there. But I vote to get this merged since they very likely don't have anything to do with bitcoind.

yyforyongyu

LGTM, thanks for the fix🙏

.github/workflows/main.yml

yyforyongyu · 2024-03-19T19:36:57Z

btw can we update our merge rules to exclude the build from macOS?

ProofOfKeags

🤝

guggero · 2024-03-20T07:49:50Z

btw can we update our merge rules to exclude the build from macOS?

It's currently not included in the list of required tests. So merges shouldn't be blocked on it (same for Windows).

pinheadmz · 2024-04-18T13:47:50Z

FWIW bitcoin/bitcoin#27679 is updated, has two ACKs already, and a review from an LND dev would be helpful to get a merge ;-)

calvinrzachman reviewed Dec 12, 2023

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

guggero force-pushed the bitcoind-26 branch 3 times, most recently from 586e50c to 1c70744 Compare December 18, 2023 17:20

guggero marked this pull request as ready for review December 18, 2023 17:42

guggero requested a review from yyforyongyu December 18, 2023 17:44

yyforyongyu reviewed Dec 19, 2023

View reviewed changes

scripts/install_bitcoind.sh Show resolved Hide resolved

chainntnfs/test_utils.go Outdated Show resolved Hide resolved

lnwallet/test/test_interface.go Outdated Show resolved Hide resolved

Roasbeef approved these changes Dec 20, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

chainntnfs/test_utils.go Show resolved Hide resolved

lnwallet/test/test_interface.go Outdated Show resolved Hide resolved

guggero force-pushed the bitcoind-26 branch 3 times, most recently from 488202b to 57aca62 Compare December 21, 2023 08:48

guggero force-pushed the bitcoind-26 branch from 57aca62 to f800b54 Compare December 22, 2023 10:58

guggero mentioned this pull request Jan 9, 2024

chainntnfs: increase timeout in unit tests #8360

Closed

yyforyongyu mentioned this pull request Jan 16, 2024

CI: upgrade bitcoind version to 25 #8386

Closed

yyforyongyu reviewed Jan 16, 2024

View reviewed changes

yyforyongyu mentioned this pull request Jan 18, 2024

itest: got error bind: address already in use #8395

Open

guggero mentioned this pull request Feb 1, 2024

Bugfix/bitcoind zmq sockets #8450

Closed

8 tasks

saubyk requested a review from ProofOfKeags February 1, 2024 15:32

saubyk assigned guggero Feb 1, 2024

saubyk added bitcoind Bitcoin Core backend CI continuous integration labels Feb 1, 2024

guggero force-pushed the bitcoind-26 branch from f800b54 to 0963afc Compare February 1, 2024 15:40

ProofOfKeags approved these changes Feb 1, 2024

View reviewed changes

yyforyongyu added the no-changelog label Feb 2, 2024

guggero force-pushed the bitcoind-26 branch from 0963afc to d1e545b Compare February 2, 2024 16:41

guggero added 8 commits March 18, 2024 16:13

chainntnfs: add netParams arg to backend funcs

11ba14a

chainntnfs: re-try miner connection more often

2cc28ba

Make sure we wait for the initial connection long enough.

multi: use chainntnfs.NewMiner for miners in unit tests

c170a98

With the chainntnfs.NewMiner now being optimized for not creating nodes with colliding ports, we use it in all unit tests that spin up temporary miners.

multi: move unit test backend funcs to new package

d40312c

To avoid circular dependency issues between packages, we move the unit test backend creation function to a new package in the lntest parent package.

lnwallet+routing: use chainntnfs.NewBitcoindBackend

aa811c7

Since we fixed a number of issues in chainntnfs.NewBitcoindBackend that makes it compatible with bitcoind v26.0, we now want to use that function in all our unit tests.

guggero force-pushed the bitcoind-26 branch from c59f6cf to 7a91ff6 Compare March 18, 2024 15:14

guggero added 5 commits March 18, 2024 17:07

make: add nocache and verbose arguments

b1b32d9

To make it easy to add "-test.v" and "-test.count=1" to the unit tests, we add two new make flags.

docs: add release notes

77a8230

lntest: give chain backend more time to get ready

ab422ba

This is a commit to attempt to fix Travis which runs on ARM64.

chainntnfs: fix race unit test issue

17b93db

Because we pass in the same config into multiple notifiers, we sometimes get a data race in the unit tests. By creating a copy of the config we avoid that.

Travis+GitHub: replace Travis CI with GitHub MacOS 14 build

f6656d1

guggero force-pushed the bitcoind-26 branch from 7a91ff6 to f6656d1 Compare March 18, 2024 16:08

guggero requested review from ProofOfKeags, Roasbeef and yyforyongyu and removed request for Roasbeef March 18, 2024 20:32

yyforyongyu added this to the v0.18.0 milestone Mar 19, 2024

yyforyongyu approved these changes Mar 19, 2024

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

ProofOfKeags approved these changes Mar 19, 2024

View reviewed changes

guggero removed the no-changelog label Mar 20, 2024

guggero merged commit ad9144f into lightningnetwork:master Mar 20, 2024
25 of 27 checks passed

guggero deleted the bitcoind-26 branch March 20, 2024 07:50

guggero mentioned this pull request Apr 19, 2024

release: create v0.17.5 release branch #8664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub: use bitcoind v26.0 for CI #8273

GitHub: use bitcoind v26.0 for CI #8273

guggero commented Dec 12, 2023 •

edited

guggero commented Dec 18, 2023

yyforyongyu left a comment

Roasbeef left a comment

yyforyongyu commented Dec 21, 2023

guggero commented Dec 22, 2023

yyforyongyu Jan 16, 2024

ProofOfKeags Feb 1, 2024

guggero Feb 2, 2024

coderabbitai bot commented Feb 1, 2024 •

edited

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

ProofOfKeags left a comment

ProofOfKeags Feb 1, 2024

guggero commented Feb 2, 2024

guggero commented Mar 18, 2024

yyforyongyu left a comment

yyforyongyu commented Mar 19, 2024

ProofOfKeags left a comment

guggero commented Mar 20, 2024

pinheadmz commented Apr 18, 2024 •

edited

GitHub: use bitcoind v26.0 for CI #8273

GitHub: use bitcoind v26.0 for CI #8273

Conversation

guggero commented Dec 12, 2023 • edited

Change Description

guggero commented Dec 18, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

yyforyongyu commented Dec 21, 2023

guggero commented Dec 22, 2023

yyforyongyu Jan 16, 2024

Choose a reason for hiding this comment

ProofOfKeags Feb 1, 2024

Choose a reason for hiding this comment

guggero Feb 2, 2024

Choose a reason for hiding this comment

coderabbitai bot commented Feb 1, 2024 • edited

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ProofOfKeags left a comment

Choose a reason for hiding this comment

ProofOfKeags Feb 1, 2024

Choose a reason for hiding this comment

guggero commented Feb 2, 2024

guggero commented Mar 18, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

yyforyongyu commented Mar 19, 2024

ProofOfKeags left a comment

Choose a reason for hiding this comment

guggero commented Mar 20, 2024

pinheadmz commented Apr 18, 2024 • edited

guggero commented Dec 12, 2023 •

edited

coderabbitai bot commented Feb 1, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

pinheadmz commented Apr 18, 2024 •

edited