Allow combining of Prob Scorers by benthecarman · Pull Request #2469 · lightningdevkit/rust-lightning

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

benthecarman · 2023-08-02T22:46:27Z

Could be useful if getting scorers from multiple different sources

tnull · 2023-08-03T06:42:19Z

Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items.

jkczyz · 2023-08-03T15:10:13Z

+ if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) {
+ channel_liquidities.insert(id, item);
+ }


The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.

Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha

codecov-commenter · 2023-08-03T17:28:50Z

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (6f58072) to head (9606fa1).
⚠️ Report is 5614 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04% 
==========================================
Files 106 106 Lines 55784 55779 -5 Branches 55784 55779 -5 ==========================================
- Hits 50398 50376 -22 - Misses 5386 5403 +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2023-08-03T17:49:35Z

+impl<T: Time> ChannelLiquidity<T> {
+	/// The difference between the upper and lower liquidity bounds.
+	fn liquidity_offset_delta(&self) -> u64 {
+ self.max_liquidity_offset_msat - self.min_liquidity_offset_msat


These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)

Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this

I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.

TheBlueMatt · 2023-08-08T23:11:42Z

Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day.

benthecarman · 2023-08-09T07:55:37Z

I think I am now correctly combining the channel liquidities

jkczyz · 2023-08-09T15:33:34Z

+ self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
+ self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);


Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.

jkczyz · 2023-08-09T15:49:56Z

+ self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history);
+
+ if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) {
+ self.last_updated = other.last_updated;


Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.

wpaulino · 2023-10-31T16:34:25Z

Is this still on your radar @benthecarman?

benthecarman · 2023-10-31T17:58:22Z

This would be cool functionality but I'm not really sure what I'm doing here

TheBlueMatt · 2023-11-05T01:05:16Z

Shame, I don't think this PR was that far away.

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

jkczyz reviewed Aug 3, 2023
View reviewed changes

benthecarman force-pushed the combine-scorer branch from b5f44c5 to 9d64597Compare August 3, 2023 17:27

jkczyz reviewed Aug 3, 2023
View reviewed changes

Allow combining of Prob Scorers
9606fa1

benthecarman force-pushed the combine-scorer branch from 9d64597 to 9606fa1Compare August 9, 2023 07:55

tnull reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs

jkczyz reviewed Aug 9, 2023
View reviewed changes

benthecarman closed this Oct 31, 2023

benthecarman deleted the combine-scorer branch October 31, 2023 17:58

TheBlueMatt mentioned this pull request Nov 5, 2023
Combine scorers #2709
Closed

		self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat);
		self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat);

Uh oh!

Conversation

benthecarman commented Aug 2, 2023

Uh oh!

tnull commented Aug 3, 2023

Uh oh!

jkczyzAug 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 3, 2023• edited by codecov BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 8, 2023

Uh oh!

benthecarman commented Aug 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wpaulino commented Oct 31, 2023

Uh oh!

benthecarman commented Oct 31, 2023

Uh oh!

TheBlueMatt commented Nov 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

benthecarman commented Aug 2, 2023

Uh oh!

tnull commented Aug 3, 2023

Uh oh!

jkczyzAug 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 3, 2023• edited by codecov BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 8, 2023

Uh oh!

benthecarman commented Aug 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jkczyzAug 3, 2023•
edited
Loading

codecov-commenter commented Aug 3, 2023•
edited by codecov Bot
Loading

jkczyzAug 3, 2023•
edited
Loading

codecov-commenter commented Aug 3, 2023•
edited by codecov Bot
Loading

jkczyzAug 3, 2023•
edited
Loading

codecov-commenter commented Aug 3, 2023•
edited by codecov Bot
Loading