Update to LDK 0.1-beta1 #137
Conversation
This used to be disabled at compile-time, but the feature was removed as it didn't net substantial performance gain, so now we disable them at runtime.
|
Need to switch to using the new inbound payment paymentid. |
TheBlueMatt
force-pushed
the
main
branch
2 times, most recently
from
2b42d4f to
fa96ed2
Compare
| @@ -226,10 +235,42 @@ impl BitcoindClient { | |||
| }); | |||
| } | |||
|
|
|||
| fn run_future_in_blocking_context<F: Future + Send + 'static>(&self, future: F) -> F::Output | |||
There was a problem hiding this comment.
Hmm, given this is the sample I don't think we'd want to advise users to employ such hacky manners? Instead, can we just switch to a pattern that avoids block_on altogether?
There was a problem hiding this comment.
I'm not sure what alternative we have? I'm not aware of another option.
There was a problem hiding this comment.
I'm not sure what alternative we have? I'm not aware of another option.
I think there are a bunch of options, but one would to create an actor that is driven by a task spawned upon intialization. Then any of these blocking trait calls could communicate with that actor via MPSC channels, for example. This should work and would keep the blocking and async contexts more or less separate, no?
(although we might still run into issues when, depending on the actual callstack, we'd block the current thread, as other tasks might still try to run on the same runtime thread, IIUC. But that seems like an orthogonal issue we can't really resolve until we have proper async support/traits throughout the codebase, AFAICT).
There was a problem hiding this comment.
I think there are a bunch of options, but one would to create an actor that is driven by a task spawned upon intialization. Then any of these blocking trait calls could communicate with that actor via MPSC channels, for example. This should work and would keep the blocking and async contexts more or less separate, no?
I don't believe the net-impact of that is any different than what we're already doing by spawning the task on a background task at the time we need it.
But that seems like an orthogonal issue we can't really resolve until we have proper async support/traits throughout the codebase, AFAICT
It looks like the only use of the sync->async inversion in the current ldk-sample codebase is in the anchor spend paths (ChangeDestinationSource+WalletSource) so it may be practical to make that whole thing dual-sync-async upstream pretty easily (with some proc macro, I assume)?
See the comment in the commit for more info on why we have to do this.
| // part of a tokio runtime (which in this case it is - the main tokio runtime). Thus, we | ||
| // have to `spawn` the `future` on the secondary runtime and then `block_on` the resulting | ||
| // `JoinHandle` on the main runtime. | ||
| tokio::task::block_in_place(move || { |
There was a problem hiding this comment.
Btw, note that this block_in_place will act on whatever runtime context is present here. This might or might not be the main runtime, it also could result being a no-op.
There was a problem hiding this comment.
Right, I think all of that is fine? Its generally expected to be the main runtime, but the point is just to tell the runtime "hey, this is gonna take some time, go ahead and make sure there's another thread to run actually-async things". Its just opportunistic, but we don't rely on it - we'll eventually return and the runtime will be happy.
No description provided.