Update to LDK 0.1-beta1 #137
Conversation
TheBlueMatt
force-pushed
the
main
branch
4 times, most recently
from
76cc0a2 to
04aaa24
Compare
|
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)?
There was a problem hiding this comment.
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)?
I'm not quite sure this is true, due to ChangeDestinationSource being used in OutputSweeper's spending method which in turn is triggered by Confirm/Listen. So if we make the traits async-optional, we'd also need to propagate it up the chain syncing logic, including all the traits and connected methods, no? But let's probably continue this discussion on lightningdevkit/rust-lightning#3540.
TheBlueMatt
force-pushed
the
main
branch
2 times, most recently
from
f7eb75b to
21f12a5
Compare
| // 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.
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.
See the comment in the commit for more info on why we have to do this.
Actually doesn't make sense since we track payments since invoice creation, so need to index by payment hash. Updated to 0.1 release, this should be good to go! |
Hmm, well, but it might be a good point in time to actually change that as it's kind of weird to have a behavioral difference between BOLT11 and BOLT12 here? FWIW, we have the same issue in LDK Node, but there we'll only be able to remedy it via introducing a new metadata store holding on to offers and invoices (cf. lightningdevkit/ldk-node#425). |
|
Good idea, added HRN support. |
Yea, we probably should eventually change the inbound payment store here to associate OfferIds with the inbound payment, but that's a bigger overhaul and we can do it another time, I think. |
No description provided.