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

mod: remove btcutil circular dependency with main module #1825

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Mar 10, 2022

Depends on #1824.

Removes the circular dependency between the main module and the btcutil sub module by introducing a bunch of new sub modules.

@guggero
Copy link
Collaborator Author

guggero commented Mar 12, 2022

Rebased after dependent PR was merged.
@Roasbeef PTAL.

@chappjc
Copy link
Contributor

chappjc commented Mar 13, 2022

Thanks a ton for both #1823 and #1824.
🙏
Really helpful to a lot of consumers, I think.

In this PR, I'm curious about the new submodules: btcutil/address, btcutil/bech32, btcutil/psbd, and btcutil/base58. I figured those would be cool to remain just as packages in the one btcutil module since making them into modules doesn't seem to break free of any obviously dependency issues like with wire, txscript, etc. I would just hate to see an unnecessary maintenance burden for you guys with too many modules. :P

Thanks again guys! I don't mean to overstep with my questions, just curious and wanting to help.

@guggero
Copy link
Collaborator Author

guggero commented Mar 14, 2022

Thanks for your input, @chappjc!
I was able to remove two of the new modules by just putting btcutil/base58 and btcutil/bech32 below the new btcutil/address module.
We still require a separate module for btcutil/address because that resolves the circular dependency between txscript and btcutil.
Before it was btcutil -> txscript -> btcutil, now it's btcutil -> txscript -> btcutil/address.

@chappjc
Copy link
Contributor

chappjc commented Mar 15, 2022

We still require a separate module for btcutil/address because that resolves the circular dependency between txscript and btcutil.

Ahh, yes, Address et al. is in btcutil!

Down in dcrd, @davecgh resolved this by introducing a stdaddr package under the txscript module (decred/dcrd#2610), then breaking txscript's dependency on dcrutil (decred/dcrd#2626, decred/dcrd#2628). That was a job and a half.

Anyway, a btcutil/address module makes sense to quickly break txscript's require on btcutil.

@guggero
Copy link
Collaborator Author

guggero commented Mar 16, 2022

Rebased on top of #1831.

@coveralls
Copy link

coveralls commented Mar 16, 2022

Pull Request Test Coverage Report for Build 2050836320

  • 22 of 168 (13.1%) changed or added relevant lines in 15 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-9.06%) to 45.122%

Changes Missing Coverage Covered Lines Changed/Added Lines %
config.go 0 2 0.0%
mining/mining.go 0 4 0.0%
rpcclient/extensions.go 0 4 0.0%
rpcclient/mining.go 0 4 0.0%
rpcclient/notify.go 0 5 0.0%
rpcclient/rawtransactions.go 0 8 0.0%
blockchain/indexers/addrindex.go 0 10 0.0%
rpcwebsocket.go 0 15 0.0%
rpcserver.go 0 19 0.0%
rpcclient/wallet.go 0 75 0.0%
Totals Coverage Status
Change from base Build 2006933401: -9.06%
Covered Lines: 16525
Relevant Lines: 36623

💛 - Coveralls

@guggero
Copy link
Collaborator Author

guggero commented Mar 16, 2022

Did a final rebase.

After merging, please push tags for the following commits:

@chappjc
Copy link
Contributor

chappjc commented Mar 16, 2022

From my point of view (a consumer), this is looking great. Looks like one of the last packages I import that will still come from the main btcd mega-module is btcjson (which imports some of the new modules you're making in this PR).

Other than btcjson, the only other thing that stands out is github.com/btcsuite/btcd/peer and maybe blockchain coming indirectly though both neutrino (neutrino.ServerPeer embeds a *peer.Peer as an exported field, so it's part of the neutrino API) and btcwallet/chain. There's probably others, but this is looking great so far!

@chappjc
Copy link
Contributor

chappjc commented Apr 22, 2022

Given recent issues, txscript should really start at txscript/v2 on account of the breaking changes since v0.22.0-beta. Two issues have popped up since where a txscript build error was hit because of the API changes: #1845 and #1843 (comment)

Combined with the "ambiguous import" issues with chainhash recently (#1839), I think all new modules should start at v2 to be safe (if packages of the same name had previously existed, so not btcutil/address), and existing ones that have those types in their API's (e.g. btcutil has in it's API types from chaincfg, chainhash, and wire) should be updated and have their module import path bumped as well. The order with which all these modules are created is obviously going to require care, but I think it needs to be done if these modules are going to be created.

Lastly, I think the best move for the next btcd module tag should be a btcd/v2 module at v2.0.0, while a corresponding product tag like release-v0.23 can be created as long as it is not a semver tag to confuse the Go tooling that treats semver tags with special meaning w.r.t. modules. Potentially the root btcd/v2 module could be created before being updated to use any of the new modules, but I'm not sure.

@Roasbeef
Copy link
Member

Care to rebase this? 😅

@guggero guggero force-pushed the btcec-v2-no-circular-dep branch 2 times, most recently from 2aec8cb to 416b4f0 Compare December 19, 2023 12:32
@guggero
Copy link
Collaborator Author

guggero commented Dec 19, 2023

I rebased everything and created new v2 module for a couple of intertwined packages.
I think with this we can get away with NOT also creating a v2 for the main btcd module, since once we push a new tag, everything will be pulled in as v2 and the "ambiguous import" problem should go away. What do you think, @chappjc?

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