Replace github.com/btcsuite/goleveldb imports with github.com/syndtr/goleveldb #1780

anupcshan · 2021-12-08T05:54:47Z

Generated using

$ find -name '*.go' | xargs sed -i "s#github.com/btcsuite/goleveldb#github.com/syndtr/goleveldb#g"
$ go mod tidy
$ go test -v ./...

Upstream goleveldb has 3+ years of bugfixes and improvements over the fork, which would be great to pull in.
I've been burned by a corruption which was fixed in syndtr/goleveldb@eb13543.

Related to #1770

coveralls · 2021-12-09T13:40:38Z

Pull Request Test Coverage Report for Build 1953020277

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+9.8%) to 79.676%

Totals
Change from base Build 1896270968:	9.8%
Covered Lines:	1231
Relevant Lines:	1545

💛 - Coveralls

anupcshan · 2021-12-09T17:21:08Z

FWIW, the failing test seems to be flaky on current master commit

$ go version
go version go1.17.3 linux/amd64

$ git rev-parse --short HEAD
780cc088

$ GORACE=halt_on_error=1 go test -race -tags=rpctest -covermode atomic -coverprofile=profile.cov -test.count 1000 ./connmgr
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
FAIL
coverage: 66.5% of statements
FAIL    github.com/btcsuite/btcd/connmgr        140.646s
FAIL

kcalvinalvin · 2021-12-10T14:00:44Z

FWIW, the failing test seems to be flaky on current master commit

$ go version
go version go1.17.3 linux/amd64

$ git rev-parse --short HEAD
780cc088

$ GORACE=halt_on_error=1 go test -race -tags=rpctest -covermode atomic -coverprofile=profile.cov -test.count 1000 ./connmgr
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:297: retry: 127.0.0.1:18555 - want state 4, got state 3
--- FAIL: TestRetryPermanent (0.00s)
    connmanager_test.go:272: retry: 127.0.0.1:18555 - want state 0, got state 3
FAIL
coverage: 66.5% of statements
FAIL    github.com/btcsuite/btcd/connmgr        140.646s
FAIL

Ah yeah this happens all the time. The test sometimes passes and sometimes doesn't. Could you git --amend so that you'll get a different commit hash and force push to trigger github actions?

Also, this PR looks good to me.

guggero · 2022-03-08T15:03:56Z

I also think we should use the original version instead of the fork if possible.
It looks like the fork was mainly created to add go module support? Though there also is this commit: btcsuite/goleveldb@9168d88 which adds a bunch of functionality.
@davecgh do you remember if that added any btcd specific functionality or was a pure sync with upstream? I don't really see some of those changes in the upstream repo, so not sure how to approach the diff.

davecgh · 2022-03-08T17:33:24Z

It was primarily added prior to modules because, back in those days, Go had many of the same problems as node.js in terms of packages being able to disappear and or get updated in an incompatible way which could either break go get (aka builds back then) or allow backdoors to be introduced. We didn't want to vendor it in the main repo, so creating the fork was the most reasonable option.

Nowadays, with modules, the module mirror, and MVS, all of those issues are solved much more nicely, so I agree that it's better to use the upstream. That is, in fact, what we do in dcrd now.

guggero · 2022-03-08T17:36:10Z

Yeah, I understand that concern, I probably would've done something similar.

So you can confirm that no btcd specific functionality was added in the fork and bumping to the latest upstream version shouldn't break anything?

davecgh · 2022-03-08T17:41:17Z

Yes, unless anything was introduced after I stopped working on btcsuite, which it doesn't look like it from the commit history, nothing unique was introduced in the fork, so changing to the upstream module shouldn't break anything and is what I would recommend as well.

guggero · 2022-03-08T17:43:00Z

Can confirm that the unit tests run fine locally.

The btcutil/go.sum needs to be updated though. I added your commit in my PR (#1823) to fast-track this fix.

anupcshan · 2022-03-08T17:44:29Z

FWIW, I've been running an instance of btcd with this applied, and it was able to get fully synced up without crashes - this doesn't guarantee correctness or consistency, but I can verify it doesn't crash.

guggero · 2022-03-08T17:47:37Z

Cool! Can you run go mod tidy in btcutil to fix the go mod error I get when running make unit?
That will also trigger the unit tests again. Then we can also merge this PR as it is instead of adding the commit to my PR.

anupcshan · 2022-03-08T18:02:00Z

Cool! Can you run go mod tidy in btcutil to fix the go mod error I get when running make unit? That will also trigger the unit tests again. Then we can also merge this PR as it is instead of adding the commit to my PR.

Done. make unit passes now. Didn't realize that was broken.

anupcshan · 2022-03-08T18:05:42Z

Actually, CI will not run automatically since this is my first btcd PR. Someone will need to approve this change before tests run.

guggero

tACK, LGTM 🎉

guggero · 2022-03-08T18:18:15Z

cc @Roasbeef.

Roasbeef · 2022-03-08T23:29:24Z

It looks like the fork was mainly created to add go module support? Though there also is this commit: btcsuite/goleveldb@9168d88 which adds a bunch of functionality.

Correct, given that we have the option to do go mod vendor and commit those deps, we no longer really need the forks.

Roasbeef

LGTM 🦖

I think that given the sub-module sum files were updated as a result, we'll also need to push new minor tags there?

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.

anupcshan force-pushed the replace-imports branch from dd70cef to 267bb2e Compare December 10, 2021 17:00

anupcshan force-pushed the replace-imports branch 2 times, most recently from 9f17681 to 554ba24 Compare February 9, 2022 05:40

guggero mentioned this pull request Feb 16, 2022

Taproot: Add taproot address and signing capabilities btcsuite/btcwallet#792

Merged

guggero mentioned this pull request Mar 8, 2022

Taproot: integrate btcec/v2 and btcwallet changes to support Taproot key spend paths in lnd's wallet lightningnetwork/lnd#6263

Merged

guggero mentioned this pull request Mar 8, 2022

Remove circular dependency issue between btcec/v2 and main package #1823

Merged

anupcshan force-pushed the replace-imports branch from 554ba24 to 519fac2 Compare March 8, 2022 18:01

Replace github.com/btcsuite/goleveldb with github.com/syndtr/goleveldb

87e3d7e

anupcshan force-pushed the replace-imports branch from 519fac2 to 87e3d7e Compare March 8, 2022 18:07

guggero approved these changes Mar 8, 2022

View reviewed changes

Roasbeef approved these changes Mar 8, 2022

View reviewed changes

Roasbeef merged commit 21b37c0 into btcsuite:master Mar 8, 2022

anupcshan deleted the replace-imports branch March 8, 2022 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace github.com/btcsuite/goleveldb imports with github.com/syndtr/goleveldb #1780

Replace github.com/btcsuite/goleveldb imports with github.com/syndtr/goleveldb #1780

anupcshan commented Dec 8, 2021

coveralls commented Dec 9, 2021 •

edited

anupcshan commented Dec 9, 2021

kcalvinalvin commented Dec 10, 2021 •

edited

guggero commented Mar 8, 2022

davecgh commented Mar 8, 2022 •

edited

guggero commented Mar 8, 2022

davecgh commented Mar 8, 2022

guggero commented Mar 8, 2022

anupcshan commented Mar 8, 2022

guggero commented Mar 8, 2022

anupcshan commented Mar 8, 2022

anupcshan commented Mar 8, 2022

guggero left a comment

guggero commented Mar 8, 2022

Roasbeef commented Mar 8, 2022 •

edited

Roasbeef left a comment

Replace github.com/btcsuite/goleveldb imports with github.com/syndtr/goleveldb #1780

Replace github.com/btcsuite/goleveldb imports with github.com/syndtr/goleveldb #1780

Conversation

anupcshan commented Dec 8, 2021

coveralls commented Dec 9, 2021 • edited

Pull Request Test Coverage Report for Build 1953020277

💛 - Coveralls

anupcshan commented Dec 9, 2021

kcalvinalvin commented Dec 10, 2021 • edited

guggero commented Mar 8, 2022

davecgh commented Mar 8, 2022 • edited

guggero commented Mar 8, 2022

davecgh commented Mar 8, 2022

guggero commented Mar 8, 2022

anupcshan commented Mar 8, 2022

guggero commented Mar 8, 2022

anupcshan commented Mar 8, 2022

anupcshan commented Mar 8, 2022

guggero left a comment

Choose a reason for hiding this comment

guggero commented Mar 8, 2022

Roasbeef commented Mar 8, 2022 • edited

Roasbeef left a comment

Choose a reason for hiding this comment

coveralls commented Dec 9, 2021 •

edited

kcalvinalvin commented Dec 10, 2021 •

edited

davecgh commented Mar 8, 2022 •

edited

Roasbeef commented Mar 8, 2022 •

edited