Assorted 0.0.116 Bindings updates by TheBlueMatt · Pull Request #2430 · lightningdevkit/rust-lightning

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt · 2023-07-18T20:15:48Z

Probably a few more to come, but here's the first batch.

The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

codecov-commenter · 2023-07-19T01:35:25Z

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.04 🎉

Comparison is base (c5c5f3f) 90.28% compared to head (35dda4e) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2430 +/- ##
==========================================
+ Coverage 90.28% 90.32% +0.04% 
==========================================
Files 106 106 Lines 55664 55977 +313 Branches 55664 55977 +313 ==========================================
+ Hits 50254 50564 +310 - Misses 5410 5413 +3

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`93.49% <ø> (-0.02%)`	⬇️
lightning/src/events/bump_transaction.rs	`79.39% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/outbound_payment.rs	`89.20% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`97.66% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.35% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`72.46% <100.00%> (-0.12%)`	⬇️

... and 5 files with indirect coverage changes

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

wpaulino · 2023-07-19T16:25:21Z

LGTM but there's one more build error in fuzz

TheBlueMatt · 2023-07-19T17:09:01Z

Oops, sorry, fixed fuzz build.

In bindings we can't practically pass a mutable transaction, and instead need to pass an owned transaction and have the sign method return a signed copy. We do this here for all build modes as the API is roughly equivalent also to Rust users.

We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

Tweak generics on derive_channel_signer to be bindings-compatible
1e119a2
The C bindings generation currently has issues looking through a generic associated type. While this should be fixed in the bindings generator, its easy to fix here for now and we can revisit it later.

TheBlueMatt added this to the 0.0.116 milestone Jul 18, 2023

TheBlueMatt force-pushed the 2023-07-116-bindings-part-1 branch from 297aa01 to 7ed4810Compare July 18, 2023 20:29

wpaulino reviewed Jul 18, 2023
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2023-07-116-bindings-part-1 branch from 7ed4810 to eca9c6cCompare July 19, 2023 00:29

TheBlueMatt force-pushed the 2023-07-116-bindings-part-1 branch from eca9c6c to 440f6f2Compare July 19, 2023 17:08

wpaulino previously approved these changes Jul 19, 2023
View reviewed changes

TheBlueMatt mentioned this pull request Jul 20, 2023
0.0.116 Bindings Changes #2434
Merged

jkczyz reviewed Jul 20, 2023
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs Outdated

TheBlueMatt added 4 commits July 20, 2023 19:49

Pass must-spend inputs to users by ownership
0c629ff
We already hold them in a vec, so there's no cost to passing them by ownership vs making it a slice. Further, this helps bindings as we can't represent slices to non-pointers in a sensible way.

Pass InFlightHltcs to the scorer by ownership rather than ref
d2c20ec
Given we build `InFlightHtlcs` per route-fetch call, there's no reason to pass them out by reference rather than simply giving the user the full object. This also allows them to tweak the in-flight set before fetching a route.

Drop c_bindings implementation of scoring on Mutex/RefCell
35dda4e
This code was always effectively dead - we have a special `MultiThreadedLockableScore` type which wraps a `Mutex` for bindings users, so there's no need to implement any bindings-specific scoring logic for them.

TheBlueMatt dismissed wpaulino’s stale review via 35dda4eJuly 20, 2023 19:49

TheBlueMatt force-pushed the 2023-07-116-bindings-part-1 branch from 440f6f2 to 35dda4eCompare July 20, 2023 19:49

wpaulino approved these changes Jul 20, 2023
View reviewed changes

jkczyz approved these changes Jul 20, 2023
View reviewed changes

TheBlueMatt merged commit e4c44f3 into lightningdevkit:mainJul 20, 2023