Uh oh!
There was an error while loading. Please reload this page.
Expose ChannelManager/Monitor read methods in bindings - #768
Merged
TheBlueMatt merged 13 commits intoFeb 3, 2021
Merged
Conversation
Codecov Report
@@ Coverage Diff @@## main #768 +/- ##
==========================================
- Coverage 90.79% 90.78% -0.01%
==========================================
Files 38 38 Lines 23168 23168 ==========================================
- Hits 21036 21034 -2 - Misses 2132 2134 +2
Continue to review full report at Codecov.
|
TheBlueMattforce-pushed
the
2020-12-chanman-bindings-deser
branch
from
January 1, 2021 01:46
748542d to
c326bcbComparejkczyz
reviewed
Feb 2, 2021
jkczyz
left a comment
Contributor
There was a problem hiding this comment.
I'm getting some compilation warnings and errors likely related to c++11 not being specified somewhere:
++ which valgrind
+ '[' -x '' ']'
+ echo 'WARNING: Please install valgrind for more testing'
WARNING: Please install valgrind for more testing
+ CLANGOPTS='-Wall -Wno-nullability-completeness -pthread'
+ clang++ -Wall -Wno-nullability-completeness -pthread demo.cpp target/debug/libldk.a -ldl
In file included from demo.cpp:5:
./include/lightningpp.hpp:7:24: warning: deleted function definitions are a C++11 extension [-Wc++11-extensions]
Event(const Event&) = delete;
^
./include/lightningpp.hpp:8:13: warning: rvalue references are a C++11 extension [-Wc++11-extensions]
Event(Event&& o) : self(o.self) { memset(&o, 0, sizeof(Event)); }
^
./include/lightningpp.hpp:9:16: warning: rvalue references are a C++11 extension [-Wc++11-extensions]
Event(LDKEvent&& m_self) : self(m_self) { memset(&m_self, 0, sizeof(LDKEvent)); }
^
./include/lightningpp.hpp:10:22: warning: reference qualifiers on functions are a C++11 extension [-Wc++11-extensions]
operator LDKEvent() && { LDKEvent res = self; memset(&self, 0, sizeof(LDKEvent)); return res; }
^
./include/lightningpp.hpp:12:24: warning: rvalue references are a C++11 extension [-Wc++11-extensions]
Event& operator=(Event&& o) { Event_free(self); self = o.self; memset(&o, 0, sizeof(Event)); return *this; }
^
...
demo.cpp:119:23: error: default initialization of an object of const type 'const LDKFeeEstimator' without a user-provided default constructor
const LDKFeeEstimator fee_est {
^
= {}
demo.cpp:119:30: error: expected ';' after top level declarator
const LDKFeeEstimator fee_est {
^
;
demo.cpp:134:58: error: a space is required between consecutive right angle brackets (use '> >')
std::vector<std::pair<LDK::OutPoint, LDK::ChannelMonitor>> mons;
^~
> >
demo.cpp:139:8: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
for (auto& mon : mons) {
^
demo.cpp:139:18: warning: range-based for loop is a C++11 extension [-Wc++11-extensions]
for (auto& mon : mons) {
^
demo.cpp:147:22: error: no matching constructor for initialization of 'LDK::ChannelMonitor'
LDK::ChannelMonitor mon(std::move(monitor_arg));
^ ~~~~~~~~~~~~~~~~~~~~~~
./include/lightningpp.hpp:304:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitor' to 'const LDK::ChannelMonitor' for 1st argument
ChannelMonitor(const ChannelMonitor&) = delete;
^
./include/lightningpp.hpp:305:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitor' to 'LDK::ChannelMonitor' for 1st argument
ChannelMonitor(ChannelMonitor&& o) : self(o.self) { memset(&o, 0, sizeof(ChannelMonitor)); }
^
./include/lightningpp.hpp:306:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitor' to 'LDKChannelMonitor &&' for 1st argument
ChannelMonitor(LDKChannelMonitor&& m_self) : self(m_self) { memset(&m_self, 0, sizeof(LDKChannelMonitor)); }
^
demo.cpp:148:16: error: no matching constructor for initialization of 'LDK::OutPoint'
LDK::OutPoint funding_txo(std::move(funding_txo_arg));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/lightningpp.hpp:334:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'const LDK::OutPoint' for 1st argument
OutPoint(const OutPoint&) = delete;
^
./include/lightningpp.hpp:335:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'LDK::OutPoint' for 1st argument
OutPoint(OutPoint&& o) : self(o.self) { memset(&o, 0, sizeof(OutPoint)); }
^
./include/lightningpp.hpp:336:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'LDKOutPoint &&' for 1st argument
OutPoint(LDKOutPoint&& m_self) : self(m_self) { memset(&m_self, 0, sizeof(LDKOutPoint)); }
^
demo.cpp:159:28: error: no matching constructor for initialization of 'LDK::ChannelMonitorUpdate'
LDK::ChannelMonitorUpdate update(std::move(monitor_arg));
^ ~~~~~~~~~~~~~~~~~~~~~~
./include/lightningpp.hpp:230:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitorUpdate' to 'const LDK::ChannelMonitorUpdate' for 1st argument
ChannelMonitorUpdate(const ChannelMonitorUpdate&) = delete;
^
./include/lightningpp.hpp:231:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitorUpdate' to 'LDK::ChannelMonitorUpdate' for 1st argument
ChannelMonitorUpdate(ChannelMonitorUpdate&& o) : self(o.self) { memset(&o, 0, sizeof(ChannelMonitorUpdate)); }
^
./include/lightningpp.hpp:232:2: note: candidate constructor not viable: no known conversion from 'LDKChannelMonitorUpdate' to 'LDKChannelMonitorUpdate &&' for 1st argument
ChannelMonitorUpdate(LDKChannelMonitorUpdate&& m_self) : self(m_self) { memset(&m_self, 0, sizeof(LDKChannelMonitorUpdate)); }
^
demo.cpp:160:16: error: no matching constructor for initialization of 'LDK::OutPoint'
LDK::OutPoint funding_txo(std::move(funding_txo_arg));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/lightningpp.hpp:334:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'const LDK::OutPoint' for 1st argument
OutPoint(const OutPoint&) = delete;
^
./include/lightningpp.hpp:335:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'LDK::OutPoint' for 1st argument
OutPoint(OutPoint&& o) : self(o.self) { memset(&o, 0, sizeof(OutPoint)); }
^
./include/lightningpp.hpp:336:2: note: candidate constructor not viable: no known conversion from 'LDKOutPoint' to 'LDKOutPoint &&' for 1st argument
OutPoint(LDKOutPoint&& m_self) : self(m_self) { memset(&m_self, 0, sizeof(LDKOutPoint)); }
^
demo.cpp:166:7: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
for (auto& mon : arg->mons) {
^
demo.cpp:166:17: warning: range-based for loop is a C++11 extension [-Wc++11-extensions]
for (auto& mon : arg->mons) {
^
demo.cpp:187:32: error: expected '(' for function-style cast or type construction
return LDKCVec_MonitorEventZ {
~~~~~~~~~~~~~~~~~~~~~ ^
demo.cpp:239:31: error: expected '(' for function-style cast or type construction
sock1 = LDKSocketDescriptor {
~~~~~~~~~~~~~~~~~~~ ^
demo.cpp:249:31: error: expected '(' for function-style cast or type construction
sock2 = LDKSocketDescriptor {
~~~~~~~~~~~~~~~~~~~ ^
demo.cpp:259:8: error: no matching constructor for initialization of 'std::thread'
t1 = std::thread(&sock_read_data_thread, pipefds_2_to_1[0], &sock1, &net1);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:408:9: note: candidate constructor template not viable: requires single argument '__f', but 4 arguments were provided
thread::thread(_Fp __f)
^
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:289:5: note: candidate constructor not viable: requires 1 argument, but 4 were provided
thread(const thread&);
^
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:296:5: note: candidate constructor not viable: requires 0 arguments, but 4 were provided
thread() _NOEXCEPT : __t_(_LIBCPP_NULL_THREAD) {}
^
demo.cpp:260:8: error: no matching constructor for initialization of 'std::thread'
t2 = std::thread(&sock_read_data_thread, pipefds_1_to_2[0], &sock2, &net2);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:408:9: note: candidate constructor template not viable: requires single argument '__f', but 4 arguments were provided
thread::thread(_Fp __f)
^
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:289:5: note: candidate constructor not viable: requires 1 argument, but 4 were provided
thread(const thread&);
^
/Library/Developer/CommandLineTools/usr/include/c++/v1/thread:296:5: note: candidate constructor not viable: requires 0 arguments, but 4 were provided
thread() _NOEXCEPT : __t_(_LIBCPP_NULL_THREAD) {}
^
demo.cpp:268:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
auto writelen = write(pipefds_1_to_2[1], con_res->contents.result->data, con_res->contents.result->datalen);
^
demo.cpp:299:35: error: expected ';' at end of declaration
LDKBroadcasterInterface broadcast {
^
;
demo.cpp:306:19: error: expected ';' at end of declaration
LDKLogger logger1 {
^
;
demo.cpp:314:15: error: expected ';' at end of declaration
LDKWatch mon1 {
^
;
demo.cpp:325:19: error: expected ';' at end of declaration
LDKLogger logger2 {
^
;
demo.cpp:333:15: error: expected ';' at end of declaration
LDKWatch mon2 {
^
;
demo.cpp:344:38: error: expected '(' for function-style cast or type construction
LDK::CVec_u8Z cm1_ser = LDKCVec_u8Z {}; // ChannelManager 1 serialization at the end of the ser-des scope
~~~~~~~~~~~ ^
demo.cpp:345:38: error: expected '(' for function-style cast or type construction
LDK::CVec_u8Z cm2_ser = LDKCVec_u8Z {}; // ChannelManager 2 serialization at the end of the ser-des scope
~~~~~~~~~~~ ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
900 warnings and 20 errors generated.
| } | ||
| pub extern "C" fn ChannelMonitorUpdate_read(ser: crate::c_types::u8slice) -> crate::c_types::derived::CResult_ChannelMonitorUpdateDecodeErrorZ { | ||
| let res = crate::c_types::deserialize_obj(ser); | ||
| let mut local_res = match res { Ok(mut o) => crate::c_types::CResultTempl::ok( { crate::chain::channelmonitor::ChannelMonitorUpdate { inner: Box::into_raw(Box::new(o)), is_owned: true } }), Err(mut e) => crate::c_types::CResultTempl::err( { crate::ln::msgs::DecodeError { inner: Box::into_raw(Box::new(e)), is_owned: true } }) }; |
Contributor
There was a problem hiding this comment.
Was having no line breaks here intentional?
CollaboratorAuthor
There was a problem hiding this comment.
There's a number of similar cases across the bindings, especially for template types (vec/result/tuples) - specifically we don't track at what level of indentation we are at that deep into type conversion, so we either have bogus indentation or no indentation. For now its none, but its something we should fix eventually.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| echo "WARNING: Please install valgrind for more testing" | ||
| fi | ||
| CLANGOPTS="-Wall -Wno-nullability-completeness -pthread" |
Contributor
There was a problem hiding this comment.
I'm seeing a ton of warnings looking to originate from this commit:
++ rustc --version --verbose
++ grep host:
+ HOST_PLATFORM='host: x86_64-apple-darwin'
+ '[' 'host: x86_64-apple-darwin' = 'host: x86_64-apple-darwin' ']'
+ sed -i '' 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h
+ gcc -Wall -g -pthread demo.c target/debug/libldk.a -ldl
In file included from demo.c:2:
./include/lightning.h:204:12: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
uint8_t *data;
^
./include/lightning.h:204:12: note: insert '_Nullable' if the pointer may be null
uint8_t *data;
^
_Nullable ./include/lightning.h:204:12: note: insert '_Nonnull' if the pointer should never be null
uint8_t *data;
^
_Nonnull CollaboratorAuthor
There was a problem hiding this comment.
Heh, that's an OSX-ism, because gcc is just a symlink to clang...I'll add the warning disable on all the compile calls.
TheBlueMattforce-pushed
the
2020-12-chanman-bindings-deser
branch
from
February 3, 2021 00:55
c326bcb to
d08bdf4CompareTheBlueMatt
commented
Feb 3, 2021
CollaboratorAuthor
Addressed review comments, rebased to fix conflicts but no changes outside of bindings generation code. |
We can fail to resolve a part of a tuple, resulting in a panic in write_template_constructor even if we're calling `understood_c_type` with the intent of figuring out whether we can print a type at all. Instead, we should pipe errors back and let `understood_c_type` return false as a result.
Previously, manual `*_read` implementations were only defined for types with inner fields, which were set to NULL to indicate read errors. This prevents exposing `*_read` for several other types, including tuples (which are needed for `ChannelManager`/ `ChannelMonitors`) and enums (which includes `Event`s, though users likely never need to call that directly). Further, this means we don't expose the actual error enum (which is likely no big deal, but is still nice). Here, we instead create the `Result<Object, DecodeError>` type and then pass it through the normal type conversion functions, giving us access to any types which we can convert normally.
jkczyz
approved these changes
Feb 3, 2021
This expands the manual implementation logic for `*_write` and `*_read` methods to most types, converting the `*_write` path to the common type-conversion logic to ensure it works. Note that `*_write_void` is still only implemented for has-inner types, as its unclear what the `void*` would point to for others.
It just stubs out to `write_rust_path` in this case anyway, which handles leading-colons just fine, so no need to panic on them.
This is most of the code to expose `ChannelManager`/`ChannelMonitor` deserialization in our C bindings, using the new infrastructure to map types in `maybe_convert_trait_impl` and passing generics in from the callsites. We also call `maybe_convert_trait_impl` for tuple types, as the `ChannelManager`/`ChannelMonitor` deserialization returns a `(BlockHash, T)` to indicate the block hash at which users need to start resyncing the chain. The final step to expose them is in the next commit.
This (finally) exposes `ChannelManager`/`ChannelMonitor` _write methods, which were (needlessly) excluded as the structs themselves have generic parameters. Sadly, we also now need to parse `(C-not exported)` doc comments on impl blocks as we otherwise try to expose _write methods for `&Vec<RouteHop>`, which doesn't work (and isn't particularly interesting for users anyway). We add such doc comments there.
This adds a new annotation for objects we take by reference in the C header indicating the pointers must not be null. We have to disable some warning clang now dumps that we haven't annotated all pointers, as cbindgen is not yet able to add a nullable annotation.
Previously, references and pointers ended up identical in C, so there was little reason to differentiate. With the addition of nullability annotations, there is a (very slight) reason to prefer references, so use them in a few places where its a trivial change.
This adds a move-assignment operator (`A& operator=(A&& o)`) to our C++ wrapper classes as well as requiring an rvalue for the move auto-convert operator (`operator CStruct()() &&`). The second makes the C++ wrapper classes much easier to work with by requiring an explicit `std::move` when the bindings will automatically move a C++-wrapper object into a C object.
This demonstrates (and tests) the newly-exposed `ChannelManager` de/serialization functions. Best revewied with -b --color-moved.
There were two issues on OSX - we need to give gcc the clang warnings flags because `gcc` *is* clang on OSX and we missed an `-std=c++11` on one of the clang++ calls, causing compile failures.
TheBlueMattforce-pushed
the
2020-12-chanman-bindings-deser
branch
from
February 3, 2021 15:12
d08bdf4 to
5d045deCompareCollaboratorAuthor
Rebased with no changes ( |
TheBlueMatt
commented
Feb 3, 2021
CollaboratorAuthor
Its just bindings, worth merging without more than one ack. |
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Based on #767 (and #761), this finally exposes ChannelManager read methods that were missed in the early bindings work. There's also a few nice cleanups of the C++ wrappers, though they are code-wise trivial. As with #767, between each bindings builder update the changes to the bindings are committed, though those commits may be squashed before merge.