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

btcec: remove import of chainhash #1848

Closed
wants to merge 1 commit into from

Conversation

chappjc
Copy link
Contributor

@chappjc chappjc commented Apr 25, 2022

One option to quickly resolve #1839

This duplicates some code, but requires no import path changes, only a new btcec/v2 tag. For a proper solution, a chainhash/v2 module is still required (e.g. 39b1193 plus a large cascade of deps like btcutil/v2 and all the way up) since it's not just btcec that will introduce this ambiguous import.

The chainhash import is challenging for consumers of the latest tagged
btcd module (v0.22.0-beta) because that module revision has its own
chainhash package.  This creates an ambiguous import when trying
to import both the older btcd module and btcec/v2 which requires the
standalone module version of chainhash.

This is the brute solution.  An alternative would be to create a
chaincfg/chainhash/v2 module and have btcec/v2 require that instead.

The chainhash import is challenging for consumers of the latest tagged
btcd module (v0.22.0-beta) because that module revision has its own
chainhash package.  This creates an ambiguous import when trying
to import both the older btcd module and btcec/v2 which requires the
standalone module version of chainhash.

This is the brute solution.  An alternative would be to create a
chaincfg/chainhash/v2 module and have btcec/v2 require that instead.
@jcvernaleo
Copy link
Member

I'm comfortable with this solution but would love to hear from @Roasbeef before approving/merging.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2220593558

  • 30 of 32 (93.75%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 54.278%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcec/schnorr/signature.go 30 32 93.75%
Files with Coverage Reduction New Missed Lines %
btcutil/gcs/gcs.go 1 81.25%
peer/peer.go 4 73.2%
Totals Coverage Status
Change from base Build 2186219467: 0.01%
Covered Lines: 25192
Relevant Lines: 46413

💛 - Coveralls

@chappjc
Copy link
Contributor Author

chappjc commented Apr 25, 2022

Yah, the v2 option seemed simpler but it has cascading effects. e.g. btcutil has to go to v2 since chainhash types are in it's exported API, which is breaking for txscript, wire, etc.

The more I think about this and see how it has been playing out for consumers, the more I think the "right" solution for the current btcd dev cycle (before your next product release) is to get to btcd/v2 (module, not product) right away, before all other module activities, because countless projects import the btcd(/v0) module. Then get btcd/v2 on a bunch of new v2 submodules. Otherwise, the legacy btcd module will become more and more problematic, especially if any new submodule imports it or anything ambiguously from it, such as chainhash. #1825 (comment)

Unless the plan to to make the btcd module something that is never imported (just there for the CLI app), only the submodules imported, it really is necessary because of breaking changes since the last v0.22 tag.

Whether we like it or not, with the invent of Go modules, the entire ecosystem is essentially forced to bump their semantic import version (the /vX on the import path) if there are any breaking changes. While more modules => more burden for btcsuite, carving out sub-modules can at least reduce the frequency of such bumps. It's a balance that's hard to get right. (And it requires far to much thought to get right IMO, but that's the state of things)

@Roasbeef
Copy link
Member

I'm on board, but think we should wait for this to be merged first: #1820

It defines some new tagged hash combinations, and just needs another pass on my end up update the nonce derivation to the latest musig2 BIP. I don't really foresee many other standardized "tagged hash" prefixes in the near future, and needing to add to a new map element in two areas (optional since they can pass a custom tag in any case) isn't too bad.

@Roasbeef
Copy link
Member

If we're going along with #1850 then I think it makes sense to actually merge this first, so the new slimmer module retains the old Go version requirement.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 28, 2022

Converting to draft as I'm actually fairly hopeful about the new resolution proposed in the comment #1839 (comment). Happy to repoen this though if it seems desirable regardless.

@chappjc
Copy link
Contributor Author

chappjc commented Apr 28, 2022

This is fortunately no longer required. 🥳

@chappjc chappjc closed this Apr 28, 2022
fardream added a commit to fardream/go-dydx that referenced this pull request May 25, 2022
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.

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