[0.0.122-bindings] Update 0.0.121 bindings branch to 0.0.122 #3013
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
Because log `Record`s are now being passed by ownership to `log`, the bindings get quite annoyed that there's a lifetime hanging around. We already don't use this lifetime in bindings (the `FormatArgs` is converted to a string and stored on the heap), so we can just drop the lifetime, even though it requires some macro'ing of the struct definition to do so.
There's not a lot of reason for downstream users to use the `WithContext` wrapper, it mostly exists for our own downstream crates anyway, and dealing with lifetimes in bindings isn't super practical, so simply no-export it.
The bindings cannot express lifetimes, so exposing any field which is a reference (and not `clone`-able, at least for garbage collected language bindings) is unsafe for those expecting a high-level interface. Thus, we simply no-export the `RouteHopCandidate` inner struct fields which are references (there are relevant accessors for them anyway).
...as an arg to `Router`. Passing an `EntropySource` around all the time is a bit strange as the `Router` may or may not actually use it, and the `DefaultRouter` can just as easily store it.
The top-level module is only a few hundred lines, so there's not a lot of reason to hide the `GraphSyncError` in its own module. Instead, we simply move it to the top-level `lib.rs`, which doesn't change the public API as it was previously re-exported at the top level.
As it does the same thing as a derived `Debug` does anyway.
While there's not really much harm in requiring a `Clone`able reference (they almost always are), it does make our bindings struggle a bit as they don't support multi-trait bounds (as it would require synthesizing a new C trait, which the bindings don't do automatically). Luckily, there's really no reason for it, and we can just call the `DefaultMessageRouter` directly when we want to route a message.
...as the bindings generation does not currently have the ability to map a reference to a `NodeId` inside a tuple.
Bindings can't handle references in return types, so reduce the visibility to pub(crate).
The bindings currently get confused by the implicit `Sign` type, so we temporarily remove it on the `impl` here.
Re-exports in Rust make `use` statements a little shorter, but for otherwise don't materially change a crate's API. Sadly, the C bindings generator currently can't figure out re-exports, but it also exports everything into one global namespace, so it doesn't matter much anyway.
Having struct fields with references to other structs is tough in our bindings logic, but even worse if the fields are in an enum. Its simplest to just take the clone penalty here.
These really could be handled in the bindings, but for lack of immediate users there's not a strong reason to, so instead we just disable them for now.
The scorer currently relies on an associated type for the fee parameters. This isn't supportable at all in bindings, and for lack of a better option we simply hard-code the parameter for all scorers to `ProbabilisticScoringFeeParameters`.
These are not expressible in C/most languages, and thus must be hidden.
Rather than `ChannelMonitor` only being clonable when the signer is clonable, we require all signers to be clonable and then make all `ChannelMonitor`s clonable.
`OnionMessageHandler`s are expected to regularly release a set of nodes which we need to directly connect to to deliver onion messages. In order to do so, they currently extend `EventsProvider`, returning that set as `Event::ConnectionNeeded`s. While this works fine in Rust, the `EventsProvider` interface doesn't map well in bindings due to it taking a flexible trait impl as a method argument. Instead, here, we convert `OnionMessageHandler` to include a single function which returns nodes in the form of a `node_id` and `Vec<SocketAddress>`. This is a bit simpler, if less flexible, API, and while largely equivalent, is easier to map in bindings.
As we cannot express slices without inner references in bindings wrappers.
If a user receives a payment preimage for an outbound payment, the `PaymentSent` event will block any eventual RAA `ChannelMonitorUpdate` from the same channel, assuming it comes in before the event can be processed. If this blocking kicks in, but the flow eventually completes with the RAA `ChannelMonitorUpdate` being persisted, but the `ChannelManager` is only persisted prior to the event being handled, on startup we'll have a fully up-to-date `ChannelMonitor` but a pending, blocked `ChannelMonitorUpdate`. When the `PaymentSent` event is replayed we'll end up trying to apply a redundant `ChannelMonitorUpdate` which will panic. See the test added in this commit for an implementation of this situation. In this commit we fix this issue by simply dropping blocked `ChannelMonitorUpdate`s the same as we do pending ones.
|
Pushed a backport of #3021 |
valentinewallace
approved these changes
Apr 25, 2024
|
CI always fails on the bindings branch, we test in downstream languages. |
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.
No changes since 0.0.122 is all just internal changes anyway.