lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.15k stars 363 forks source link

bitcoin 0.32 upgrade followups #3249

Closed TheBlueMatt closed 1 month ago

TheBlueMatt commented 1 month ago

Mostly feature list cleanups now that we can, but also tiny tweaks to BufReader.

Fixes #3097

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 26.66667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.72%. Comparing base (dd37077) to head (8049f99). Report is 13 commits behind head on main.

Files Patch % Lines
lightning/src/util/ser.rs 15.38% 8 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3249 +/- ## ======================================= Coverage 89.72% 89.72% ======================================= Files 124 124 Lines 102386 102352 -34 Branches 102386 102352 -34 ======================================= - Hits 91867 91840 -27 + Misses 7819 7816 -3 + Partials 2700 2696 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheBlueMatt commented 1 month ago

I'm honestly not entirely sure about the patch changes yet. Will have to see how working with hard-coded patch will be in practice.

When working directly on LDK it seems to be fine (just have to get used to cargo -p) but it may be a bit more annoying for you cause you'll need to manually patch all the LDK crates if you're working on a project that depends on LDK and you want to point to a local lightning. I can walk it back and do it all via path if you feel strongly, but rust-bitcoin is doing it this way it it seemed a bit nicer than the current half-fixes we have in-tree.

TheBlueMatt commented 1 month ago

Added two more commits but I think sadly we can't materially change the feature set on the lightning crate just cause we have no-std-specific dependencies, which Cargo.toml can't represent. I think with this PR as-is we can cut an 0.0.124 beta.

tnull commented 1 month ago

but it may be a bit more annoying for you cause you'll need to manually patch all the LDK crates if you're working on a project that depends on LDK and you want to point to a local lightning. I can walk it back and do it all via path if you feel strongly, but rust-bitcoin is doing it this way it it seemed a bit nicer than the current half-fixes we have in-tree.

Mh, yeah, that's what I thought. It would def. make my life easier if we'd find a solution that would allow me to patch the whole workspace with a single command as I frequently do so for lightning-liquidity and LDK Node, but reverting to the way it has been (i.e., having to patch all of them individually) isn't the end of the world.

tnull commented 1 month ago

CI is unhappy as you seem to have introduced a bunch of (mostly unused import) warnings.

Feel free to squash the fixup.

TheBlueMatt commented 1 month ago

Mh, yeah, that's what I thought. It would def. make my life easier if we'd find a solution that would allow me to patch the whole workspace with a single command

Okay, moved them all to paths in every Cargo.toml. I'll make my life easier for bindings, too, just a lot more verbose.

Kixunil commented 1 month ago

FYI regarding [patch] we use it because we have a "dependency hole". secp256k1 is not in our git tree so if we didn't use [patch] we would have no way to tell it to use the new version of hashes which it optionally depends on. [patch] solves this well. If you don't have such problem then it's not clear to me which approach is better.

TheBlueMatt commented 1 month ago

Squashed without further changes, gonna keep paths everywhere.

TheBlueMatt commented 1 month ago

Rebased (and fixed the commit message to note that we are doing the path option not the patch one).