btcsuite / btcd

An alternative full node bitcoin implementation written in Go (golang)
https://github.com/btcsuite/btcd/blob/master/README.md
ISC License
6.23k stars 2.36k forks source link

ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules #1839

Closed shrimalmadhur closed 2 years ago

shrimalmadhur commented 2 years ago

Hello,

I am using the beta version (btcd@v0.22.0-beta) and recently I got this error while building

github.com/btcsuite/btcd/btcec tested by
github.com/btcsuite/btcd/btcec.test imports
github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
github.com/btcsuite/btcd v0.22.0-beta (/Users/madhurshrimal/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/Users/madhurshrimal/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)

It didn't use to come before and suddenly it popped up. Any idea how I can solve this?

sarvalabs-sai commented 2 years ago

You may try deleting go.mod ,go.sum and do go clean --modcache before doing go mod init

shrimalmadhur commented 2 years ago

@sarvalabs-sai I did that I still get the same result. For more context, it is happening in this repository - https://github.com/coinbase/rosetta-sdk-go during running make format command.

chappjc commented 2 years ago

@shrimalmadhur I believe the issue is that your make format target does go run golang.org/x/tools/cmd/goimports with no version suffix and from the module folder. From the go help run output for both go 1.17 and 1.18 (the only two Go versions that are not EOL):

If the package argument has a version suffix (like @latest or @v1.0.0),
"go run" builds the program in module-aware mode, ignoring the go.mod file in
the current directory or any parent directory, if there is one. This is useful
for running programs without affecting the dependencies of the main module.

If the package argument doesn't have a version suffix, "go run" may run in
module-aware mode or GOPATH mode, depending on the GO111MODULE environment
variable and the presence of a go.mod file. See 'go help modules' for details.
If module-aware mode is enabled, "go run" runs in the context of the main
module.

You want go run to leave your module's go.mod alone. So, you can try to append @latest:

go run golang.org/x/tools/cmd/goimports@latest -w .

Or you can just install goimports like you do with golines and goveralls. You'll have to do that if you still want to use go 1.16 or earlier. Since that feature of go run was introduced in 1.17, and there seems to be better handling with ambiguous imports with 1.17.

erwanor commented 2 years ago

I was having the same issue building a downstream script that uses ethclient (four or five layers removed from btcsuite/btcd/chaincfg/chainhash).

github.com/myrepo/myrepo imports
        github.com/ethereum/go-ethereum/accounts/abi/bind imports
        github.com/ethereum/go-ethereum/crypto imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.20.1-beta (/home/user/go/pkg/mod/github.com/btcsuite/btcd@v0.20.1-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/home/user/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)

I solved it by forcing go.mod to resolve the package to the latest version (v1.0.0), by appending:

require github.com/btcd/chaincfg/chainhash v1.0.0 in my go.mod

richardbertozzo commented 2 years ago

The same here, what worked for me was forcing the version of btcd and go-ethereum at go.mod

module example

go 1.16

require (
    github.com/btcsuite/btcd v0.22.0-beta
    github.com/ethereum/go-ethereum v1.10.16    
)
shrimalmadhur commented 2 years ago

@richardbertozzo yea seems like it happens when I upgrade go-ethereum to v1.10.17 keeping the btcd version constant. I want to update to the newer version due to a security update https://github.com/coinbase/rosetta-sdk-go/security/dependabot/1

@erwanor adding that line to go.mod is still not helping my case.

chappjc commented 2 years ago

@shrimalmadhur Is it erroring on go build for you or just that go run command of the goimports tool? What Go version are you using? One user of go-ethereum and btcd suggested the following, presumably for this import ambiguity issue, although I don't know what Go versions have trouble without it: https://github.com/ethereum/go-ethereum/pull/24554/commits/f9ea1f1d6b4cd9c476d767df3fea1b66348e9c47#

shrimalmadhur commented 2 years ago

@chappjc I am using go1.16. So when I followed your suggestion to do the way I am doing golines - I could run the make format command but still some of the other commands (running make gen, that doesn't really install any dependency, we use it for codegen) are having the same issue.

Just saw your updated comment. let me try that.

shrimalmadhur commented 2 years ago

@chappjc even that other change doesn't help either

I just realized, one of our script is doing

./imports.sh;
go: downloading github.com/incu6us/goimports-reviser v0.1.6
go: downloading github.com/incu6us/goimports-reviser/v2 v2.5.1
go: downloading golang.org/x/mod v0.5.1
go: downloading golang.org/x/tools v0.1.8
go: downloading golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: downloading github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2-0.20220316175102-8d5c75c28923
go: downloading gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
go: downloading github.com/google/go-cmp v0.5.4
go: downloading github.com/OneOfOne/xxhash v1.2.2
go: downloading github.com/spaolacci/murmur3 v1.1.0
go: downloading github.com/leanovate/gopter v0.2.9
go: downloading github.com/kr/pretty v0.2.1
go: downloading github.com/kr/text v0.2.0
github.com/coinbase/rosetta-sdk-go/keys imports
    github.com/btcsuite/btcd/btcec tested by
    github.com/btcsuite/btcd/btcec.test imports
    github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
    github.com/btcsuite/btcd v0.22.0-beta (/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
    github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2-0.20220316175102-8d5c75c28923 (/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.2-0.20220316175102-8d5c75c28923)
make: *** [gen] Error 1

Specifically it's downloading - go: downloading github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2-0.20220316175102-8d5c75c28923

Maybe that's the problem.

Update: The above is because I added the suggestion mentioned in go-ethereum. removing that it downloads go: downloading github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 so doesn't look like that's the issue

chappjc commented 2 years ago

That imports.sh issue is also related to changes in the go tooling. Namely, doing go get is trying to modify the current module, but you're really trying to install that goimports-reviser. go help get:

Building and installing packages with get is deprecated. In a future release,
the -d flag will be enabled by default, and 'go get' will be only be used to
adjust dependencies of the current module. To install a package using
dependencies from the current module, use 'go install'. To install a package
ignoring the current module, use 'go install' with an @version suffix like
"@latest" after each argument.

I just tested with Go 1.16, you can do go install github.com/incu6us/goimports-reviser/v2@latest and it will leave the go.mod alone.

chappjc commented 2 years ago

Also, I'm not really sure about your deps target (which codegen.sh runs) given what I noted about go get above:

deps:
    go get ./...

I think you just want go build ./... or go install ./... there.

Earlier versions of go worked very differently with go get. Now go get is aimed primarily at modifying the current module's go.{mod,sum} files, not for building or installing anything.

If at all possible, you should update to Go 1.17 or 1.18. Go 1.16 is EOL and you're not going to get any security updates, such as the one announced for release today: https://groups.google.com/g/golang-announce/c/vtbMjE04kPk/m/xE-FGxCXCAAJ?utm_medium=email&utm_source=footer

shrimalmadhur commented 2 years ago

@chappjc that makes sense. I think I am going to update Go first as 1.16 is EOL. for import issue I already updated the import.sh script to use go install. Let me update Go first and also change make deps. Thanks for the detail explanation. I will post an update once I've some done all those things.

shrimalmadhur commented 2 years ago

@chappjc Hmm. so I updated to go1.17 - It adjusted go.mod and go.sum quite a bit but I still see same issue when I put go-ethereum v1.10.17

./imports.sh;
go: downloading github.com/incu6us/goimports-reviser/v2 v2.5.1
go: downloading github.com/incu6us/goimports-reviser v0.1.6
go: downloading golang.org/x/mod v0.5.1
go: downloading golang.org/x/tools v0.1.8
go: finding module for package golang.org/x/xerrors
go: finding module for package golang.org/x/sys/execabs
go: downloading golang.org/x/sys v0.0.0-20220406163625-3f8b81556e12
go: downloading golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: found golang.org/x/sys/execabs in golang.org/x/sys v0.0.0-20220406163625-3f8b81556e12
go: found golang.org/x/xerrors in golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: downloading github.com/google/go-cmp v0.5.4
go: downloading github.com/spaolacci/murmur3 v1.1.0
go: downloading github.com/leanovate/gopter v0.2.9
go: downloading gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
go: downloading github.com/OneOfOne/xxhash v1.2.2
go: downloading github.com/kr/pretty v0.2.1
go: downloading github.com/kr/text v0.2.0
go: downloading github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0
github.com/coinbase/rosetta-sdk-go/keys imports
    github.com/btcsuite/btcd/btcec tested by
    github.com/btcsuite/btcd/btcec.test imports
    github.com/btcsuite/btcd/chaincfg/chainhash loaded from github.com/btcsuite/btcd@v0.22.0-beta,
    but go 1.16 would fail to locate it:
    ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
    github.com/btcsuite/btcd v0.22.0-beta (/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
    github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)

To proceed despite packages unresolved in go 1.16:
    go mod tidy -e
If reproducibility with go 1.16 is not needed:
    go mod tidy -compat=1.17
For other options, see:
    https://golang.org/doc/modules/pruning
make: *** [gen] Error 1

Not sure why it's still trying to resolve for go1.16 - but go 1.16 would fail to locate it:

chappjc commented 2 years ago

Wow, that's irritating. go mod really trying to please go 1.16, which seems to have a disability here. :/

For a general solution, I don't know what to suggest for go 1.16 users, it may be impossible. For your module, I think you might be forced require 1.17 (go mod tidy -compat=1.17). Also don't forget the @latest when using go install in imports.sh and elsewhere so it doesn't touch the go.mod.

As an alternative, I would suggest switching the github.com/btcsuite/btcd require to github.com/btcsuite/btcd/btcec/v2 (an entirely separate module), since your project only seems to use btcd for the btcec package. The changes are trivial: update the import with a /v2 and omit the curve argument in two spots. However, github.com/Zilliqa/gozilliqa-sdk will be giving the same ambiguity issue as they're similarly requiring both btcd and go-ethereum.

@Roasbeef, if the btcec/v2 module did not require the ambiguous chaincfg/chainhash module in its test, these import issues for users still requiring the btcd v0.22.0-beta or older modules would vanish. Just another possible resolution since it's seems hard for users to resolve if they need to be importing the older btcd module too. EDIT: oh dang it's used in the regular schorr code too, not just tests

shrimalmadhur commented 2 years ago

I already did go mod tidy -compat=1.17 when I updated to 1.17. Even after that it's showing me go1.16 failed to locate issue. Using @latest with go install now - thanks for the tip.

I tried your diff but as expected, it is now giving the same issue with github.com/Zilliqa/gozilliqa-sdk

github.com/coinbase/rosetta-sdk-go/keys imports
    github.com/Zilliqa/gozilliqa-sdk/schnorr imports
    github.com/btcsuite/btcd/btcec tested by
    github.com/btcsuite/btcd/btcec.test imports
    github.com/btcsuite/btcd/chaincfg/chainhash loaded from github.com/btcsuite/btcd@v0.21.0-beta.0.20201114000516-e9c7a5ac6401,
    but go 1.16 would fail to locate it:
    ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
    github.com/btcsuite/btcd v0.21.0-beta.0.20201114000516-e9c7a5ac6401 (/go/pkg/mod/github.com/btcsuite/btcd@v0.21.0-beta.0.20201114000516-e9c7a5ac6401/chaincfg/chainhash)
    github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)
chappjc commented 2 years ago

Yeah, looks like every go mod tidy command would have to have that switch until the ambiguity is ultimately eliminated... somehow. :( ugh. The go mod tidy in the docker command in codegen.sh namely. The imports script at least runs fine for me if it uses @latest in the command. I'm not sure why the suggestion from @erwanor doesn't work on your module. I'm guessing that zilliqua mod breaks it for 1.16.

shrimalmadhur commented 2 years ago

Oh wait a min. With you diff and adding go mod tidy -compat=1.17 in codegen.sh did not give me this error. I somehow was doing go mod tidy -compat=1.17 from my terminal and didn't realize the codegen.sh part. Maybe I was missing that and might not need the v2 change either? Let me try

Update: Ya I don't need that v2 change either. 🤦 - somehow I missed that go mod tidy in codegen.sh. But this does mean that it won't be compatible for go1.16 right?

Also can I still import this package in other repo which are using go1.16? Or they also need to be updated?

chappjc commented 2 years ago

Don't know what go 1.16 users can expect, sorry.

Regardless, the btcec/v2 update in go-ethereum indirectly caused it to require the module version of the package github.com/btcsuite/btcd/chaincfg/chainhash and not just the chainhash package from the older btcd module. We can see this if we try to instruct it not to use the module version of chainhash:

$  go get -d github.com/ethereum/go-ethereum@v1.10.17
go: upgraded github.com/ethereum/go-ethereum v1.10.16 => v1.10.17

$  go get -d github.com/btcsuite/btcd/chaincfg/chainhash@none  # explicitly remove the module requirement
go: removed github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0
go: downgraded github.com/ethereum/go-ethereum v1.10.17 => v1.10.16

The go tooling can only resolve that by downgrading go-ethereum. It's not clear why go 1.16 and earlier aren't ok with an explicit require github.com/btcd/chaincfg/chainhash v1.0.0 to disambiguate.

In general, it's probably good for most btcd consumers like gozilliqa-sdk to start consuming only the new sub-modules that stick to SIV. In the meantime, looks like some pain.

EDIT: Ooooof, apparently there was a screwy procedure to "carve out" a new submodule: https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository (second case) Seems to be too late for that now. Yuk, go modules are really a nightmare sometimes.

shrimalmadhur commented 2 years ago

I see. seems like it might work with existing go1.16 though. I will try that.

Dmdv commented 2 years ago

Keeps reproducing in all dependencies from 1.17

Dmdv commented 2 years ago

I've tried everything. And go mod tidy -compat=1.17 didn't help

require (
    github.com/ethereum/go-ethereum v1.10.17
    github.com/pkg/errors v0.9.1
    github.com/status-im/keycard-go v0.0.0-20210911161356-c8058144cee8
    github.com/stretchr/testify v1.7.1
    go.uber.org/zap v1.21.0
    golang.org/x/crypto v0.0.0-20220411220226-7b82a4e95df4
)

require (
    github.com/0chain/errors v1.0.3 // indirect
    github.com/0chain/gosdk v1.7.8 // indirect
    github.com/ReneKroon/ttlcache/v2 v2.11.0 // indirect
    github.com/btcsuite/btcd/btcec/v2 v2.1.3 // indirect
    github.com/deckarep/golang-set v1.8.0 // indirect
    github.com/decred/dcrd/dcrec/secp256k1/v4 v4.0.1 // indirect
    github.com/didip/tollbooth v4.0.2+incompatible // indirect
    github.com/fsnotify/fsnotify v1.5.1 // indirect
    github.com/go-ole/go-ole v1.2.6 // indirect
    github.com/go-stack/stack v1.8.1 // indirect
    github.com/google/uuid v1.3.0 // indirect
    github.com/gorilla/websocket v1.5.0 // indirect
    github.com/hashicorp/hcl v1.0.0 // indirect
    github.com/herumi/bls-go-binary v1.0.1-0.20210830012634-a8e769d3b872 // indirect
    github.com/lithammer/shortuuid/v3 v3.0.7 // indirect
    github.com/magiconair/properties v1.8.6 // indirect
    github.com/mitchellh/mapstructure v1.4.3 // indirect
    github.com/patrickmn/go-cache v2.1.0+incompatible // indirect
    github.com/pelletier/go-toml v1.9.4 // indirect
    github.com/pelletier/go-toml/v2 v2.0.0-beta.8 // indirect
    github.com/rjeczalik/notify v0.9.2 // indirect
    github.com/shirou/gopsutil v3.21.11+incompatible // indirect
    github.com/spf13/afero v1.8.2 // indirect
    github.com/spf13/cast v1.4.1 // indirect
    github.com/spf13/jwalterweatherman v1.1.0 // indirect
    github.com/spf13/pflag v1.0.5 // indirect
    github.com/spf13/viper v1.11.0 // indirect
    github.com/subosito/gotenv v1.2.0 // indirect
    github.com/tklauser/go-sysconf v0.3.10 // indirect
    github.com/tklauser/numcpus v0.4.0 // indirect
    github.com/tyler-smith/go-bip39 v1.1.0 // indirect
    github.com/yusufpapurcu/wmi v1.2.2 // indirect
    go.uber.org/atomic v1.9.0 // indirect
    go.uber.org/multierr v1.8.0 // indirect
    golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect
    golang.org/x/sys v0.0.0-20220412211240-33da011f77ad // indirect
    golang.org/x/text v0.3.7 // indirect
    golang.org/x/time v0.0.0-20220411224347-583f2d630306 // indirect
    gopkg.in/ini.v1 v1.66.4 // indirect
    gopkg.in/natefinch/lumberjack.v2 v2.0.0 // indirect
    gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce // indirect
    gopkg.in/yaml.v2 v2.4.0 // indirect
    gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
)

go 1.17

The output:

ethereum/handler imports
        github.com/ethereum/go-ethereum/crypto imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.22.0-beta (/Users/dima/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/Users/dima/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)
Dmdv commented 2 years ago

Downgrade to go-ethereum 1.10.16 will not work for me either because I need 1.10.17

Dmdv commented 2 years ago

I was having the same issue building a downstream script that uses ethclient (four or five layers removed from btcsuite/btcd/chaincfg/chainhash).

github.com/myrepo/myrepo imports
        github.com/ethereum/go-ethereum/accounts/abi/bind imports
        github.com/ethereum/go-ethereum/crypto imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.20.1-beta (/home/user/go/pkg/mod/github.com/btcsuite/btcd@v0.20.1-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/home/user/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)

I solved it by forcing go.mod to resolve the package to the latest version (v1.0.0), by appending:

require github.com/btcd/chaincfg/chainhash v1.0.0 in my go.mod

Not working

go: github.com/btcd/chaincfg/chainhash@v1.0.0: invalid version: git ls-remote -q origin in /Users/dima/go/pkg/mod/cache/vcs/8145d96c3e759b89ceeccb36491dc7d17a6d1b874f2973b498cd66993ae3ef2f: exit status 128:
    remote: Repository not found.
    fatal: repository 'https://github.com/btcd/chaincfg/' not found
fjl commented 2 years ago

If we can fix it in go-ethereum by adding an explicit requirement of github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 or any other version, I'm willing to apply this fix immediately.

I still don't understand what the issue is? Does it only happen with Go 1.16?

chappjc commented 2 years ago

Not working

go: github.com/btcd/chaincfg/chainhash@v1.0.0: invalid version: git ls-remote -q origin in /Users/dima/go/pkg/mod/cache/vcs/8145d96c3e759b89ceeccb36491dc7d17a6d1b874f2973b498cd66993ae3ef2f: exit status 128:
  remote: Repository not found.
  fatal: repository 'https://github.com/btcd/chaincfg/' not found

@Dmdv you left out the "btcsuite" from that url

I still don't understand what the issue is? Does it only happen with Go 1.16?

@fjl Me neither. :/ Go modules at their worst here.

fjl commented 2 years ago

I have merged a PR that might fix this in go-ethereum. Everyone affected, please retry go get github.com/ethereum/go-ethereum in your modules.

shrimalmadhur commented 2 years ago

I still don't understand what the issue is? Does it only happen with Go 1.16?

@fjl It does happen with Go 1.16 and Go 1.17 and go-ethereum >= 1.10.17. If I update to Go 1.17 (with go-ethereum >= 1.10.17) and do go mod tidy -compat=1.17 I don't see this issue. So it seems if it has Go 1.16 dependency hashes then it's giving this issue.

Also I did try the latest master of go-ethereum and keeping it compatible with Go 1.16 but I am getting the same issue.

chappjc commented 2 years ago

Seems like half the internet is getting stuck on this, and the only resolution is to (1) upgrade go, (2) use special tidy flags, and (3) disambiguate with an indirect require directive for this chainhash module. Not great.

It's probably too late to follow the "carve out" procedure described in https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository (e.g https://github.com/Azure/go-autorest/pull/455), so I think the options for btcd maintainers are:

  1. drop the require on chainhash from btcec/v2, somehow, OR
  2. make a chainhash/v2 and update btcec/v2 and any others to use that instead

The chainhash module appears to have been carved out of the btcd module at a commit after the btcec/v2 module was created (where the btcec package no longer exists), so I have a hard time seeing how to make the described import cycle without causing even more dependency issues.

@Roasbeef @jcvernaleo @guggero I'm out of ideas and I feel a little responsible having updated go-ethereum to use btcec/v2

In summary. the issue manifests if a project tries to require both btcd@v0.22.0-beta and another project that uses either btcec/v2 (or just chainhash@v1.x.y), such as go-ethereum does. Note that the problem would eventually go away on its own if there was a v0.23.0 that used the new chainash module and all consumers in the ecosystem updated to use it, but that's not too realistic.

jcvernaleo commented 2 years ago

@chappjc would making a v0.23.0 release help at all (even if not everyone adopts it right away)?

And would making that new tag and release hurt anything in relation to this?

If the answers to those are 'yes' and 'no' in that order, I'm happy to get a new release going asap (which probably means later this week).

chappjc commented 2 years ago

@chappjc would making a v0.23.0 release help at all (even if not everyone adopts it right away)?

Not really unfortunately. A project could require that new version or try to force it's depends to require it, but it would be a breaking change for a number of reasons, including the txscript changes and the btcec/v2 module. So unless everyone updates and migrates to accommodate for the breaking changes, it's likely to create more havoc.

This does raise the bigger question of how the next btcd release should be tagged. As awkward as it is, the Go-friendly solution for the next btcd module tag would be to have a btcd/v2 module because of various breaking changes on master since v0.22.0-beta. That doesn't mean the product needs to be v2.0.0, just the module. The way to do that is to tag product (the btcd CLI app) releases like release-v0.23, and a semver tag for a module release like v2.0.0 (for the root btcd module). The dcrd/dcrwallet repository has had to start this approach some time ago, and it seems to be a good solution to deal with Go's SIV system for modules.

That's mostly unrelated to the chainhash import ambiguity though, except that in general I think we're seeing the safest way to move forward and "carve out" modules from a multi-module repository is to start with a v2.

Roasbeef commented 2 years ago

@chappjc thanks for the great summary!

, so I think the options for btcd maintainers are: drop the require on chainhash from btcec/v2, somehow, OR make a chainhash/v2 and update btcec/v2 and any others to use that instead

Re 1: the btcec module only really ues the chainhash package for tests, and also the bIp-340 (and soon musig2) signatures. So just living with some copy-paste/duplication shouldn't be too difficult.

Re 2: is this what's meant by "carve out"? This is even more straight forward than option #1 imo. I could get this done by the end of the week easily if this is the path we want to go on with.

I don't think we should immediately do a v0.23.0 release, mainly because we've merged in some rather large items into master since the lsat release including: full taproot support, BIP 151 support, schnorr support, and also soon musig2. I'd ideally want to have a proper release candidate after the musig2 work gets in (though that's just at the btcec level as Bitcoin consensus doesn't actually know about musig2). On the lnd, side we'll be tagging the first rc of our 0.15 release, which uses many of these features, so the rc periods could run concurrently.

Roasbeef commented 2 years ago

. As awkward as it is, the Go-friendly solution for the next btcd module tag would be to have a btcd/v2

IIUC, that wouldn't actually fix this issue, but instead would be intended to be more forwarding looking as it's essentially a clean "break" off the existing module?

Roasbeef commented 2 years ago

Also fwiw, re Go 1.16 vs the rest: once we update the main modules to Go 1.18, with our policy of supporting the past two releases (which would be 1.18 and 1.17 in this case), we wouldn't necessarily be concerned with issues when building/including with prior releases. Not to say that we can't change the policy, but just wanted to lay that out.

chappjc commented 2 years ago

Re 1: the btcec module only really ues the chainhash package for tests, and also the bIp-340 (and soon musig2) signatures. So just living with some copy-paste/duplication shouldn't be too difficult.

Yeah, doesn't look bad: https://github.com/btcsuite/btcd/compare/master...chappjc:btcec-no-chainhash?expand=1

Re 2: is this what's meant by "carve out"? This is even more straight forward than option https://github.com/btcsuite/btcd/issues/1 imo. I could get this done by the end of the week easily if this is the path we want to go on with.

The carving out refers to when the chainhash/chaincfg module was first created from a package in a parent module. Turns out it's a lot more nuanced than expected if you wanna start that new sub-module at v1 vs v2. This is the "second class" of the job mentioned on the go wiki, which it calls out as possibily leading to such ambiguous imports. (I did not anticipate this, to be clear!)

. As awkward as it is, the Go-friendly solution for the next btcd module tag would be to have a btcd/v2

IIUC, that wouldn't actually fix this issue, but instead would be intended to be more forwarding looking as it's essentially a clean "break" off the existing module?

Yah, more forward looking, to head off similar issues since the btcd module is so ubiquitous in the entire ecosystem. One person upgrades, and everything breaks until everyone upgrades. The btcd/v2 approach would mosltly solve that because an app can happily use both btcd(/v1) and btcd/v2 in the same app/lib (something that is apparently super common when considering transitive dependencies) without trouble because they are distinct. I expect that would come after all the other sub-module decisions are made though. Anyway, going forward with that means just bumping the modules major version so the import path changes whenever there are breaking changes, and as mentioned the product version tags like release-v0.23 can be mostly uncorrelated with the module tags that are the semver tags.

we wouldn't necessarily be concerned with issues when building/including with prior releases

I tend to take the same stance. I think it's partly still an issue because even with go 1.18 and 1.17 tooling you're required to explicitly command go mod tidy to not accommodate 1.16, even if the go.mod already says go 1.17, which is odd.

chappjc commented 2 years ago

But yeah, just bumping to chaincfg/chainhash/v2 and updating everything else for that would be simple.

Another idea that comes to mind is to move chainhash down from chaincfg, i.e. github.com/btcsuite/btcd/chainhash. I haven't really snooped enough to know, but it's not totally clear if it needs to live under chaincfg. Moving it could fix the import ambiguity as well. Just might be a less obvious change for consumers to track.

Roasbeef commented 2 years ago

I haven't really snooped enough to know, but it's not totally clear if it needs to live under chaincfg. Moving it could fix the import ambiguity as well. Just might be a less obvious change for consumers to track.

Yeah agreed. I can't really think of any reason it should live under chaincfg, since no matter what network of Bitcoin you're using (sim, test, main, etc), sha256 is still present.

chappjc commented 2 years ago

Which way are you thinking to go @Roasbeef? v2, move it up, or purify btcec of the require? I'm thinking one of the first two would be better since it's probably not just btcec that can lead to this, so just moving away from the ambiguous module seems best.

One thing that would be a headache is that btcutil would have to become btcutil/v2 also since chainhash types are in its API (and many many other package's APIs - more reasons for a btcd/v2 module before the next product release I think). Perhaps that's for the best though too because I'm sure there would eventually be similar ambiguous import issues with btcutil as well.

This also relates to https://github.com/btcsuite/btcd/pull/1825#issuecomment-1069439619. That plan seemed perfectly fine to me at the time, but seeing how these ambiguous imports can be a problem I wonder if it's safer just to start those at v2 (if they already existed as packages in the btcd module that is). The txscript package has breaking changes since v0.22.0-beta anyway so that really should start at v2

Roasbeef commented 2 years ago

v2, move it up, or purify btcec of the require

Leaning towards v2, if that means creating chainhash/v2, and then updating the other dependancies to use that.

subimage commented 2 years ago

require ( github.com/btcsuite/btcd v0.22.0-beta github.com/ethereum/go-ethereum v1.10.16
)

Thank you, this worked for me!

cyrildever commented 2 years ago

require ( github.com/btcsuite/btcd v0.22.0-beta github.com/ethereum/go-ethereum v1.10.16 )

Thank you, this worked for me!

Except there is an alert on go-ethereum <= v1.10.16 for a possible DOS attack, so we're stuck.

chappjc commented 2 years ago

@chappjc would making a v0.23.0 release help at all (even if not everyone adopts it right away)?

@jcvernaleo Going back to this, this could help if a v0.23 release branch were forked from the v0.22.0-beta tag, and on that branch things were update to use the chainhash submodule. This would mean that people would just need to change their v0.22.0-beta require to v0.23.0 and the ambiguous import issue would vanish.

This hypothetical branch couldn't be much further down master because of other breaking changes and that there's no longer a btcec package in the btcd module past a certain point, and the btcec package is probably the most imported package outside of btcsuite project.

As a bonus, it would put the gcash retract statement into effect and people would stop getting the v0.18 tag which (somewhat strangely) resolves as newer than v0.22.0-beta because of the -beta suffix.

Thoughts @davecgh? Maybe a v0.22 branch and a v0.22.1 tag would be just as good to solve the issue.

davecgh commented 2 years ago

From looking into this a bit there are a few issues at play and the cleanest way to resolve them, basing on my experience with managing all of the dcrd modules for a few years now is that what @chappjc recapped is the best way to resolve the issue.

Namely, as can be seen in the module release workflow docs, versions without pre-release components are preferred over those with them. In other words, v0.22.0-beta is seen as before v0.18.1, so currently anyone just importing or doing a plain go get github.com/btcsuite/btcd is going to attempt to get v0.18.1 which is a broken version. Addressing that first issue implies that a new tag without a pre-release version on it is required.

Next, the latest pre-release tag v0.22.0-beta has chaincfg/chainhash as a package, but there is also now a separate module with the same path. This is 100% guaranteed to break consumers who try to use both v0.22.0-beta and anything that simultaneously uses the new module such as btcec/v2, which there are already consumers in the ecosystem that need to do that. Addressing that issue also requires a new tag that is seen as later than v0.18.1 in which the chaincfg/chainhash package has been removed.

Finally, since there are a lot of other breaking changes and changes that are not necessarily ready for a release, what is really needed is a new e.g. v0.23.0 tag that is code branched off of v0.22.0-beta with only the problematic package removed and the code updated to use the module instead.

That way v0.23.0 will be seen as the latest release module by consumers, it will not have the package that is causing conflicts in it, and it will not include a bunch of code that is not yet ready for release.

You could also opt for a v0.22.1, btw, since it will be seen as newer as well. The important parts are that:

  1. The new tag is not a pre-release version so that it is seen as the most recent proper release by the go tooling
  2. The new tag must use the new chaincfg/chainhash module and not contain the chaincfg/chainhash package
  3. The new tag does not contain code that is not ready to be released yet.

EDIT: Note that this approach will also fix the fact that https://pkg.go.dev/github.com/btcsuite/btcd is showing v0.14.6 and the incorrect README and all.

davecgh commented 2 years ago

I opened #1851 which does what my comment and @chappjc suggested.

Roasbeef commented 2 years ago

Pushed a new tag here after the merge of #1851: https://github.com/btcsuite/btcd/releases/tag/v0.22.1

All consumer now should be able update to that tag, and then freely use the new chainhash and btcec modules w/o the import ambiguity weirdness.

davecgh commented 2 years ago

Confirmed the issues are now resolved:

$ go mod init foo; go get github.com/btcsuite/btcd; go get github.com/btcsuite/btcd/btcec/v2; go get github.com/ethereum/go-ethereum
go: creating new go.mod: module foo
go: added github.com/aead/siphash v1.0.1
go: added github.com/btcsuite/btcd v0.22.1
go: added github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1
go: added github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
go: added github.com/btcsuite/btcutil v1.0.3-0.20201208143702-a53e38424cce
go: added github.com/btcsuite/go-socks v0.0.0-20170105172521-4720035b7bfd
go: added github.com/btcsuite/goleveldb v1.0.0
go: added github.com/btcsuite/snappy-go v1.0.0
go: added github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792
go: added github.com/btcsuite/winsvc v1.0.0
go: added github.com/davecgh/go-spew v1.1.1
go: added github.com/decred/dcrd/lru v1.0.0
go: added github.com/jessevdk/go-flags v1.4.0
go: added github.com/jrick/logrotate v1.0.0
go: added github.com/kkdai/bstream v0.0.0-20161212061736-f391b8402d23
go: added golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37
go: added github.com/btcsuite/btcd/btcec/v2 v2.1.3
go: added github.com/decred/dcrd/dcrec/secp256k1/v4 v4.0.1
go: added github.com/ethereum/go-ethereum v1.10.17
go: upgraded golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37 => v0.0.0-20210322153248-0c34fe9e7dc2
go: upgraded golang.org/x/sys v0.0.0-20190412213103-97732733099d => v0.0.0-20210816183151-1e6c022a8912

Also the documentation is correct again as expected.

shrimalmadhur commented 2 years ago

Thanks everybody!

chappjc commented 2 years ago

Glad you got this resolved @shrimalmadhur! I'm not entirely sure your go get is needed or doing what's intended in your deps target given it's purposed change in recent Go versions to specifically updating the go.mod rather than building anything, but as long as you've got things building without hacks, I'm very pleased with the resolution.

shrimalmadhur commented 2 years ago

Interesting - let me verify once. I am still using Go 1.16 ( which eventually I will move to newer versions). I do have a separate install and build target. I guess eventually deps target can be removed (deps is no being used in any CI ). But thanks for the tip @chappjc

qdm12 commented 2 years ago

Hello all, I hope I won't spam this thread too much. However having read most of it, I can't figure out how to solve this dependency hell.

I have github.com/ethereum/go-ethereum v1.10.23 and github.com/btcsuite/btcd/btcec/v2 v2.2.1 // indirect requirements, but it fails with the error message

github.com/ChainSafe/gossamer/lib/crypto/secp256k1 imports
        github.com/ethereum/go-ethereum/crypto imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.22.0-beta (/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1 (/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.1)

I am also trying to upgrade go-libp2p* libraries, so maybe this has also something to do with it?

Would anyone be able to rescue me 🙏

git clone --single-branch --depth 1 --branch qdm12/deps/libp2p https://github.com/ChainSafe/gossamer.git
cd gossamer
go mod tidy # fails
go get github.com/btcsuite/btcd # upgraded github.com/btcsuite/btcd v0.22.0-beta => v0.23.1
go mod tidy # succeeds
go mod tidy # fails
chappjc commented 2 years ago

Pretty sure the issue in gossamer is the require on the old btcutil module that requires an old btcd. I think you'll want to change your btcutil import:

diff --git a/go.mod b/go.mod
index 50ff8f49..a13f3bfa 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ require (
        github.com/ChainSafe/go-schnorrkel v1.0.1-0.20220711122024-027d287d27bf
        github.com/OneOfOne/xxhash v1.2.8
        github.com/breml/rootcerts v0.2.6
-       github.com/btcsuite/btcutil v1.0.3-0.20201208143702-a53e38424cce
+       github.com/btcsuite/btcd/btcutil v1.1.2
        github.com/chyeh/pubip v0.0.0-20170203095919-b7e679cf541c
        github.com/cosmos/go-bip39 v1.0.0
        github.com/dgraph-io/badger/v2 v2.2007.4

and update your import paths too e.g.

--- a/dot/rpc/modules/system_integration_test.go
+++ b/dot/rpc/modules/system_integration_test.go
@@ -13,7 +13,7 @@
        "testing"
        "time"

-       "github.com/btcsuite/btcutil/base58"
+       "github.com/btcsuite/btcd/btcutil/base58"

etc