Skip to content

Routing improvements - #651

Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged

Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom
naumenkogs:2020-06-routing-data-improvements

Conversation

@naumenkogs

@naumenkogsnaumenkogs commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646

TheBlueMatt
TheBlueMatt reviewed Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs Outdated
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch from 6818e76 to 922be75CompareJune 30, 2020 11:45
@naumenkogs

naumenkogs commented Jun 30, 2020

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.

TheBlueMatt
TheBlueMatt reviewed Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/util/config.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch from 922be75 to 5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs mentioned this pull request Jul 8, 2020
Closed
@valentinewallace
valentinewallace self-requested a review July 9, 2020 21:35
valentinewallace
valentinewallace reviewed Jul 10, 2020
View reviewed changes

@valentinewallacevalentinewallace 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 for the contribution @naumenkogs =]

Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs Outdated
Comment threadlightning/src/util/config.rs Outdated
Comment threadlightning/src/util/config.rs Outdated
Comment threadlightning/src/routing/network_graph.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch 2 times, most recently from a96ba6c to a761fb4CompareJuly 15, 2020 10:54
@naumenkogs

naumenkogs commented Jul 15, 2020

Copy link
Copy Markdown
ContributorAuthor

Ready for review again :)
Would use some help with figuring why fuzzing fails though.

@valentinewallace

valentinewallace commented Jul 15, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Ready for review again :)
Would use some help with figuring why fuzzing fails though.

I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do

Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).

naumenkogs
naumenkogs commented Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch 3 times, most recently from 26fb2e0 to 80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt reviewed Jul 20, 2020
View reviewed changes

@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.

Looks good. A few minor nits, but great work!

Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs Outdated
}
}

// TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat

@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)

@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..

I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).

@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.

@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:

  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.

But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?
So you are suggesting to just ignore that spec part, and implement the flow I listed above?

@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.

Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch 3 times, most recently from 8b62d1a to d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch 6 times, most recently from b0141e2 to f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch from f3b4d8d to 45ee4e8CompareJuly 23, 2020 11:31
@codecov

codecovBot commented Jul 23, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #651 into master will increase coverage by 0.04%.
The diff coverage is 99.23%.

Impacted file tree graph

@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.

TheBlueMatt
TheBlueMatt reviewed Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
else if code == 0x1000 | 20 {
res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
let mut flags = chan_update.contents.flags as u16;
if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};

@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.

@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``
If I switch to res.push(upd.contents.flags);, 4 tests fail.

But also, I don't understand why you're suggesting to omit the message_flags part of flags?

@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.

@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.

Comment threadlightning/src/ln/channelmanager.rs Outdated
let mut res = Vec::with_capacity(8 + 128);
res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
let mut flags = upd.contents.flags as u16;
if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};

@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.

@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch 2 times, most recently from 905ba70 to a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed the 2020-06-routing-data-improvements branch from a086924 to dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt merged commit 779ff67 into lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt
, '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" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, '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('^' + ".*" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, '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('^' + ".*" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, '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" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, '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('^' + ".*" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, '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); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Routing improvements by naumenkogs · Pull Request #651 · lightningdevkit/rust-lightning · GitHub
Skip to content
Routing improvements - #651
Merged
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Jul 27, 2020
Merged
Routing improvements#651
TheBlueMatt merged 8 commits into
lightningdevkit:masterfrom 
naumenkogs:2020-06-routing-data-improvements
Conversation
@naumenkogs
@naumenkogsnaumenkogs
commented
Jun 29, 2020

Copy link


Copy Markdown

Contributor
A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646
TheBlueMatt
TheBlueMatt
reviewed
Jun 29, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
6818e76 to
922be75CompareJune 30, 2020 11:45
@naumenkogs
naumenkogs
commented
Jun 30, 2020

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.
TheBlueMatt
TheBlueMatt
reviewed
Jul 2, 2020
View reviewed changes
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
922be75 to
5f551a8CompareJuly 3, 2020 07:41
@naumenkogsnaumenkogs
mentioned this pull request
Jul 8, 2020

Closed

@valentinewallace
valentinewallace
self-requested a review
July 9, 2020 21:35
valentinewallace
valentinewallace
reviewed
Jul 10, 2020
View reviewed changes
@valentinewallacevalentinewallace
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 for the contribution @naumenkogs =]
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/util/config.rs
Outdated

Comment threadlightning/src/routing/network_graph.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
a96ba6c to
a761fb4CompareJuly 15, 2020 10:54
@naumenkogs
naumenkogs
commented
Jul 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
@valentinewallace
valentinewallace
commented
Jul 15, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Ready for review again :)

Would use some help with figuring why fuzzing fails though.
I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do
Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).
naumenkogs
naumenkogs
commented
Jul 16, 2020
View reviewed changes
Comment threadlightning/src/ln/msgs.rs
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
26fb2e0 to
80308d8CompareJuly 18, 2020 13:52
TheBlueMatt
TheBlueMatt
reviewed
Jul 20, 2020
View reviewed changes
@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.

Looks good. A few minor nits, but great work!
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/routing/network_graph.rs
Outdated

 }
 }

 // TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat
@TheBlueMattTheBlueMattJul 20, 2020

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.

Why not just do it now :)
@naumenkogsnaumenkogsJul 21, 2020

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.

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..
I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).
@TheBlueMattTheBlueMattJul 22, 2020

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.

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.
@naumenkogsnaumenkogsJul 23, 2020

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.

Let me re-iterate on your suggestion:
  1. Add max_htlc_value_in_flight field to ChannelDetails struct
  2. When calling for ChannelDetails by list_channels, this value should be filled from the Channel record
  3. Then a user will be providing these channels to get_route via first_hops field
  4. Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.
But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?

So you are suggesting to just ignore that spec part, and implement the flow I listed above?
@TheBlueMattTheBlueMattJul 23, 2020

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.

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
3 times, most recently
from
8b62d1a to
d2bd959CompareJuly 22, 2020 12:15
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
6 times, most recently
from
b0141e2 to
f3b4d8dCompareJuly 23, 2020 11:20
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
f3b4d8d to
45ee4e8CompareJuly 23, 2020 11:31
@codecov
codecovBot
commented
Jul 23, 2020•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #651 into master will increase coverage by 0.04%.

The diff coverage is 99.23%.
Impacted file tree graph
@@ Coverage Diff @@## master #651 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04% 
==========================================
Files 35 35 Lines 21281 21388 +107 ==========================================
+ Hits 19420 19527 +107 
Misses 1861 1861 
Impacted FilesCoverage Δ
lightning/src/routing/router.rs96.58% <95.65%> (+0.12%)⬆️
lightning/src/ln/channel.rs86.64% <100.00%> (ø)
lightning/src/ln/channelmanager.rs85.25% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_tests.rs97.14% <100.00%> (+<0.01%)⬆️
lightning/src/ln/msgs.rs90.31% <100.00%> (+0.27%)⬆️
lightning/src/routing/network_graph.rs91.43% <100.00%> (+0.36%)⬆️
lightning/src/util/test_utils.rs86.13% <100.00%> (+0.05%)⬆️
lightning/src/util/enforcing_trait_impls.rs100.00% <0.00%> (ø)
... and 1 more
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update 50df4cf...dd0e4f4. Read the comment docs.
TheBlueMatt
TheBlueMatt
reviewed
Jul 23, 2020
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 else if code == 0x1000 | 20 {
 res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
 let mut flags = chan_update.contents.flags as u16;
 if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

flags is a u8, I think this diff hunk can be droped.
@naumenkogsnaumenkogsJul 24, 2020

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.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``

If I switch to res.push(upd.contents.flags);, 4 tests fail.
But also, I don't understand why you're suggesting to omit the message_flags part of flags?
@TheBlueMattTheBlueMattJul 24, 2020

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.

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.
@naumenkogsnaumenkogsJul 27, 2020

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.

Addressed.
Comment threadlightning/src/ln/channelmanager.rs
Outdated

 let mut res = Vec::with_capacity(8 + 128);
 res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
 let mut flags = upd.contents.flags as u16;
 if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};
@TheBlueMattTheBlueMattJul 23, 2020

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.

Same here.
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
2 times, most recently
from
905ba70 to
a086924CompareJuly 27, 2020 10:53
@naumenkogs
naumenkogsforce-pushed
the
2020-06-routing-data-improvements
branch
from
a086924 to
dd0e4f4CompareJuly 27, 2020 11:06
@TheBlueMatt
TheBlueMatt
merged commit 779ff67
into
lightningdevkit:masterJul 27, 2020
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@naumenkogs@valentinewallace@TheBlueMatt