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.

Sign up for GitHub

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

Jump to bottom

Update btcd to master, make itest btcd harness more robust #4765

Merged
merged 5 commits into from
Dec 4, 2020
Merged

Update btcd to master, make itest btcd harness more robust #4765

merged 5 commits into from
Dec 4, 2020

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Nov 12, 2020 •
edited
Loading

This PR updates btcd to current master and fixes some flakes that were caused by btcd's random listening port selection during itests that sometimes resulted in collisions.

@guggero guggero force-pushed the itest-miner-fix branch 4 times, most recently from 0510c6d to 6504203 Compare November 12, 2020 15:05
@guggero guggero requested a review from Roasbeef as a code owner November 12, 2020 15:05
@Roasbeef
Copy link
Member

Can be rebased now the the dependent PR is in!

@bhandras bhandras self-requested a review November 30, 2020 12:56
@bhandras
Copy link
Collaborator

Ping when ready for review.

@guggero
Copy link
Collaborator Author

guggero commented Dec 1, 2020 •
edited
Loading

I wanted to do more bitcoind flake hunting with this, but I think it's already a valid flake fix as is. Can always continue flake hunting after this is merged. So @bhandras, PTAL.

bhandras
bhandras approved these changes Dec 1, 2020
View reviewed changes
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK, looking fwd to this!

lntest/node.go Outdated Show resolved Hide resolved
lntest/btcd.go Show resolved Hide resolved
@halseth halseth self-requested a review December 2, 2020 08:58
halseth
halseth suggested changes Dec 2, 2020
View reviewed changes
lntest/btcd.go
@@ -88,7 +88,7 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
// make sure they stay connected if it happens.
"--nobanning",
}
chainBackend, err := rpctest.New(netParams, nil, args)
chainBackend, err := rpctest.New(netParams, nil, args, GetBtcdBinary())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what benefit does this give us? Mostly for local testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It avoids the test harness compiling btcd for every run by invoking go build. That way we also have more certainty that the actually built version of btcd is what we define in go.mod. Also we don't overwrite any pre-installed btcd on the system anymore.

Makefile
@@ -32,7 +31,7 @@ COMMIT_HASH := $(shell git rev-parse HEAD)

BTCD_COMMIT := $(shell cat go.mod | \
grep $(BTCD_PKG) | \
tail -n1 | \
head -n1 | \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, was this wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It caused an error if you had a replace directive for btcd. The way we now compile the binary actually allows replace directives to be used. Only the unit tests still won't work with the replace (will pick what's in the upper part of go.mod). Maybe we should try to switch the unit tests over to an explicit binary as well and get rid of the DEPGET invocation for btcd?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but could probably be done in another PR 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, wasn't thinking about adding it to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ran into this the other day!

Makefile
@$(call print, "Building itest lnd and lncli.")
$(GOBUILD) -tags="$(ITEST_TAGS)" -o lnd-itest $(ITEST_LDFLAGS) $(PKG)/cmd/lnd
$(GOBUILD) -tags="$(ITEST_TAGS)" -o lncli-itest $(ITEST_LDFLAGS) $(PKG)/cmd/lncli
@$(call print, "Building itest btcd and lnd.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to see what actually changes here, and what is simply a refactor, would be great if you could split the commit!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. I split the commit.

scripts/itest_part.sh

# Windows insists on having the .exe suffix for an executable, we need to add
# that here if necessary.
EXEC="$WORKDIR"/itest.test"$EXEC_SUFFIX"
LND_EXEC="$WORKDIR"/lnd-itest"$EXEC_SUFFIX"
echo $EXEC -test.v "$@" -logoutput -goroutinedump -logdir=.logs-tranche$TRANCHE -lndexec=$LND_EXEC -splittranches=$NUM_TRANCHES -runtranche=$TRANCHE
BTCD_EXEC="$WORKDIR"/btcd-itest"$EXEC_SUFFIX"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the benefit of explicitly specifying it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean why in this file and not in the Makefile? I guess I wanted to avoid passing in even more parameters.

@guggero
multi: update to latest btcd version
2edc3cf 
With this commit we update btcd to the latest version which allows us to
specify the btcd binary we want to use when spinning up a new node.
@guggero
lntest: use nextAvailablePort for miner and btcd backend
36756c0 
With the new btcd version we can specify our own listen address
generator function for any btcd nodes. This should reduce flakiness as
the previous way of getting a free port was based on just picking a
random number which lead to conflicts.
We also double the default values for connection retries and timeouts,
effectively waiting up to 4 seconds in total now.
@guggero guggero requested a review from halseth December 3, 2020 10:37
halseth
halseth approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✔️

@guggero guggero added this to the 0.12.0 milestone Dec 3, 2020
cfromknecht
cfromknecht reviewed Dec 3, 2020
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile
@@ -32,7 +31,7 @@ COMMIT_HASH := $(shell git rev-parse HEAD)

BTCD_COMMIT := $(shell cat go.mod | \
grep $(BTCD_PKG) | \
tail -n1 | \
head -n1 | \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ran into this the other day!

@guggero
make: replace ITEST goal with script
b91b743 
To make the Makefile a bit easier to understand, we remove the implicit
ITEST goal/command variable and switch all itest execution over to
explicit goals in the main Makefile.
@guggero
make+Travis: use EXEC_SUFFIX for Windows, remove explicit goal
8829960 
To remove the need to have an extra make goal for the Windows itests, we
instead add the flag windows=1 that sets the make variable EXEC_SUFFIX
to properly add the ".exe" suffix to all executable names.
@guggero
make+scripts: use pre-compiled btcd in itest
b42c5e5 
To make sure we build the exact version of btcd that is referenced in
the project's go.mod file and to not overwrite any binary the user might
already have installed on the system, we compile btcd into an explicit
file in the itest directory.
This should also speed up invocations of "make itest-only" because the
test harness doesn't always compile btcd on its own.

We also fix a bug with the version parsing where adding a "replace"
directive in the go.mod would result in the awk commands to extract the
wrong version. Because we no longer use the DEPGET goal to build and
install btcd, using a replace directive now actually works for itests.
@guggero guggero requested a review from cfromknecht December 3, 2020 22:24
cfromknecht
cfromknecht approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

@cfromknecht cfromknecht merged commit 3e1ba20 into lightningnetwork:master Dec 4, 2020
@guggero guggero deleted the itest-miner-fix branch December 4, 2020 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halseth halseth halseth approved these changes

@cfromknecht cfromknecht cfromknecht approved these changes

@bhandras bhandras bhandras approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@guggero @Roasbeef @bhandras @cfromknecht @halseth