-
Notifications
You must be signed in to change notification settings - Fork 419
lightning-liquidity
: Add serialization logic, persist service state
#4059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
124211d
to
26f3ce3
Compare
a98dff6
to
d630c4e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4059 +/- ##
==========================================
- Coverage 88.75% 88.51% -0.25%
==========================================
Files 177 179 +2
Lines 133318 134190 +872
Branches 133318 134190 +872
==========================================
+ Hits 118330 118778 +448
- Misses 12293 12661 +368
- Partials 2695 2751 +56
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d630c4e
to
70118e7
Compare
70118e7
to
dd43edc
Compare
this all LGTM. I have a small concern: maybe I’m being a little paranoid, but read_lsps2_service_peer_states and read_lsps5_service_peer_states pull every entry from the KVStore into memory with no limit. That could lead to unbounded state, exhausting memory and crash. Maybe we can add a limit on how many entries we load into memory to protect against this dos? not sure how realistic this is though. maybe an attacker could have access to or share the same storage with the victim, and they could dump effectively infinite data onto disk. in this scenario, probably the victim would be vulnerable to other attacks too, but still.. |
Reading state from disk (currently) happens on startup only, so crashing wouldn't be the worst thing, we would simply fail to start up properly. Some even argue that we need to panic if we hit any IO errors at this point to escalate to an operator. We could add some safeguard/upper bound, but I'm honestly not sure what it would protect against.
Heh, well, if we assume the attacker has write access to our |
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
dd43edc
to
f73146b
Compare
pub token: Option<String>, | ||
} | ||
|
||
impl_writeable_tlv_based!(LSPS2GetInfoRequest, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to have two ways to serialize all these types? Wouldn't it make more sense to just use the serde
serialization we already have and wrap that so that it can't all be misused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think I'd be in favor of using TLV serialization for our own persistence.
Note that the compat guarantees of LSPS0/the JSON/serde format might not exactly match what we require in LDK, and our Rust representation might also diverge from the pure JSON impl. On top of that JSON is of course much less efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is there some easy way to avoid exposing that in the public API, then? Maybe a wrapper struct oe extension trait for serialization somehow? Seems like kinda a footgun for users, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is there some easy way to avoid exposing that in the public API, then? Maybe a wrapper struct oe extension trait for serialization somehow? Seems like kinda a footgun for users, I think?
Not quite sure I understand the footgun? You mean because these types then have Writeable
as well as Serialize
implementations on them and users might wrongly pick Writeable
when they use the types independently from/outside of lightning-liquidity
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, for example. Someone who uses serde presumably has some wrapper that serde-writes Writeable
structs and suddenly their code could read/compile totally fine and be reading the wrong kind of thing. If they have some less-used codepaths (eg writing Events before they process them and then removing them again after) they might not find immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, for example. Someone who uses serde presumably has some wrapper that serde-writes
Writeable
structs and suddenly their code could read/compile totally fine and be reading the wrong kind of thing.
I'm confused - Writeable
is an LDK concept not connected to serde
? Do you mean Serialize
? But that also has completely separate API? So how would they trip up? You mean they'd confuse Writeable
and Serialize
?
) -> Pin<Box<dyn Future<Output = Result<(), lightning::io::Error>> + Send>> { | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
let mut futures = Vec::new(); | ||
for (counterparty_node_id, peer_state) in outer_state_lock.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? Why would we ever want to do a single huge persist pass and write every peer's state at once? Shouldn't we be doing this iteratively? Same applies in the LSPS2 service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only persisting what's needed/changed will be part of the next PR as it ties into how we wake the BP to drive persistence (cf. "Avoid re-persisting peer states if no changes happened (needs_persist
flag everywhere)" bullet over at #4058 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why we're adding this method then? If its going to be removed in the next PR in the series we should just not add it in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not gonna be removed, but extended: PeerState
(here as well as in LSPS2) will gain a dirty/needs_persist
flag and we'd simply skip persisting any entries that haven't been changed since the last persistence round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a weird design if we need to persist something immediately while its being operated on - we have the node in question why walk a whole peer list? Can you put up the followup code so we can see how its going to be used? Given this PR is mostly boilerplate I honestly wouldn't mind it being a bit bigger, as long as the code isn't too crazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a weird design if we need to persist something immediately while its being operated on - we have the node in question why walk a whole peer list?
Yes, this is why persist_peer_state
is a separate method - for inline persistence where we already hold the lock to the peer state we'd just call that. For the general/eventual persistence the background processor task calls LiquidityManager::persist
which calls through to the respective LSPS*ServiceHandler::persist
methods which then only persists the entries marked dirty since the last persistence round.
Can you put up the followup code so we can see how its going to be used? Given this PR is mostly boilerplate I honestly wouldn't mind it being a bit bigger, as long as the code isn't too crazy.
Sure will do as soon as it's ready an in a coherent state, although I had hoped to land this PR this week.
f73146b
to
2971982
Compare
Rebased to address minor conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded to the outstanding comments, not quite sure I fully get all the rationale here.
d647793
to
ec94cb3
Compare
0u8 => { | ||
// LSPS0ClientEvents are not persisted. | ||
continue; | ||
}, | ||
1u8 => { | ||
// LSPS1ClientEvents are not persisted. | ||
continue; | ||
}, | ||
2u8 => { | ||
// LSPS1ServiceEvents are not persisted. | ||
continue; | ||
}, | ||
3u8 => { | ||
// LSPS2ClientEvents are not persisted. | ||
continue; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit f Write length of actually persisted events
is on the wrong base commit, it should be on Implement serialization for event queue types
.. this is likely only temporary necessary as we can drop our own `dummy_waker` implementation once we bump MSRV.
We add simple `persist` call to `LSPS2ServiceHandler` that sequentially persist all the peer states under a key that encodes their node id.
We add simple `persist` call to `LSPS5ServiceHandler` that sequentially persist all the peer states under a key that encodes their node id.
We add simple `persist` call to `EventQueue` that persists it under a `event_queue` key.
.. as we currently prune the the pending request state on peer disconnection anyways, so even if peers would reconnect the service can't use the events anymore anyways.
We read any previously-persisted state upon construction of `LiquidityManager`.
We read any previously-persisted state upon construction of `LiquidityManager`.
We read any previously-persisted state upon construction of `LiquidityManager`.
We let the background processor task regularly call `LiquidityManger::persist`. We also change the semantics of the `Future` for waking the background processor to also be used when we need repersisting (which we'll do in the next commit).
.. we only persist the event queue if necessary and wake the BP to do so when something changes.
.. we only persist the service handler if necessary.
.. to allow access in a non-async context
.. and wrap them accordingly for the `LSPS2ServiceHandlerSync` variant.
.. we only persist the service handler if necessary.
We add a simple test that runs the LSPS2 flow, persists, and ensures we recover the service state after reinitializing from our `KVStore`.
We add a simple test that runs the LSPS5 flow, persists, and ensures we recover the service state after reinitializing from our `KVStore`.
Previously, we'd persist peer states to the `KVStore`, but, while we pruned them eventually from our in-memory state, we wouldn't remove it from the `KVStore`. Here, we change this and regularly prune and delete peer state entries from the `KVStore`. Note we still prune the state-internal data on peer disconnection, but leave removal to our (BP-driven) async `persist` calls.
We update the docs of `invoice_params_generated` and mention that `user_channel_id` needs to be unique value.
ec94cb3
to
f3da21e
Compare
// We already have persisted otherwise by now. | ||
return Ok(()); | ||
} else { | ||
peer_state_lock.needs_persist = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: maybe optimistically setting needs_persist=false
and then releasing the locks would maybe create some data loss?
with a little help of chatgpt to get a timeline:
Initial State:
- Peer ABC123 exists in the HashMap
- peer_state.needs_persist = true (needs saving)
Timeline:
Time | Thread 1 (Persistence) | Thread 2 (Cleanup) | State |
---|---|---|---|
T1 | Acquires outer READ lock | needs_persist = true | |
T2 | Acquires peer mutex | needs_persist = true | |
T3 | Sets needs_persist = false | needs_persist = false | |
T4 | Encodes peer state | needs_persist = false | |
T5 | Releases ALL locks | needs_persist = false | |
T6 | Starts disk write (slow operation) | Acquires outer WRITE lock | needs_persist = false |
T7 | Disk write FAILS | Removes peer from HashMap | Peer DELETED |
T8 | Disk write FAILS | Releases locks | Peer DELETED |
T9 | Tries to acquire locks for recovery | (not running) | Peer DELETED |
T10 | get(&peer_id) returns None | (not running) | Peer DELETED |
T11 | .map() does nothing silently | (not running) | DATA LOST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed in #3905, we here 'simply' follow the same pattern: if we'd update the state only after the fact we would risk extra writes. Instead we simply revert the needs_persist
flag if the write fails. Note that for important operations we initiate the persistence directly inline.
But sure, no matter how you turn it, if you create new data, and all write attempts fail until you shutdown, it will be lost and not persisted. Apart from that, I'm not quite following how to interpret that timeline and 'Peer DELETED' above (FWIW, not sure it's helpful here?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, a handful of small comments, but I'm ready to land this once they're addressed. Feel free to squash as you go imo.
waker.wake(); | ||
} | ||
|
||
self.condvar.notify_one(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but since we can push multiple events for one Drop
this needs to notify_all
not just one.
0u8 => { | ||
// LSPS0ClientEvents are not persisted. | ||
continue; | ||
}, | ||
1u8 => { | ||
// LSPS1ClientEvents are not persisted. | ||
continue; | ||
}, | ||
2u8 => { | ||
// LSPS1ServiceEvents are not persisted. | ||
continue; | ||
}, | ||
3u8 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother allocating numbers for these? We can't use them if we ever start writing new fields cause they dont read a length and discard that many bytes.
/// You must call [`LSPS1ServiceHandler::update_order_status`] to update the client | ||
/// regarding the status of the payment and order. | ||
/// | ||
/// **Note: ** This event will *not* be persisted across restarts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIssed some events below.
}, | ||
/// You should open a channel using [`ChannelManager::create_channel`]. | ||
/// | ||
/// **Note: ** As this event is persisted and might get replayed after restart, you'll need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any update here?
let pending_requests = new_hash_map(); | ||
let intercept_scid_by_user_channel_id = new_hash_map(); | ||
let intercept_scid_by_channel_id = new_hash_map(); | ||
let needs_persist = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to init this to true
? Presumably we shouldn't do an init persist until we allocate a channel.
peer_state_lock.is_prunable() | ||
} else { | ||
return; | ||
async fn persist_peer_state( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a separate method from prune_peer_state
? Can't we just do them in the same pass?
pub(crate) fn new_with_time_provider( | ||
event_queue: Arc<EventQueue>, pending_messages: Arc<MessageQueue>, channel_manager: CM, | ||
node_signer: NS, config: LSPS5ServiceConfig, time_provider: TP, | ||
peer_states: Vec<(PublicKey, PeerState)>, event_queue: Arc<EventQueue<K>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as LSPS2, let's just read into a hashmap directly so we don't have to into_iter.collect.
} | ||
} | ||
|
||
pub(crate) async fn prune_peer_state(&self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as LSPS2, ISTM this should be combined with the normal persist loop.
LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE, | ||
LSPS5_SERVICE_PERSISTENCE_SECONDARY_NAMESPACE, | ||
&key, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think this is relying on #4113.
/// The primary namespace under which the [`LiquidityManager`] will be persisted. | ||
/// | ||
/// [`LiquidityManager`]: crate::LiquidityManager | ||
pub const LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE: &str = "liquidity"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we be a bit more verbose in the namespace name? Something like ldk_lightning_liquidity_state
?
This is the second PR in a series of PRs adding persistence tolightning-liquidity
(see #4058). As this is already >1000LoC, I now decided to put this up as an intermediary step instead of adding everything in one go.In this PR we add the serialization logic for for the LSPS2 and LSPS5 service handlers as well as for the event queue. We also have
LiquidityManager
take aKVStore
towards which it persists the respective peer states keyed by the counterparty's node id.LiquidityManager::new
now also deserializes any previously-persisted state from that givenKVStore
.We then have
BackgroundProcessor
drive persistence, skip persistence for unchanged LSPS2/LSPS5PeerState
s, and useasync
inline persistence forLSPS2ServiceHandler
where needed.This also adds a bunch of boilerplate to account for both
KVStore
andKVStoreSync
variants, following the approach we previously took withOutputSweeper
etc.cc @martinsaposnic