Use MonitorUpdatingPersister
#661
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
| 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.
| // 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.
We use LDK's `MonitorUpdatingPersister` to persist (and read) differential updates, instead of always repersisting the full monitors.
|
Squashed the fixup without further changes.. Going ahead landing this: > git diff-tree -U2 c9f7a65e fc6a7ff1
> |
4 participants
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.