Bindings Updates for #649 #721
Conversation
Codecov Report
@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 91.39% 91.34% -0.06%
==========================================
Files 37 37
Lines 21964 21964
==========================================
- Hits 20074 20063 -11
- Misses 1890 1901 +11
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
ca2a879 to
3c62c1b
Compare
|
Rebased and should be working fully with the latest code :) |
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
3c62c1b to
6193f37
Compare
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
6193f37 to
3ae49b8
Compare
|
The C++ header got resorted every run, but I've made it deterministic now. |
|
Pushed another commit to remove the (useless) box indirection in tuple mappings. |
| // templates_defined is walked to write the C++ header, so if we use the default hashing it get | ||
| // reordered on each genbindings run. Instead, we use SipHasher (which defaults to 0-keys) so that | ||
| // the sorting is stable across runs. It is deprecated, but the "replacement" doesn't actually | ||
| // accomplish the same goals, so we just ignore it. | ||
| #[allow(deprecated)] | ||
| type NonRandomHash = hash::BuildHasherDefault<hash::SipHasher>; |
There was a problem hiding this comment.
Could this same thing be accomplished by collecting the HashMap's keys in a Vec, sorting them, then iterating over the vector and looking up entries in the map? Then the order would both stable and somewhat sensible to someone reading the header.
There was a problem hiding this comment.
I'm not sure that alphabetical is much more sensible than random - usage order would be nice, but for that we'd have to not use a HashMap and I didn't look into if that's practical. At the end of the day the set of things in the file is huge, and they're all identical, so I'm not sure anyone is ever gonna read lightningpp.hpp instead of just inferring it from lightning.h.
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
8b3b202 to
7fa1588
Compare
Somehow we'd never had any cases of it and it requires an extra (trivial) match arm.
This pushes down the logic to check if a given Path is, in fact, a reference to a generic into the common maybe_resolve_path instead of doing it redundantly in a few different places. Net loss of LoC.
Like the previous commit pushing into maybe_resolve_path, this makes generic resolution a part of type resolution everywhere.
A few places got a None in the previous commit to avoid increasing the diff size. However, it makes sense to have GenericTypes contexts there, so we pipe them through the neccessary places.
When we have a `Trait` wrapped as an `Option<Trait>`, we called `<*const Trait>.is_null()` which resulted in rustc trying to take the most braindead option of dereferencing the whole thing and hitting a recursive dereference since we `impl Deref<Target=Trait> for Trait` for all our traits. Instead, we can be explicit and just compare the pointer directly with `std::ptr::null()` which avoids this.
Currently the check_binidngs GitHub CI job always fails when there is a new cbindgen release because the cbindgen version is in the generated header file. When the new version doesn't change the generated header except for the version number, we should ignore the difference.
When mapping an `Option<T>` where T is a mapped trait, we need to move out of the `*mut T`, however the current generation results in a `*const T` and a conversion that just takes a reference to the pointed-to object. This is because the only place this code was previously used was for slices, which *do* need a reference. Additionally, we need to know how to convert `Option` containers which do not contain an opaque type. Sadly, the easiest way to get the desired result is to add another special case in container mapping, keeping the current behavior for slices, but moving out of the pointed-to object for other types.
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
7fa1588 to
3be9a12
Compare
| LDKLogger* logger; | ||
|
|
||
| void ConnectBlock(const uint8_t (*header)[80], uint32_t height, LDKCVec_C2Tuple_usizeTransactionZZ tx_data, LDKBroadcasterInterface broadcast, LDKFeeEstimator fee_est) { |
There was a problem hiding this comment.
I think I'm missing something, but why make a new struct and not use the LDKChainMonitor? It'd be nice to get some bindings coverage for it somewhere
There was a problem hiding this comment.
Hmm, it was just the path of least resistance to adapt the existing code to the new API. I kinda like this cause its just more code, which covers more cases for the bindings, even if it covers less actual Rust code. It'd be great to have more than the two demos so that we could use different features.
A lot of our container mapping depends on the `is_owned` flag which we have for in-crate mapped objects to map references and non-references into the same container type. Transaction was mapped to two completely different types (a slice and a Vec type), which led to a number of edge cases in the bindings generation. Specifically, I spent a few days trying to map `[(A, &Transaction)]` properly and came up empty - we map slices into the same types as Vecs (and rely on the `is_owned` flag to avoid double-free) and the lack of one for `Transaction` would have required a special-case in numerous functions. Instead, we just add a flag in `Transaction` to mirror what we do for in-crate types and check it before free-ing any underlying memory. Note that, sadly, because the c_types objects aren't mapped as a part of our C++ bindings generation, you have to manually call `Transaction_free()` even in C++.
New work upstream puts tuples in slices, which is a very reasonable thing to expect, however we don't know how to generate conversions for such objects. Making it more complicated, upstream changes also include references to things inside such slices, which requires special handling to avoid creating dangling references. This adds support for converting such objects, noting that slices need to be converted first into Vecs which own their underlying objects and then need to map any reference types into references.
In some cases, things which are a Rust Reference (ie slices), we may still want to map them as a non-reference and need to put a "mut " in front of the variable name in a function decl. This worked fine by just checking for the slice case, except that we are about to add support for type aliases, which no longer match the naive case. Instead, we can just have the types module print out the C type and check if it begins with a '&' to figure out if it is a reference.
For non-generic type aliases which are meant as convinient aliases for more complex types, we need to store the aliased type (with all paths made absolute) and use that in type resolution. The most code by far is just making all the paths in a type absolute but its not too bad either.
In general we should stop enforcing that all lifetimes are static - we may take references from C and its up to reviewing the diff on the bindings changes and the user(s) to ensure lifetimes are valid. Also asserts a success criteria that was missed before.
This is a limitations in the bindings crate, but not one that's going to be fixed right now.
Because the C++ wrappers require being able to memset(0) the C structs to skip free(), we'd previously mapped tuples with two pointer indirections. However, because all other types already support memset(0)'ing to disable free() logic, we can skip the pointer indirections and the behavior is still correct.
TheBlueMatt
force-pushed
the
2020-09-649-bindings
branch
from
3be9a12 to
5988789
Compare
There was a problem hiding this comment.
This is looking really good to me! I pretty much reviewed all the generated code besides lightningpp.hpp. Also reviewed the demos and they LGTM.
(Btw, does anyone know if there's a way for git diff to ignore moved code? that would be super helpful here.)
I have a few questions but most of the changes seemed pretty straightforward. I'll take a look at the c-bindings-gen and the C++ headers next (though mostly what I care about is the generated code and demos).
| @@ -374,6 +376,17 @@ impl ChannelInfo { | |||
| ret | |||
| } | |||
| } | |||
| /// Protocol features of a channel communicated during its announcement | |||
| #[no_mangle] | |||
| pub extern "C" fn ChannelInfo_get_features(this_ptr: &ChannelInfo) -> crate::ln::features::ChannelFeatures { | |||
There was a problem hiding this comment.
OOC, I'm having trouble figuring out why all the getters/setters for ChannelFeatures and NodeFeatures are only being added now (like, is there a technical reason)?
There was a problem hiding this comment.
The Features stuff all uses type X = Y aliasing because they all use a common Features<Context> base, so I assume its because we started actually processing type aliasing now.
| let mut local_htlc = if htlc.inner.is_null() { None } else { Some((* { unsafe { &*htlc.inner } }).clone()) }; | ||
| let mut ret = unsafe { &mut *(this_arg as *mut nativeInMemoryChannelKeys) }.sign_justice_transaction(&justice_tx.into_bitcoin(), input, amount, &::bitcoin::secp256k1::key::SecretKey::from_slice(&unsafe { *per_commitment_key}[..]).unwrap(), &local_htlc, &bitcoin::secp256k1::Secp256k1::new()); | ||
| let mut local_ret = match ret { Ok(mut o) => crate::c_types::CResultTempl::ok( { crate::c_types::Signature::from_rust(&o) }), Err(mut e) => crate::c_types::CResultTempl::err( { 0u8 /*e*/ }) }; | ||
| local_ret | ||
| } | ||
| #[must_use] | ||
| extern "C" fn InMemoryChannelKeys_ChannelKeys_sign_counterparty_htlc_transaction(this_arg: *const c_void, htlc_tx: crate::c_types::Transaction, mut input: usize, mut amount: u64, per_commitment_point: crate::c_types::PublicKey, htlc: &crate::ln::chan_utils::HTLCOutputInCommitment) -> crate::c_types::derived::CResult_SignatureNoneZ { | ||
| extern "C" fn InMemoryChannelKeys_ChannelKeys_sign_counterparty_htlc_transaction(this_arg: *const c_void, mut htlc_tx: crate::c_types::Transaction, mut input: usize, mut amount: u64, mut per_commitment_point: crate::c_types::PublicKey, htlc: &crate::ln::chan_utils::HTLCOutputInCommitment) -> crate::c_types::derived::CResult_SignatureNoneZ { |
There was a problem hiding this comment.
Hmm I can't tell why these muts are being added?
There was a problem hiding this comment.
We generally should be setting "mut" everywhere since it gives us more flexibility in the body and doesn't change the API, I think.
| /// [`get_outputs_to_watch`]: #method.get_outputs_to_watch | ||
| #[must_use] | ||
| #[no_mangle] | ||
| pub extern "C" fn ChannelMonitor_block_connected(this_arg: &mut ChannelMonitor, header: *const [u8; 80], mut txdata: crate::c_types::derived::CVec_C2Tuple_usizeTransactionZZ, mut height: u32, mut broadcaster: crate::chain::chaininterface::BroadcasterInterface, mut fee_estimator: crate::chain::chaininterface::FeeEstimator, mut logger: crate::util::logger::Logger) -> crate::c_types::derived::CVec_C2Tuple_TxidCVec_TxOutZZZ { |
There was a problem hiding this comment.
Just to check my understanding: is the reason this function is only being exposed now, because previously we didn't support returning a vec/slice of tuples?
|
Arguably could be out of scope for this particular PR, but I noticed that Also I forgot to point out that ironically the |
|
re: The new issue - does it work if you leave the first lto build alone and only remove the |
This isn't (quite) complete, but the only thing missing is type aliases, which should be pretty easy to add. Likely doesn't make sense to go in before 649, but opening it now in case anyone is curious.