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

Bump rust-bitcoin to 0.30.0 #2124

Closed
benthecarman opened this issue Mar 23, 2023 · 18 comments · Fixed by #2740
Closed

Bump rust-bitcoin to 0.30.0 #2124

benthecarman opened this issue Mar 23, 2023 · 18 comments · Fixed by #2740
Assignees

Comments

@benthecarman
Copy link
Contributor

benthecarman commented Mar 23, 2023

Might not be a fun one 😅

bitcoindevkit/bdk#916

@TheBlueMatt TheBlueMatt added this to the 0.0.116 milestone Mar 27, 2023
@wpaulino wpaulino self-assigned this Apr 27, 2023
@wpaulino
Copy link
Contributor

The bump in lightning-block-sync is blocked on rust-bitcoin/rust-bitcoin#1820.

@TheBlueMatt
Copy link
Collaborator

Turned out to be complicated and 116 has too much else going on.

@tcharding
Copy link
Contributor

tcharding commented Nov 2, 2023

You guys don't really need me poking my nose in but you likely want to jump to 0.31.0 because you need left and right shift on Target. @wpaulino knows whats up. Flagging in case anyone else stumbles on this.

ref: rust-bitcoin/rust-bitcoin#1820

@TheBlueMatt TheBlueMatt removed this from the 0.0.119 milestone Nov 4, 2023
@tnull
Copy link
Contributor

tnull commented Nov 7, 2023

Mh, any chance this would still land for 0.0.119? BDK's 1.0 branch already updated to rust-bitcoin 0.30 a while back, so in order to start using it in LDK Node eventually, LDK would need to be API compatible with the re-exported types...

I suspect this is also the reason why Mutiny still pins BDK 1.0.0-alpha.1, @benthecarman ?

@benthecarman
Copy link
Contributor Author

I suspect this is also the reason why Mutiny still pins BDK 1.0.0-alpha.1, @benthecarman ?

Yes, we can't use 0.30 until LDK does

@TheBlueMatt
Copy link
Collaborator

Sure, absolutely, if they get the difficulty calculation stuff over the line it can make whatever release it makes. I do not, however, think we should hold any release until this lands.

@tcharding
Copy link
Contributor

So you guys need rust-bitcoin/rust-bitcoin#2090 merged and backported to 0.30.1 as well as 0.31.1, right?

I can put it at the top of my priority list.

cc @vincenzopalazzo

@vincenzopalazzo
Copy link
Contributor

So you guys need rust-bitcoin/rust-bitcoin#2090 merged and backported to 0.30.1 as well as 0.31.1, right?

Is ready for review, this PR is blocking me on nakamoto, but I think I will have a followup regarding the API.

@wpaulino
Copy link
Contributor

With 0.31.0, we already have all we need. We don't need rust-bitcoin/rust-bitcoin#2090.

@tcharding
Copy link
Contributor

Oh yeah? Magic, that makes my life easier by far.

@tcharding
Copy link
Contributor

tcharding commented Nov 15, 2023

Do you want the shift stuff backported to 0.30.0 or are you happy jumping over the top of that release? The 0.30 to 0.31 upgrade is not painful like the 0.29 to 0.30 one :)

@tnull
Copy link
Contributor

tnull commented Nov 15, 2023

Do you want the shift stuff backported to 0.30.0 or are you happy jumping over the top of that release? The 0.30 to 0.31 upgrade is not painful like the 0.29 to 0.30 one :)

One benefit of the backport would be that we'd immediately regain compatibility with BDK which had been on 0.30 for a while but is blocked on upgrading to 0.31 due to rust-miniscript not being upgraded (which in turn is blocked on rust-bitcoincore-rpc, etc..., as you are aware). Given that we have users waiting on being able to use the most-recent BDK and LDK versions, having the necessary changes backported would be awesome, at least if it's not too much work on your end.

We could therefore first upgrade to 0.30 and then better coordinate the upgrade to 0.31 with BDK (cc @notmandatory)

@Kixunil
Copy link
Contributor

Kixunil commented Nov 15, 2023

BDK's 1.0 branch already updated to rust-bitcoin

What the actual hell?! Are they exposing rust-bitcoin types in the API and trying to stabilize? If so, that's a very bad idea as we expect things to break in the future. I even know some specific things that will very likely break.

@tnull
Copy link
Contributor

tnull commented Nov 15, 2023

What the actual hell?! Are they exposing rust-bitcoin types in the API and trying to stabilize? If so, that's a very bad idea as we expect things to break in the future. I even know some specific things that will very likely break.

Ehm, yes, we all use rust-bitcoin re-exported types, also in the API. Even simple types such as bitcoin::Txid or bitcoin::Transaction won't be compatible if the used rust-bitcoin dependencies are not.

The discussion about the semantics of stabilisation and "when 1.0" is likely off-topic here. However, it's still important that our users can upgrade LDK / BDK libraries in a somewhat reasonable timeframe. And they can't as long as each library depends on a different (API-incompatible) version of rust-bitcoin.

@Kixunil
Copy link
Contributor

Kixunil commented Nov 15, 2023

@tnull we could move the discussion here: bitcoindevkit/bdk#1215

Yeah, I suspected that basic types like Transaction would be what's exposed in the API. Do you think it'd be useful to focus on stabilizing those ASAP in a separate library so that you could publicly depend on those while still being able to depend on full bitcoin crate internally? I have some ideas how to do that reasonably.

@tnull
Copy link
Contributor

tnull commented Nov 15, 2023

@tnull we could move the discussion here: bitcoindevkit/bdk#1215

Yeah, I suspected that basic types like Transaction would be what's exposed in the API. Do you think it'd be useful to focus on stabilizing those ASAP in a separate library so that you could publicly depend on those while still being able to depend on full bitcoin crate internally? I have some ideas how to do that reasonably.

I mean maybe this would be a long-term solution. However, LDK and BDK really also make use of some of the advanced types and might also want to expose some of it in our API as we're modular low-level libraries ourselves. My personal view is that short- to mid-term we in the rust-bitcoin ecosystem should simply communicate better and do a better job coordinating API-breaking version upgrades when they could affect downstream users that depend on multiple crates (which is why I really appreciate @tcharding's initiative here).

@Kixunil
Copy link
Contributor

Kixunil commented Nov 15, 2023

Please see rust-bitcoin/rust-bitcoin#2160 (comment)

As for advanced stuff, I see these options: put that behind unstable feature flag, put it into a separate crate, we try harder to support more types (e.g. also keys, not just transactions).

@tcharding
Copy link
Contributor

Coming at ya! rust-bitcoin/rust-bitcoin#2196

apoelstra added a commit to rust-bitcoin/rust-bitcoin that referenced this issue Nov 16, 2023
ba99aa4 Bump version to 0.30.2 (Tobin C. Harding)
3c9bbca Expose valid (min, max) difficulty transition thresholds (Wilmer Paulino)

Pull request description:

  Patch one backports #1820, I believe this is all that is needed for LDK to be able to upgrade to `rust-bitcoin v0.30`.

  Context:

  - #1820
  - lightningdevkit/rust-lightning#2124

  Patch 2 adds a changelog entry and bumps the minor version.

ACKs for top commit:
  Kixunil:
    ACK ba99aa4

Tree-SHA512: 93adb52bbc8e61ece63e5653dea7cf6d2d25028f25cb8bbda14606d9ebf74becaee0a0c4f4f3d7fde56c0e2b8a57f8c9be263e04833161a1da0baaf8fcca8764
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 a pull request may close this issue.

7 participants