Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
[#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' [#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' [#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' [#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' [#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); [#2189] Score Fee Params as a passed in parameter by henghonglee · Pull Request #2237 · lightningdevkit/rust-lightning · GitHub
Skip to content

[#2189] Score Fee Params as a passed in parameter - #2237

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params
May 11, 2023
Merged

[#2189] Score Fee Params as a passed in parameter#2237
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
henghonglee:issue-2189-score-params

Conversation

@henghonglee

@henghongleehenghonglee commented Apr 26, 2023

Copy link
Copy Markdown
Contributor
[#2189] Score Fee Params as a passed in parameter
Description:
Aims to fix https://github.com/lightningdevkit/rust-lightning/issues/2189
1. Introduce ScoreParams to Score Trait, allowing channel penalty calculations to have custom parameters
2. Split up ProbablisticScoreParamters into FeeParameters and DecayParameters
3. Couple tightly decay_params but decouple fee params so that modifying and customizing fee parameters can be more flexible.
4. Propagate changes to affect get_route and find_route in the router
5. Fix tests, put placeholder values on the various test scorers

@henghongleehenghonglee changed the title Issue 2189: Changes to score paramshttps://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score paramsApr 27, 2023
@henghongleehenghonglee changed the title https://github.com/lightningdevkit/rust-lightning/issues/2189 Issue 2189: Changes to score params[Issue 2189] Changes to score paramsApr 27, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 9ec4e0f to d3273d3CompareApril 27, 2023 07:34

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 9 times, most recently from 65a65ea to b00c185CompareApril 28, 2023 21:21
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Needs rebase, though it looks like you just have some commits that made their way upstream on this branch?

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from 7605325 to 25924b3CompareMay 3, 2023 15:21

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, Ive tried to list down the issues im facing here. tried a few different variations of this solution but ive rolled back to this one since its the simplest and imo closest to what i want to achieve

Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from db1db6e to 9362cbdCompareMay 3, 2023 16:03
Comment threadlightning-background-processor/src/lib.rs Outdated

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt could you also take a quick look at the other comments that i made? in router.rs

Comment threadlightning-background-processor/src/lib.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 530cb5d to 422d73dCompareMay 5, 2023 01:15

@henghongleehenghonglee left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TheBlueMatt, it's finally ready for review :)

Comment threadlightning-background-processor/src/lib.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 422d73d to 1c8c153CompareMay 5, 2023 01:23
@henghonglee
henghonglee marked this pull request as ready for review May 5, 2023 04:15
@henghongleehenghonglee changed the title [Issue 2189] Changes to score params[Issue 2189] Score Fee Params as a passed in parameterMay 5, 2023
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 2 times, most recently from c3e3764 to 490f401CompareMay 9, 2023 00:53
@TheBlueMatt

TheBlueMatt commented May 9, 2023

Copy link
Copy Markdown
Collaborator

You can tell github thinks your line is too long because it cut off the last word and replace it with ... :). I think its cutoff is around 75, but I'm not 100% sure. 70 is always safe. It was 70.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 6d882ab to 567a225CompareMay 9, 2023 04:50
@arik-so
arik-so self-requested a review May 9, 2023 16:51
Comment threadlightning/src/routing/scoring.rs Outdated
pub manual_node_penalties: HashMap<NodeId, u64>,


/// This penalty is applied when `htlc_maximum_msat` is equal to or larger than half of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing out htlc_maximum_msat ≥ 0.5 * channel_capacity might make this a bit easier to read

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this. but happy to make this change if thats necessary

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it in a separate commit to preserve the move-only-ness, but nothing wrong with cleaning things up when they're getting touched anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i can tease that out it into a seperate commit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadlightning/src/routing/scoring.rs Outdated
Comment on lines +525 to +526
/// these decay parameters affect the score of the channel penalty but are usually not changed on a per-route
/// penalty cost call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These
Also, "per-route" could probably go on the next line.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay will do

Comment threadlightning/src/routing/scoring.rs
Comment threadlightning/src/routing/scoring.rs Outdated
Comment threadlightning/src/routing/scoring.rs Outdated
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 567a225 to d1e9af7CompareMay 9, 2023 23:34
@henghonglee
henghonglee requested a review from arik-soMay 9, 2023 23:35
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 5 times, most recently from 8aacbe7 to a2d0363CompareMay 10, 2023 05:11
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Its not critical, but if you do have to push again for some reason, your commit message still has lines (in the body, post-title) that are longer than 70 chars long. This LGTM, however, I'll let @arik-so take another look.

@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch 3 times, most recently from 04dc495 to 0614a6aCompareMay 10, 2023 18:21
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so

@arik-so

Copy link
Copy Markdown
Contributor

Looks good, but there are failing tests, so I imagine you're probably gonna need to push again.

This PR aims to create a "stateless" scorer. Instead of passing
in fee params at construction-time, we want to parametrize the
scorer with an associated "parameter" type, which is then
passed to the router function itself, and allows passing
different parameters per route-finding call.
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from 0614a6a to ecd7992CompareMay 10, 2023 19:53
@henghonglee
henghongleeforce-pushed the issue-2189-score-params branch from ecd7992 to 86af670CompareMay 10, 2023 22:32
@henghonglee

Copy link
Copy Markdown
ContributorAuthor

@arik-so checks seems to be passing now

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


#[cfg(test)]
impl ProbabilisticScoringDecayParameters {
fn zero_penalty() -> Self {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we don't really need a duplicate of default that does the same thing :)

@TheBlueMatt
TheBlueMatt merged commit e61b128 into lightningdevkit:mainMay 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@henghonglee@TheBlueMatt@codecov-commenter@arik-so@jkczyz