Various cleanups to make bindings generation simpler #2595
Merged
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
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
46aa6d4 to
ac36e44
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2595 +/- ##
==========================================
- Coverage 89.09% 89.06% -0.04%
==========================================
Files 112 112
Lines 86932 86919 -13
Branches 86932 86919 -13
==========================================
- Hits 77456 77413 -43
- Misses 7246 7277 +31
+ Partials 2230 2229 -1
☔ View full report in Codecov by Sentry. |
|
Actually, more coming. |
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
ac36e44 to
f6cbff3
Compare
|
Okay, initial draft is building. |
domZippilli
added a commit
to domZippilli/rust-lightning
that referenced
this pull request
Sep 26, 2023
domZippilli
added a commit
to domZippilli/rust-lightning
that referenced
this pull request
Sep 26, 2023
G8XSU
reviewed
Sep 28, 2023
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
2 times, most recently
from
e944c03 to
c8d7302
Compare
jkczyz
reviewed
Sep 29, 2023
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
c8d7302 to
dc3bfd1
Compare
|
Fixed the CI failures 🤦 $ git diff-tree -U1 c8d7302ee dc3bfd1fc
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b34481db0..900353673 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -4572,3 +4572,3 @@ mod tests {
assert!(
- pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
+ pre_update_monitor.update_monitor(&replay_update, &&broadcaster, &&chanmon_cfgs[1].fee_estimator, &nodes[1].logger)
.is_err());
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index 708811afe..03dc6a5b0 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -254,3 +254,4 @@ pub fn create_onion_message<ES: Deref, NS: Deref, T: CustomOnionMessageContents>
path: OnionMessagePath, message: OnionMessageContents<T>, reply_path: Option<BlindedPath>,
-) -> Result<(PublicKey, msgs::OnionMessage), SendError> where
+) -> Result<(PublicKey, msgs::OnionMessage), SendError>
+where
ES::Target: EntropySource,
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index eeb2ddc7a..35e473c42 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -943,6 +943,6 @@ mod tests {
let mut persisted_chan_data_0 = persister_0.read_all_channel_monitors_with_updates(
- broadcaster_0, &chanmon_cfgs[0].fee_estimator).unwrap();
+ &broadcaster_0, &&chanmon_cfgs[0].fee_estimator).unwrap();
assert_eq!(persisted_chan_data_0.len(), 0);
let mut persisted_chan_data_1 = persister_1.read_all_channel_monitors_with_updates(
- broadcaster_1, &chanmon_cfgs[1].fee_estimator).unwrap();
+ &broadcaster_1, &&chanmon_cfgs[1].fee_estimator).unwrap();
assert_eq!(persisted_chan_data_1.len(), 0);
@@ -953,3 +953,3 @@ mod tests {
persisted_chan_data_0 = persister_0.read_all_channel_monitors_with_updates(
- broadcaster_0, &chanmon_cfgs[0].fee_estimator).unwrap();
+ &broadcaster_0, &&chanmon_cfgs[0].fee_estimator).unwrap();
// check that we stored only one monitor
@@ -972,3 +972,3 @@ mod tests {
persisted_chan_data_1 = persister_1.read_all_channel_monitors_with_updates(
- broadcaster_1, &chanmon_cfgs[1].fee_estimator).unwrap();
+ &broadcaster_1, &&chanmon_cfgs[1].fee_estimator).unwrap();
assert_eq!(persisted_chan_data_1.len(), 1);
@@ -1037,3 +1037,3 @@ mod tests {
// Make sure the expected number of stale updates is present.
- let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(broadcaster_0, &chanmon_cfgs[0].fee_estimator).unwrap();
+ let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(&broadcaster_0, &&chanmon_cfgs[0].fee_estimator).unwrap();
let (_, monitor) = &persisted_chan_data[0];
@@ -1145,3 +1145,3 @@ mod tests {
// open.
- let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(broadcaster_0, &chanmon_cfgs[0].fee_estimator).unwrap();
+ let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(&broadcaster_0, &&chanmon_cfgs[0].fee_estimator).unwrap();
assert_eq!(persisted_chan_data.len(), 0);
@@ -1156,3 +1156,3 @@ mod tests {
// Get the monitor and make a fake stale update at update_id=1 (lowest height of an update possible)
- let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(broadcaster_0, &chanmon_cfgs[0].fee_estimator).unwrap();
+ let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates(&broadcaster_0, &&chanmon_cfgs[0].fee_estimator).unwrap();
let (_, monitor) = &persisted_chan_data[0]; |
jkczyz
previously approved these changes
Sep 29, 2023
|
LGTM but there's a conflict? |
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
dc3bfd1 to
f1c77f7
Compare
|
Oops, fixed. Cause of #2621. |
wpaulino
previously approved these changes
Sep 29, 2023
jkczyz
previously approved these changes
Sep 29, 2023
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
f1c77f7 to
d2a10a8
Compare
|
Ugh, fixed the fuzz build failure (and a new warning): $ git diff-tree -U1 f1c77f78 d2a10a8f
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 6a2007165..2a1b9e9a7 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -157,3 +157,3 @@ impl chain::Watch<TestChannelSigner> for TestChainMonitor {
read(&mut Cursor::new(&map_entry.get().1), (&*self.keys, &*self.keys)).unwrap().1;
- deserialized_monitor.update_monitor(update, &&TestBroadcaster{}, &FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
+ deserialized_monitor.update_monitor(update, &&TestBroadcaster{}, &&FuzzEstimator { ret_val: atomic::AtomicU32::new(253) }, &self.logger).unwrap();
let mut ser = VecWriter(Vec::new());
diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index ae942a058..bd66e4cae 100644
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -425,3 +425,3 @@ impl<Signer: sign::WriteableEcdsaChannelSigner> chainmonitor::Persist<Signer> fo
- fn update_persisted_channel(&self, funding_txo: OutPoint, update: Option<&channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<Signer>, update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateStatus {
+ fn update_persisted_channel(&self, funding_txo: OutPoint, _update: Option<&channelmonitor::ChannelMonitorUpdate>, _data: &channelmonitor::ChannelMonitor<Signer>, update_id: MonitorUpdateId) -> chain::ChannelMonitorUpdateStatus {
let mut ret = chain::ChannelMonitorUpdateStatus::Completed;
$ |
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
d2a10a8 to
6e254fb
Compare
|
Oops, one more compile issue and an unused drop warning: $ git diff-tree -U1 d2a10a8f 6e254fbd
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index aa9508d23..e87d082d9 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -326,3 +326,2 @@ where C::Target: chain::Filter,
// operations going forward fail immediately.
- core::mem::drop(monitor_state);
core::mem::drop(monitor_lock);
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 721767602..26ecbb0bd 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -3783,3 +3783,3 @@ fn test_retry_custom_tlvs() {
Event::PaymentClaimable { onion_fields, .. } => {
- assert_eq!(onion_fields.unwrap().custom_tlvs(), &custom_tlvs);
+ assert_eq!(&onion_fields.unwrap().custom_tlvs()[..], &custom_tlvs[..]);
},
$ |
In 6b0d94a we switched most tests to `Default::default()` for scoring parameters passed to route-fetching. Here we do the same for the scoring parameters when passed to score-updating.
In our scoring logic we have a handful of unnecessary bounds, leading to extra diff in the bindings branch when those bounds have to be removed as well as a few cases where bindings generation simply gets confused. Here we remove a number of bounds across the scoring traits and impls, cleaning things up and simplifying bindings changes.
In a few places we require a unified scorer, which implements both `ScoreLookUp` and `ScoreUpdate`. Rather than double-bounding (which the bindings generator can't handle directly), we use a top-level `Score` trait which requires both and is implemented for all implementers of both supertraits.
The C bindings implements `Deref` for all traits, so having a blanket `Deref` implementation ends up conflicting with this.
The bindings can't easily return a reference to a `Vec`, so we instead split the implementation into vec/slice depending on the `c_bindings` flag.
Unexported macros don't need to use the `$crate` prefix.
Its honestly likely not all that useful as its not materially interoperable with other PSBT libraries. Instead, users should simply fetch the full PSBT and use the inputs from it as they see fit.
The trait itself has no purpose for bindings, as all structs are concretized anyway. Further, the bindings have specific handling for generic bounding traits like this.
The new `create_onion_message` function in `OnionMessenger` is hard to handle - it has various generic requirements indirectly via the struct, but they're not bounded by any of the method parameters. Thus, you can't simply call `OnionMessenger::create_onion_message`, as various bounds are not specified. Instead, we move it to a freestanding function so that it can be called directly without explicitly setting bounds.
The new `MonitorUpdatingPersister` has a few redundant type bounds (re-specified on functions after having been specified on the struct itself), which we remove here. Further, it requires a `Deref<FeeEstimator>` which is `Clone`able. This is generally fine in rust, but annoying in bindings, so we simply elide it in favor if a `&Deref<FeeEstimator>`.
This is kinda dumb, but the bindings get confused when referring to `Vec` absolutely in a `use` statement, and there's no reason not to load our prelude everywhere.
TheBlueMatt
force-pushed
the
2023-09-117-bindings-part1
branch
from
6e254fb to
7a583bb
Compare
jkczyz
approved these changes
Oct 3, 2023
wpaulino
approved these changes
Oct 3, 2023
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.
Mostly type generic bounds simplifications.