bitcoin 0.32 upgrade followups by TheBlueMatt · Pull Request #3249 · lightningdevkit/rust-lightning

TheBlueMatt · 2024-08-16T21:44:06Z

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

codecov · 2024-08-16T22:30:10Z

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 ⚠️

Additional details and impacted files

@@ 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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tnull

Mostly looks good to me, just a few questions.

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.

CI-shellcheck is unhappy, btw.

tnull · 2024-08-17T06:41:14Z

 	}
 }

+// Drop this entirely if rust-bitcoin releases a version bump with https://github.com/rust-bitcoin/rust-bitcoin/pull/3173


Prefix with TODO or FIXME for discoverability? Maybe also open an issue for it to make really sure we don't forget as shipping this might have quite an impact on efficiency?

TheBlueMatt · 2024-08-17T20:04:52Z

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 · 2024-08-17T21:14:22Z

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 · 2024-08-18T07:24:04Z

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 · 2024-08-18T07:24:43Z

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

Feel free to squash the fixup.

TheBlueMatt · 2024-08-18T13:43:17Z

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.

arik-so · 2024-08-18T16:57:12Z

 done

-for DIR in lightning lightning-invoice lightning-rapid-gossip-sync; do
+for DIR in lightning lightning-rapid-gossip-sync; do


do we need a separate loop for the bindings tests?

Grouping by RUSTFLAGS avoids rebuilding the dependencies again and again.

Kixunil · 2024-08-19T12:44:33Z

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.

tnull

LGTM, I think.

Feel free to squash the fixups. I don't feel too strongly about patch vs. paths, so feel free to go either way.

TheBlueMatt · 2024-08-19T13:59:36Z

Squashed without further changes, gonna keep paths everywhere.

When we reach EOF we may return a full buffer when we should return an empty one.

`rust-bitcoin` doesn't ever actually *use* its `BufRead` requirement when deserializing objects, and forcing it is somewhat inefficient, so we optimize the only (actual) case here by passing reads straight through to the backing stream.

In order to ensure our crates depend on the workspace copies of each other in test builds we need to override the crates.io dependency with a local `path`. We can do this in one of two ways - either specify the `path` in the dependency listing in each crate's `Cargo.toml` or use the workspace `Cargo.toml` to `patch` all dependencies. The first is tedious while the second lets us have it all in one place. However, the second option does break `cargo *` in individual crate directories (forcing the use of `cargo -p crate *` instead) and makes it rather difficult to depend on local versions of workspace crates. Thus, here we drop the `patch.crates-io` from our top-level `Cargo.toml` entirely. Still, we do update the `ci/ci-tests.sh` script here to use `cargo -p crate` instead of switching to each crate's directory as it allows `cargo` to use a shared `target` and may speed up tests.

Now that we don't have to have everything in our entire ecosystem use the same `std`/`no-std` feature combinations we should start by untangling our own features a bit. This takes the first step by removing the `no-std` feature entirely from the `lightning-background-processor` crate and removing most feature implications on dependencies from the remaining `std` feature. It also addresses a CI oversight where we were not testing `lightning-background-processor` without the `std` feature in CI at all.

Now that we don't have to have everything in our entire ecosystem use the same `std`/`no-std` feature combinations we should start by untangling our own features a bit. This takes another step by removing the `no-std` feature entirely from the `lightning-invoice` crate and removing all feature implications on dependencies from the remaining `std` feature.

Now that we don't have to have everything in our entire ecosystem use the same `std`/`no-std` feature combinations we should start by untangling our own features a bit. This takes another step by removing the `no-std` feature entirely from the `lightning-rapid-gossip-sync` crate and removing all feature implications on dependencies from the remaining `std` feature.

This exists just for tests, so there's no reason for it to be publicly visible.

Now that we don't have to have everything in our entire ecosystem use the same `std`/`no-std` feature combinations we should start by untangling our own features a bit. This takes one last step, removing the implications of the `std` feature from the `lightning` crate.

TheBlueMatt · 2024-08-19T15:10:12Z

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

arik-so

lgtm!

vincenzopalazzo

LGTM, a little bit not sure about the rust-bitcoin (rust-bitcoin/rust-bitcoin#3173) but I think I just need to read the rust-bitcoin PR in deep

TheBlueMatt · 2024-08-16T21:44:06Z