[0.0.111-bindings] Bindings Branch Rebase + Backports by TheBlueMatt · Pull Request #1730 · lightningdevkit/rust-lightning

TheBlueMatt · 2022-09-19T15:38:35Z

Rebase of the existing 0.0.110-bindings branch, with the score concretization stuff skipped since we don't need it anymore, and a few additional commits that are pretty trivial. The first few commits are backports of #1694, #1728, and #1729.

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

In c353c3e, new scorer-updating methods were added to the `Router` object, however they were passed as a `Vec` of references. We use the list-of-references pattern to make bindings simpler (by not requiring allocations per entry), however there's no reason prefer to passing a `Vec` over a slice, given the `Vec` doesn't hold ownership of the objects anyway.

In c353c3e an accessor method was added which returns an `Option<&u64>`. While this allows Rust to return an 8-byte object, returning a reference to something pointer-sized is a somewhat strange API. Instead, we opt for a straight `Option<u64>`, which is sadly somewhat larger on the stack, but is simpler and already supported in the bindings generation.

codecov-commenter · 2022-09-19T15:56:27Z

Codecov Report

Base: 90.86% // Head: 90.86% // No change to project coverage 👍

Coverage data is based on head (4ae65e8) compared to base (4ae65e8).
Patch has no changes to coverable lines.

❗ Current head 4ae65e8 differs from pull request most recent head 0da9554. Consider uploading reports for the commit 0da9554 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## 0.0.111-bindings #1730 +/- ##
=================================================
Coverage 90.86% 90.86% =================================================
Files 86 86 Lines 46482 46482 Branches 46482 46482 =================================================
Hits 42234 42234 Misses 4248 4248

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

valentinewallace

Basically LGTM

valentinewallace · 2022-09-22T16:50:41Z

 #[cfg(c_bindings)]
-/// (C-not exported)
+/// A locked `MultiThreadedLockableScore`.
+pub struct MultiThreadedLockableScoreLock<'a, S: Score>(MutexGuard<'a, S>);


Oof. Maybe MultiThreadedScoreLock?

This already went upstream in #1729, I'll do this in a PR for upstream but will have to land there first.

`hashbrown` depends on `ahash` which depends on `once_cell`. Sadly, in matklad/once_cell#201 the `once_cell` maintainer decided they didn't want to do the work of having an MSRV policy for `once_cell`, making `ahash`, and thus `hashbrown` require the latest compiler. I've reached out to `ahash` to suggest they drop the dependency (as they could trivially work around not having it), but until then we simply downgrade `hashbrown`. `rust-bitcoin` also requires an older `hashbrown` so we're actually reducing our total `no-std` code here anyway.

In the bindings, we don't directly export any `std` types. Instead, we provide trivial wrappers around them which expose only a bindings-compatible API (and which is code in-crate, where the bindings generator can see it). We never quite finished this for `MultiThreadedLockableScore` - due to some limitations in the bindings generator and the way the scores are used in `lightning-invoice`, the scoring API ended up being further concretized in patches for the bindings. Luckily the scoring interface has been rewritten somewhat, and it so happens that we can now generate bindings with the upstream code. The final piece of the puzzle is done here, where we add a struct which wraps `MutexGuard` so that we can expose the lock for `MultiThreadedLockableScore`.

...as the bindings generation does not currently have the ability to map a reference to a `NodeId` inside a tuple.

.. as the current C bindings generator isn't capable of handling type aliases in generics in type alias definition currently.

Bindings can't handle references in return types, so reduce the visibility to pub(crate).

Currently `Writeable` is mapped manually, making it impossible to automatically map a trait method that is parameterized by `Writeable` (as is true for the `write` method on `KVStore`). Ultimately we'll want to move to automatically mapping `Writeable` like any other trait (only manually mapping the std `Write` and `Read` traits), so this is only a candidate for the bindings branch, not upstream. That may take a few releases, however.

Re-exports in Rust make `use` statements a little shorter, but for otherwise don't materially change a crate's API. Sadly, the C bindings generator currently can't figure out re-exports, but it also exports everything into one global namespace, so it doesn't matter much anyway.

valentinewallace

I don't feel the most qualified to sign off on the first commit but LGTM

TheBlueMatt · 2022-09-23T16:09:51Z

The first commit is a backport :)

TheBlueMatt · 2022-09-19T15:38:35Z

Rebase of the existing 0.0.110-bindings branch, with the score concretization stuff skipped since we don't need it anymore, and a few additional commits that are pretty trivial. The first few commits are backports of #1694, #1728, and #1729.

We do this to enable users to create routers that do not need a scorer. This can be useful if they are running a node the delegates pathfinding. * Move `Score` type parameterization from `InvoicePayer` and `Router` to `DefaultRouter` * Adds a new field, `scorer`, to `DefaultRouter` * Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we will use to wrap the new `scorer` field, so scoring only happens in `DefaultRouter` explicitly. * Add scoring related functions to `Router` trait that we used to call directly from `InvoicePayer`. * Instead of parameterizing `scorer` in `find_route`, we replace it with inflight_map so `InvoicePayer` can pass on information about inflight HTLCs to the router. * Introduced a new tuple struct, InFlightHtlcs, that wraps functionality for querying used liquidity.

In c353c3e, new scorer-updating methods were added to the `Router` object, however they were passed as a `Vec` of references. We use the list-of-references pattern to make bindings simpler (by not requiring allocations per entry), however there's no reason prefer to passing a `Vec` over a slice, given the `Vec` doesn't hold ownership of the objects anyway.

In c353c3e an accessor method was added which returns an `Option<&u64>`. While this allows Rust to return an 8-byte object, returning a reference to something pointer-sized is a somewhat strange API. Instead, we opt for a straight `Option<u64>`, which is sadly somewhat larger on the stack, but is simpler and already supported in the bindings generation.

codecov-commenter · 2022-09-19T15:56:27Z

Codecov Report

Base: 90.86% // Head: 90.86% // No change to project coverage 👍

Coverage data is based on head (4ae65e8) compared to base (4ae65e8).
Patch has no changes to coverable lines.

❗ Current head 4ae65e8 differs from pull request most recent head 0da9554. Consider uploading reports for the commit 0da9554 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## 0.0.111-bindings #1730 +/- ##
=================================================
Coverage 90.86% 90.86% =================================================
Files 86 86 Lines 46482 46482 Branches 46482 46482 =================================================
Hits 42234 42234 Misses 4248 4248

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

valentinewallace

Basically LGTM

valentinewallace · 2022-09-22T16:50:41Z

 #[cfg(c_bindings)]
-/// (C-not exported)
+/// A locked `MultiThreadedLockableScore`.
+pub struct MultiThreadedLockableScoreLock<'a, S: Score>(MutexGuard<'a, S>);
