routing: include htlc amount in bandwidth hint queries by carlaKC · Pull Request #5512 · lightningnetwork/lnd

carlaKC · 2021-07-14T12:52:57Z

This PR bubbles our htlc amount all the way down to MayAddOutgoingHtlc , implementing the full fix for #5468 (replacing the temporary fix introduced in #5478 to account for nodes that set a 0 minHtlc).

carlaKC · 2021-07-19T06:51:49Z

itests failing, removing request for review while I look into whether it's flakes vs this change

Roasbeef

Completed an initial pass, looks like the changes didn't turn out to be too elaborate which is nice. I took a look at the itest failures, and don't think they're directly related to this PR. Would wager a new rebase (with all the recent flakes that have been hunted) would eliminate a lot of the failures we're seeing right now for this PR.

Roasbeef · 2021-08-06T00:04:17Z

Do we actually need the entire edge info here? Given it can become out of date after a channel update, or do we only care about the channel ID itself?

Do you mean replace the edgeInfo with just a balance hint? Could do that, this is just blind copy-fu from the old impl.

Yeah, I mean just tracking the minimal amount of information here given that's all we need, and everything else can possibly become inconsistent shortly after we start up.

Roasbeef · 2021-08-06T00:07:57Z

Do we actually need both of these? (still getting through the diff)

Could def cut down and just have one method that takes a pointer to amount (if present), I thought it was nicer from the caller's perspective to not need to care about that detail.

Seems duplicate here. I think what we want here,

know the current bandwidth

check if our bandwidth can carry the amount

Step 2 is implemented in both getPolicyLocal and MayAddOutgoingHtlc, sort redundant?

yyforyongyu · 2021-08-25T02:03:51Z

Should we have QueryBandwidth in the Config?

Interested in your opinion here. The QueryBandwidth (now replaced with GetLink) function isn't actually used in these tests. I'm usually in favor of not creating unnecessary mocks. If the test is ever updated to need the function, it'll panic and it will be very apparent where the change needs to be made.

yyforyongyu · 2021-08-25T02:41:39Z

Seems duplicate here. I think what we want here,

know the current bandwidth

check if our bandwidth can carry the amount

Step 2 is implemented in both getPolicyLocal and MayAddOutgoingHtlc, sort redundant?

orijbot · 2021-09-03T07:14:25Z

Visit https://dashboard.github.orijtech.com?back=0&pr=5512&remote=true&repo=carlaKC%2Flnd to see benchmark details.

carlaKC · 2021-09-03T08:48:15Z

Rebased this on #5642 and itest flakes are totally resolved. If we're happy to block this on that merge (don't really see another option), I'll push the rebased version? cc @Roasbeef

Roasbeef · 2021-10-05T23:26:37Z

Rebase now to see if those flakes have been resolved?

carlaKC · 2021-10-07T08:56:04Z

Rebase now to see if those flakes have been resolved?

Much greener after rebase! Failure look unrelated, I think there may be an issue w/ the htlcswitch tests rn?

Roasbeef · 2021-10-08T00:23:47Z

This one?

    switch_test.go:3486: 
        	Error Trace:	switch_test.go:3486
        	Error:      	Target error should be in err chain:
        	            	expected: "dust threshold exceeded"
        	            	in chain: 
        	Test:       	TestSwitchDustForwarding

That was recently added by @Crypt-iQ when fixing some of the dust stuff

yyforyongyu · 2021-10-14T11:21:07Z

Changes looking good, glad all the flakes are gone. Got a few nits and, is there an easy way for us to add itest for this change?

Pass htlc amount down to the channel so that we don't need to rely on minHtlc (and pad it when the channel sets a 0 min htlc). Update test to just check some sane values since we're no longer relying on minHtlc amount at all.

carlaKC · 2021-10-19T07:52:25Z

is there an easy way for us to add itest for this change?

We already have itest coverage for the validation in MayAddOutgoingHtlc in "max htlc pathfind", as for itest coverage for the specific change of passing the htlc amount in rather than using minimum htlc amount, I think that the unit tests are sufficient?

yyforyongyu

LGTM👍

as for itest coverage for the specific change of passing the htlc amount in rather than using minimum htlc amount, I think that the unit tests are sufficient?

I'd say they are different. Imo we should have a unit test to check the function is working within the package and an itest to check the function fits the whole system, which are very different tests. Nonblocking tho as the current change doesn't break the itest for MayAddOutgoingHtlc.

Also the itest timeouts would be fixed by #5845 I think.

Roasbeef

LGTM 🌤

Roasbeef · 2021-10-19T23:30:48Z

+	case lc.channelState.LocalChanCfg.MinHTLC != 0:
+		mockHtlcAmt = lc.channelState.LocalChanCfg.MinHTLC
+
+	// As a last resort, we just add a non-zero amount.


carlaKC requested review from Crypt-iQ and Roasbeef July 14, 2021 12:53

Roasbeef added this to the v0.14.0 milestone Jul 14, 2021

Roasbeef added bug fix routing labels Jul 14, 2021

carlaKC force-pushed the 5468-amountawarehint branch from 3e68f16 to 1ae8b33 Compare July 16, 2021 09:30

carlaKC removed request for Crypt-iQ and Roasbeef July 19, 2021 06:51

Roasbeef reviewed Aug 6, 2021

View reviewed changes

carlaKC force-pushed the 5468-amountawarehint branch from 1ae8b33 to d90e894 Compare August 10, 2021 11:32

Roasbeef requested a review from yyforyongyu August 22, 2021 20:34

carlaKC force-pushed the 5468-amountawarehint branch 2 times, most recently from a6bdb8a to 98ca67a Compare August 24, 2021 13:37

yyforyongyu reviewed Aug 25, 2021

View reviewed changes

carlaKC force-pushed the 5468-amountawarehint branch 2 times, most recently from 0c90d64 to 9b9eb95 Compare August 26, 2021 09:04

Roasbeef added the P2 should be fixed if one has time label Aug 31, 2021

carlaKC force-pushed the 5468-amountawarehint branch from 9b9eb95 to 840ff7e Compare September 3, 2021 07:14

carlaKC force-pushed the 5468-amountawarehint branch from 840ff7e to c1b80fa Compare September 7, 2021 09:21

carlaKC force-pushed the 5468-amountawarehint branch 3 times, most recently from 9acb955 to 85005a2 Compare October 6, 2021 07:58

carlaKC requested review from Roasbeef and yyforyongyu October 7, 2021 08:56

carlaKC force-pushed the 5468-amountawarehint branch from 85005a2 to fee093a Compare October 8, 2021 10:39

yyforyongyu reviewed Oct 14, 2021

View reviewed changes

Comment thread routing/bandwidth.go Outdated

Comment thread routing/bandwidth_test.go Outdated

Comment thread server.go Outdated

carlaKC force-pushed the 5468-amountawarehint branch from fee093a to 8876b57 Compare October 19, 2021 07:44

carlaKC added 3 commits October 19, 2021 09:50

multi: move bandwidth hints behind interface

45de686

release-notes: add amount sensitive bandwidth hints

721ef7d

carlaKC force-pushed the 5468-amountawarehint branch from 8876b57 to 721ef7d Compare October 19, 2021 07:52

yyforyongyu approved these changes Oct 19, 2021

View reviewed changes

Roasbeef approved these changes Oct 19, 2021

View reviewed changes

Roasbeef merged commit 290b78e into lightningnetwork:master Oct 19, 2021

Conversation

carlaKC commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlaKC commented Jul 19, 2021

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orijbot commented Sep 3, 2021

Uh oh!

carlaKC commented Sep 3, 2021

Uh oh!

Roasbeef commented Oct 5, 2021

Uh oh!

carlaKC commented Oct 7, 2021

Uh oh!

Roasbeef commented Oct 8, 2021

Uh oh!

yyforyongyu commented Oct 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlaKC commented Oct 19, 2021

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlaKC commented Jul 14, 2021 •

edited

Loading