[0.0.111-bindings] Bindings Branch Rebase + Backports #1730
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 ReportBase: 90.86% // Head: 90.86% // No change to project coverage 👍
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. |
TheBlueMatt
force-pushed
the
2022-09-111-bindings-and-backports
branch
2 times, most recently
from
0da9554 to
22e9709
Compare
| @@ -188,12 +188,39 @@ pub struct MultiThreadedLockableScore<S: Score> { | |||
| score: Mutex<S>, | |||
| } | |||
| #[cfg(c_bindings)] | |||
| /// (C-not exported) | |||
| /// A locked `MultiThreadedLockableScore`. | |||
| pub struct MultiThreadedLockableScoreLock<'a, S: Score>(MutexGuard<'a, S>); | |||
There was a problem hiding this comment.
Oof. Maybe MultiThreadedScoreLock?
There was a problem hiding this comment.
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.
TheBlueMatt
force-pushed
the
2022-09-111-bindings-and-backports
branch
from
22e9709 to
30a8d18
Compare
valentinewallace
approved these changes
Sep 23, 2022
|
The first commit is a backport :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.