feat: Make MonitorUpdatingPersister change persist type based on size

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach

[Prabhat1308]

fixes#3770

• Skips full persistence when ChannelMonitor is smaller than a pre-determined size.
• Adds a field minimum_monitor_size_for_updates to specify the minimum size for full persistence to be activated
• Adds new_with_default_threshold function to setup MonitorUpdatingPersister with a default minimum_monitor_size_for_updates value

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.

[tnull]

Mostly looks good, but can we add some test coverage for the new behavior?

Additionally, benchmarks would indeed very helpful to evaluate what a reasonable threshold value would be.

[tnull]

Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/

[Prabhat1308]

I have created a minimal benchmarking setup here for this.
https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

[TheBlueMatt]

I'm not sure we can conclude all that much from a local-filesystem benchmark, sadly. We have several users (and hopefully more soon with ldk-node) who use the MonitorUpdatingPersister with remote storage, where costs can be very different from local (eg IP packet size bound). I'd say we use a threshold of 8192 for now and call it a day.

[tnull]

I have created a minimal benchmarking setup here for this. https://github.com/Prabhat1308/rust-lightning/tree/probot/benchmark

from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger. Maybe @TheBlueMatt can provide some realistic monitor sizes here?

I'm asking because I suspect the results may stem from a latency vs. throughput trade-off, and under certain circumstances one might dominate over the other. The filesystem store for example likely (especially assuming that most file systems by now have 4kb block size) does not incur that much more latency when reading/writing 'larger' monitors (as 8kb is still tiny and the syscall / IO latency likely the dominant factor), while persistence to a remote server would see much slower write speeds and hence higher latency when (re-)persisting full monitors.

[TheBlueMatt]

Maybe @TheBlueMatt can provide some realistic monitor sizes here?

Yea, so 8KiB is a reasonable "channel that got opened and has only done a handful of HTLC operations in its history" threshold (tho is maybe even a bit too small for a more active mobile wallet). On my routing node the largest monitor is ~73MiB, I imagine c= has some that get into the hundreds of MiB.

[Prabhat1308]

Thanks for taking a first look at these benchmarks. How did you arrive at some of these assumptions? E.g., how did you decide on the 8KB threshold being 'large' monitors. I believe in reality monitors could end up being much larger.

My assumptions here were to test the crossover point to select what was the optimal value of the threshold . Since the talks in the issue for threshold were in kBs , I assumed them to be in the same range and didn't go as far as to check the monitors in MB range and the results I was getting was becoming worse as I was moving towards bigger sizes which also made me not go higher. So the comments to monitor sizes in benchmark are not of much significance as they were relative to 4kB monitor size.

[domZippilli]

Left some thoughts on the issue, since my thoughts aren't about this implementation but more the feature itself. But posting here since this is where the recent action is. 🙇

[yuvicc]

Concept ACK

[tnull]

Current changes look good to me, but I'll defer ACKing to after the discussion on whether we want this change afterall has concluded.

[domZippilli]

I wonder if, instead of a "with" constructor, it would be nicer/better to either add this to the default constructor, or use a config struct, or write a builder.

It's not a problem here, but I wonder if we'll think of another tunable for MUP and then there will be a third constructor, and so on.

[Prabhat1308]

I have for now added this in the default constructor , since there is only 1 parameter that was affected . If more such tunables are added in the future , I would lean towards the config approach