WIP: Initial LdkLite architecture and project layout #1
Conversation
tnull
force-pushed
the
2022-08-start-of-ldk-lite
branch
from
6a54722 to
31a43f6
Compare
| let _outbound_payments = Arc::new(Mutex::new(HashMap::new())); | ||
|
|
||
| // Step 14: Handle LDK Events | ||
| let event_queue = mpsc::sync_channel(CHANNEL_BUF_SIZE); |
There was a problem hiding this comment.
Sadly we can't queue events. They must be fully handled in-line (and any resulting data persisted to disk) before the handle-event method returns. This ensures we never lose events, as we do not remove them from the channel manager until that point.
There was a problem hiding this comment.
Ah, wait, but that requirement is for LDK events, right? The event_queue here is meant for LdkLiteEvents.
The idea would be: LDK events get handled by event::LdkLiteEventHandler, which processes them and emits more user-friendly LdkLiteEvents (notifications really, possibly wrapping the original events) to said event queue. Yes, these events should probably not get lost either, but it's maybe not as critical if we already handled technicalities for, e.g, FundingGenerationReady or SpendableOutputs in the event handler?
There was a problem hiding this comment.
Ah okay, I see your point. I assume eg payment history is persisted directly in LdkLite before we hand events to users (this would imply LdkLite is required to track history)? Are there no cases where users could lose data from missing an event?
There was a problem hiding this comment.
Yes, I payment history will be persisted directly by the event handler I assume.
But, I'm honestly not sure how we could ensure that no data is lost ever when the user doesn't process the events given. If event queue gets dropped, the user will probably not notified again about the queued events. However, I'm not sure if "we hand you individual events and it's on you if you don't act on them before dropping" is that different from "we hand you a list/queue of events, and it's on you if you don't act on all of them before dropping" at that point?
There was a problem hiding this comment.
Well, the first is implementable, the second is impossible if the program crashes (eg is running on mobile and the user goes back to the home screen).
There was a problem hiding this comment.
Hm, but I'm not sure exposing this kind of interface is in line with the usability we're trying to achieve with LdkLite. I agree we should handle all security-critical things before we even notify the user, but forcing him to handle events inline is sometimes just not practical? Also, it would likely force us to block the event handler and thereby the entire BP thread? Also not sure the given example matches exactly, because IIUC at least under iOS the app would be suspended and its state saved until it is brought to the foreground again? (Which opens a whole other topic on whether we would cleanly recover and continue operation from different states, but that's 'future work').
There was a problem hiding this comment.
Well the other way to go about it is ensure the user never does their own event responses. Its kinda an annoying API, but you can remove all details from events and just generate a singular "you should poll various LDK structs now, cause things have changed" event flag. This forces users into being able to handle arbitrary changes which they may have missed on restart.
Indeed, we want the API to be usable, but we absolutely cannot trade off safety for that, and an event stream abstraction which is impossible to implement correctly is not a fix.
Also, it would likely force us to block the event handler and thereby the entire BP thread?
It wouldn't necessarily have to, LdkLite could persist the event itself into its own DB, return control to LDK, then let the user handle the event as they wish.
There was a problem hiding this comment.
regarding event reliability, as long as these events represent non-critical notifications type of things, it would have been fine and maybe we can lose those events.
But the problem is do we expect clients to poll through this ? I don't think that is ideal.
Hence handling individual events using event handler is definitely better UX.
Also, it might be helpful to have some distinction b/w core/critical events and some notification/alarm kind of things.
tnull
force-pushed
the
2022-08-start-of-ldk-lite
branch
from
1e67afc to
1512e9d
Compare
tnull
force-pushed
the
2022-08-start-of-ldk-lite
branch
from
1512e9d to
5b9e4c4
Compare
src/access.rs
Outdated
| let blockchain = Mutex::new(blockchain); | ||
| let wallet = Mutex::new(wallet); |
There was a problem hiding this comment.
please separate this whole chain-access logic implementation into another PR.
keep the interfaces/empty/dummy functions in this PR.
We should review it carefully separately.
PR Splits:
- This PR (with mostly interfaces/empty/dummy functions, license, rustfmt, base setup, logger, ldk-lite builder )
- event handling
- LDK-lite Error/Exception standards
- Chain Access Impl
- create_funding_transaction
- setup runtime
I focused on reviewing parts that belonged to this PR.
There was a problem hiding this comment.
Hm, to be honest I'd rather not because in this initial step the individual components are pretty interdependent and due to change. I imagine it to be more annoying to constantly having to rebase the individual PRs on each other.
So, if you not really insist, I'd like to keep it in this one PR for now and handle forthcoming changes in individual PRs.
|
|
||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
No, currently std is used all over?
tnull
force-pushed
the
2022-08-start-of-ldk-lite
branch
from
f025939 to
5fcb938
Compare
tnull
force-pushed
the
2022-08-start-of-ldk-lite
branch
from
0925bb3 to
d6f09ad
Compare
|
As requested by @G8XSU, I'm closing this in favor of individual PRs. |
This PR is an initial WIP implementation of the simplified LDK Lite API and also provides the general project layout. I referenced / adapted code from the sample node where possible and also started to add documentation.
I'm really looking forward to any feedback, but in particular comments on the taken design choices would be highly appreciated, since they probably should be adapted ASAP, if suboptimal.
As in the current state it's hard to draw clear lines, I intend to add basic initial implementations of the following in this PR:
However, as this will make it probably pretty unwieldy any further features/modification will be done in follow-ups (also see TODOs in the code and Issues).