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.

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

Bump btcec to v2, use integrated btcutil module #6285

Merged
merged 3 commits into from
Mar 9, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Feb 21, 2022

Extracted out from #6263.

This PR switches lnd over to using the new btcec/v2 and btcutil modules located in the btcsuite/btcd repo.

All commits starting with SQUASHME will be squashed into a single commit before merging. They are only pushed separately for easier review.

@Roasbeef
Copy link
Member

Seeing this test failure uniformly:

--- FAIL: TestLightningWallet (114.32s)
    --- FAIL: TestLightningWallet/btcwallet/neutrino:publish_transaction (15.92s)
        test_interface.go:1914: expected ErrDoubleSpend, got: <nil>

I think it might be due to: lightninglabs/neutrino#240 ?

go.mod Show resolved Hide resolved
@Roasbeef
Copy link
Member

I think you may need to go into each sub-module and to go mod tidy, so it can pick up other dependencies like the psbt package.

@guggero
Copy link
Collaborator Author

guggero commented Feb 23, 2022

I think it might be due to: lightninglabs/neutrino#240 ?

Yes, you were right. It looks like that PR didn't account for reject messages correctly. I created a fix for it in lightninglabs/neutrino#247 and downgraded this PR to a version just before lightninglabs/neutrino#240 was merged.

@lightninglabs-deploy
Copy link

@guggero, remember to re-request review from reviewers when ready

@Roasbeef
Copy link
Member

Roasbeef commented Mar 3, 2022

Needs a rebase! We'll eventually update to that neutrino version before we tag the next major release, so fine w/ going with the downgraded version here.

@guggero guggero force-pushed the btcec-v2-upgrade branch 2 times, most recently from 6c5bc01 to 1309001 Compare March 5, 2022 11:00
@guggero
Copy link
Collaborator Author

guggero commented Mar 5, 2022

Rebased.

@guggero
Copy link
Collaborator Author

guggero commented Mar 8, 2022

Rebased again. @positiveblue PTAL.

@Roasbeef
Copy link
Member

Roasbeef commented Mar 9, 2022

Modules pops up with this warning:

no required module provides package github.com/btcsuite/btcd/btcutil/gcs/builder; to add it:

I think it's missing a go mod tidy somewhere? But perhaps it's also just due to the set of intermediate commits.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🍭

Now that the PR uses the prior version of neutrino, we're no longer blocked on that fix, allowing us to move forward here on the taproot front!

@guggero
Copy link
Collaborator Author

guggero commented Mar 9, 2022

Modules pops up with this warning:

no required module provides package github.com/btcsuite/btcd/btcutil/gcs/builder; to add it:

I think it's missing a go mod tidy somewhere? But perhaps it's also just due to the set of intermediate commits.

Yeah, the intermediate commits (prefixed with SQUASHME-) won't build on their own. My plan is to squash them into a single commit before merging.

Copy link
Collaborator

@positiveblue positiveblue 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 thanks for splinting this in its own PR

The errors that Github actions show are only because some of the commits do not compile by itself but after squashing everything should pass (as it does in the last commit)

This commit was previously split into the following parts to ease
review:
 - 2d746f68: replace imports
 - 4008f0fd: use ecdsa.Signature
 - 849e33d1: remove btcec.S256()
 - b8f6ebbd: use v2 library correctly
 - fa80bca9: bump go modules
@guggero
Copy link
Collaborator Author

guggero commented Mar 9, 2022

Thank you for the review, @positiveblue!

Going to squash the commits prefixed with SQUASHME- into a single one.
Adding this comment here to preserve the split on GitHub:

Parts:

  • 2d746f68: replace imports
  • 4008f0fd: use ecdsa.Signature
  • 849e33d1: remove btcec.S256()
  • b8f6ebbd: use v2 library correctly
  • fa80bca9: bump go modules

@guggero guggero merged commit 262591c into lightningnetwork:master Mar 9, 2022
@guggero guggero deleted the btcec-v2-upgrade branch March 9, 2022 18:55
guggero added a commit to guggero/lnd that referenced this pull request Mar 21, 2022
With the recent PR lightningnetwork#6285 merged that bumped the btcd dependency, we no
longer need to bump the github.com/onsi/ginkgo package with a replace
directive. Instead it was bumped indirectly by merging
btcsuite/btcd#1780 which is included in the btcd
version we reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants