Pre-C-Bindings Cleanups #3 #676
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
2020-08-c-bindings-cleanups-3
branch
from
2331bc5 to
6cc8fc6
Compare
Codecov Report
@@ Coverage Diff @@
## master #676 +/- ##
==========================================
+ Coverage 91.35% 91.38% +0.03%
==========================================
Files 35 35
Lines 21723 21736 +13
==========================================
+ Hits 19844 19864 +20
+ Misses 1879 1872 -7
Continue to review full report at Codecov.
|
There are a few cases where the upcoming C bindings don't know how to handle something which depends on something defined later in the file. Instead of adding another pass to the C bindings generator, it is much simpler to just reorder structs.
This makes it a little easier to write C bindings generation as we only have to handle one case instead of both.
This avoids one case the bindings generation hasn't bothered to handle by simply importing types that are referred to.
Instead of using the explicit type which is being returned, refer to them as Self::AssociatedType, to make clear to the bindings what type of thing is being returned.
Because the C bindings maps objects into new structs which contain only a pointer to the underlying (immovable) Rust type, it cannot create a list of Rust types which are contiguous in memory. Thus, in order to allow C clients to call certain Rust functions, we have to use &[&Type] not &[Type]. This commit fixes this issue for the get_route function.
Lightning OutPoints only have 16 bits to express the output index instead of Bitcoin's 32 bits, implying that some outputs are possibly not expressible as lightning OutPoints. However, such OutPoints can never be hit within the lightning protocol, and must be on-chain spam sent by a third party wishing to donate us money. Still, in order to do so, the third party would need to fill nearly an entire block with garbage, so this case should be relatively safe. A new comment in channelmonitor explains the reasoning a bit further.
TheBlueMatt
force-pushed
the
2020-08-c-bindings-cleanups-3
branch
from
6cc8fc6 to
44b8197
Compare
jkczyz
approved these changes
Aug 25, 2020
| use ln::msgs; | ||
| use ln::msgs::UnsignedChannelAnnouncement; |
There was a problem hiding this comment.
Could you open an issue to fix the underlying problem in the bindings generation? Sometimes using the module prefix is preferable.
valentinewallace
approved these changes
Aug 26, 2020
Its somewhat awkward that ChannelManagerReadArgs requires a mutable reference to a HashMap of ChannelMonitors, forcing the callsite to define a scope for the HashMap which they almost certainly won't use after deserializing the ChannelManager. Worse, to map the current version to C bindings, we'd need to also create a HashMap binding, which is overkill for just this one use. Instead, we just give the ReadArgs struct ownership of the HashMap and add a constructor which fills the HashMap for you.
The C bindings automatically create a _new() function for structs which contain only pub fields which we know how to map. This conflicts with the actual TxCreationKeys::new() function, so we simply rename it to capture its nature as a derivation function.
In order to calculate a route, it is likely that users need to take a read()-lock on NetGraphMsgHandler::network_graph. This is not possible naively from C bindings, as Rust's native RwLock is not exposed. Thus, we provide a simple wrapper around the RwLockReadGuard and expose simple accessor methods.
TheBlueMatt
force-pushed
the
2020-08-c-bindings-cleanups-3
branch
from
44b8197 to
d224c1d
Compare
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.
Hopefully this is the last of them given we're getting close on the C bindings PR. See individual commits for details.