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

Update btcd #788

Closed
wants to merge 9 commits into from
Closed

Update btcd #788

wants to merge 9 commits into from

Conversation

3nprob
Copy link

@3nprob 3nprob commented Jan 25, 2022

Moving btcutil required as per #787.

Keeping in draft hoping that lightninglabs/neutrino#242 will be merged and forking it will not be necessary.

github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
github.com/btcsuite/btcutil v1.0.3-0.20210929233259-9cdf59f60c51
github.com/btcsuite/btcutil/psbt v1.0.3-0.20210929233259-9cdf59f60c51
github.com/btcsuite/btcwallet/wallet/txauthor v1.1.0
github.com/btcsuite/btcwallet/wallet/txrules v1.1.0
github.com/btcsuite/btcwallet/wallet/txsizes v1.1.0
github.com/btcsuite/btcwallet/walletdb v1.3.5
github.com/btcsuite/btcwallet/wtxmgr v1.3.0
Copy link
Contributor

@chappjc chappjc Jan 25, 2022

Choose a reason for hiding this comment

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

For a Go consumer of the btcwallet module to use btcwallet as an import where the replaces here aren't applied, this version of wtxmgr is too old. I had to update it to a pseudo version like v1.3.1-0.20211201210108-79de92f527dc in #782, which your PR would hopefully replace. Ideally the maintainers would tag this as wtxmgr 1.4.0 (or whatever), but otherwise a pseudo-version is needed either here (ideal) or in the go.mod of the third-party btcwallet consumer.

But, the btcwallet submodules that also require btcutil and btcd are:

  • wallet/txauthor
  • wtxmgr
  • wallet/txrules

I think I would update these submodules go.mod requires first, then come back to this main btcwallet go.mod to have it point to new pseudo-versions of these updated submodules.

Copy link
Author

@3nprob 3nprob Jan 26, 2022

Choose a reason for hiding this comment

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

Feedback appreciated; I'm currently a bit stuck ironing out all these in a way that makes everything align and build when imported in lnd. Hopefully I'll make some progress. Feel free to extend on this if you feel up for it.

Copy link
Author

@3nprob 3nprob Feb 1, 2022

Choose a reason for hiding this comment

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

I think I've addressed all your points now. All checks seem to work out as well. Unfortunately I have not been able to figure out a way to get everything to line up without also simultaneously using an updated version of neutrino (and same thing there, making this a bit cyclical).

As such, 9aa22b4 is here temporarily

Would be really good if we can make some further progress here so any assistance to get this mergable is very much appreciated.

Copy link
Contributor

@chappjc chappjc Feb 1, 2022

Choose a reason for hiding this comment

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

Just to clarify that I'm not a maintainer here, just another fellow consumer with an interest in seeing the updates.

In general I think the way forward needs to involve a little back and forth between the repos. With the wallet submodules (wallet/txauthor, wtxmgr, wallet/txrules) updated for the new btcutil, that would let neutrino update it's requires on those wallet submodules (in lightninglabs/neutrino#242, but without replaces), then come back to btcwallet and update it's neutrino require.

These gymnastics are happening because btcd is still v0.x, not yet following semantic versioning. The btcutil move is a breaking change to consumers with btcutil types in their exported APIs, and normally in Go land that would require a change to the semantic import versioning (SIV) in such consumers, e.g. btcd/v2 so that btcd consumers are not borken. This wouldn't mean the product would have to be released as v2.0, just that the root module would be v2 for Go purposes.

Unfortunately, the same breaking change I've described in btcd is about to happen to these three wallet submodules that are v1.x. So, it all depends on how the maintainers want to handle this (allow breaking changes thus violating SIV or bump these wallet submodules to /v2 and update any requires on them). There are already breaking changes in wtxmgr between v1.x tags anyway, so... I don't know.

Copy link
Author

Choose a reason for hiding this comment

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

Just to clarify that I'm not a maintainer here, just another fellow consumer with an interest in seeing the updates.

That was clear - the request for assistance was more a shout in the void than directly addressing you personally, though of course good to see you're still interested in this as well :)

I think I finally got what you were already trying to say above. Taking a new shot for the first step of that lock-step change. Really hoping maintainers will pop in here soon as quite a lot of things are blocked by this right now.

wallet/txauthor/author_test.go Show resolved Hide resolved
wallet/txrules/rules.go Outdated Show resolved Hide resolved
wtxmgr/tx.go Outdated Show resolved Hide resolved
@3nprob 3nprob marked this pull request as ready for review February 1, 2022 21:41
Seems necessary to have types match
@Roasbeef
Copy link
Member

Roasbeef commented Feb 4, 2022

@3nprob I can take this over and handle pushing out all the updated sub-module tags if you'd like?

@Roasbeef
Copy link
Member

Roasbeef commented Feb 4, 2022

I think I would update these submodules go.mod requires first, then come back to this main btcwallet go.mod to have it point to new pseudo-versions of these updated submodules.

Yes this is what we usually do: update the sub-modules, push a new tag, then update the top-level go.mod.

@Roasbeef
Copy link
Member

Roasbeef commented Feb 4, 2022

In general I think the way forward needs to involve a little back and forth between the repos. With the wallet submodules (wallet/txauthor, wtxmgr, wallet/txrules) updated for the new btcutil, that would let neutrino update it's requires on those wallet submodules (in lightninglabs/neutrino#242, but without replaces), then come back to btcwallet and update it's neutrino require.

Yep this is usually how we do things. Admittedly, as we maintain btcwallet+neutrino+lnd, etc -- we at times cut a few corners when it comes to modules/tagging. Though happy to clean things up, so things are less annoying for other dependent users like y'all.

@Roasbeef
Copy link
Member

Roasbeef commented Feb 5, 2022

I can take this over and handle pushing out all the updated sub-module tags if you'd like?

Started to port everything over and confirmed that we are indeed in module dep hell rn....

This should remedy things though: btcsuite/btcd#1805

After that, we need a smol btcutil tag, then we should be able to update everything (hit this snag when I tried locally).

@Roasbeef
Copy link
Member

Roasbeef commented Feb 7, 2022

This PR updates just the sub-modules: #789

@Roasbeef
Copy link
Member

Roasbeef commented Mar 8, 2022

This has been done in existing PRs, closing, thanks for starting this!

@Roasbeef Roasbeef closed this Mar 8, 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.

3 participants