Uh oh!
There was an error while loading. Please reload this page.
Use MonitorUpdatingPersister - #661
Conversation
👋 Thanks for assigning @joostjager as a reviewer! |
Uh oh!
There was an error while loading. Please reload this page.
| pub(crate) fn do_test_store<K: KVStoreSync + Sync>(store_0: &K, store_1: &K) { | ||
| // This value is used later to limit how many iterations we perform. | ||
| let persister_0_max_pending_updates = 7; | ||
| // Intentionally set this to a smaller value to test a different alignment. |
There was a problem hiding this comment.
I've seen this also in rust-lightning, and also there I didn't understand what alignment there is to test because the nodes are independent?
There was a problem hiding this comment.
Yeah, you'd have to ask the OP regarding the comment, but it's also of course not wrong to set a different value here.
There was a problem hiding this comment.
It isn't wrong, but thought it might be worth knowing a bit more about it now that you are taking over.
There was a problem hiding this comment.
@joostjager I think I wrote this sentence first in rust-lightning in this PR: https://github.com/lightningdevkit/rust-lightning/pull/2359/files#diff-0f1c2f21752b2bf468479020ff092a484d8b0a4a1c018241e3a127b62aa51de2R914
I think what we were concerned with was weird edge cases / off-by-one errors in the compaction algorithm in particular. I think there was a state of this PR where odd numbered maximum_pending_updates would drop one or something. Initially we just tested the kind of obvious/likely powers of 10. So deliberately using a few different primes became a thing.
| // Send a few more payments to try all the alignments of max pending updates with | ||
| // updates for a payment sent and received. | ||
| let mut sender = 0; | ||
| for i in 3..=persister_0_max_pending_updates * 2 { |
There was a problem hiding this comment.
This also seems a bit overkill at the ldk node level?
There was a problem hiding this comment.
Mhh, a bit more test coverage doesn't hurt? FWIW, this could be made a proptest to have it be less deterministic, but apart from that there isn't much harm?
There was a problem hiding this comment.
It doesn't hurt in terms over coverage, but it is probably duplicative, and code carries cost.
| assert_eq!(node_txn.len(), 1); | ||
| let txn = vec![node_txn[0].clone(), node_txn[0].clone()]; | ||
| let dummy_block = create_dummy_block(nodes[0].best_block_hash(), 42, txn); | ||
| connect_block(&nodes[1], &dummy_block); |
There was a problem hiding this comment.
Some independent refactoring here?
There was a problem hiding this comment.
Yeah, doesn't hurt either though.
There was a problem hiding this comment.
I meant to say that it is better to isolate in a separate commit.
| )); | ||
| // Read ChannelMonitor state from store | ||
| let channel_monitors = match persister.read_all_channel_monitors_with_updates() { |
There was a problem hiding this comment.
You'd have to ask OP for the original reason, I left it in as grouping the channel monitor IO with setting up the persister and ChainMonitor made sense to me, but there might be different ways to think about it, probably. Let me know if you'd prefer to revert the change.
There was a problem hiding this comment.
It's ok. The origin of the comment was diff optimization.
| const VSS_HARDENED_CHILD_INDEX: u32 = 877; | ||
| const VSS_LNURL_AUTH_HARDENED_CHILD_INDEX: u32 = 138; | ||
| const LSPS_HARDENED_CHILD_INDEX: u32 = 577; | ||
| const PERSISTER_MAX_PENDING_UPDATES: u64 = 100; |
There was a problem hiding this comment.
Is there science to the number 100?
There was a problem hiding this comment.
No, just handwaving currently. There has been some benchmarking over at lightningdevkit/rust-lightning#3834 but it never got anywhere super conclusive. Let me know if you have a better guess.
There was a problem hiding this comment.
Does not seem unreasonable. Going higher is probably not gaining much. Maybe it could be a bit lower. But also fine as is.
joostjager
left a comment
There was a problem hiding this comment.
Replied. Nothing blocking.
We use LDK's `MonitorUpdatingPersister` to persist (and read) differential updates, instead of always repersisting the full monitors.
tnull
commented
Oct 16, 2025
Squashed the fixup without further changes.. Going ahead landing this: > git diff-tree -U2 c9f7a65e fc6a7ff1> |
Uh oh!
There was an error while loading. Please reload this page.
Closes#441
We use LDK's
MonitorUpdatingPersisterto persist (and read) differential updates, instead of always repersisting the full monitors.This is mostly a simple rebase of #456 (whose original author moved on) on top of current main.