Skip to content

Include inbound-claimed-HTLCs in reported channel balances - #1268

Merged
arik-so merged 1 commit into
lightningdevkit:mainfrom
TheBlueMatt:2022-01-balance-underflow
Feb 11, 2022
Merged

Include inbound-claimed-HTLCs in reported channel balances#1268
arik-so merged 1 commit into
lightningdevkit:mainfrom
TheBlueMatt:2022-01-balance-underflow

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Jan 20, 2022

Copy link
Copy Markdown
Collaborator

Given the balance is reported as "total balance if we went to chain
ignoring fees", it seems reasonable to include claimed HTLCs - if
we went to chain we'd get those funds, less on-chain fees. Further,
if we do not include them, its possible to have pending outbound
holding-cell HTLCs underflow the balance calculation, causing a
panic in debug mode, and bogus values in release.

This resolves a subtraction underflow bug found by the
chanmon_consistency fuzz target.

@TheBlueMattTheBlueMatt added this to the 0.0.105 milestone Jan 20, 2022
@codecov-commenter

codecov-commenter commented Jan 20, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1268 (1818c4a) into main (35d4ebb) will increase coverage by 1.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## main #1268 +/- ##
==========================================
+ Coverage 90.40% 91.96% +1.55% 
==========================================
Files 71 71 Lines 38135 47519 +9384 ==========================================
+ Hits 34476 43700 +9224 - Misses 3659 3819 +160 
Impacted FilesCoverage Δ
lightning/src/ln/channel.rs92.95% <100.00%> (+3.60%)⬆️
lightning/src/ln/functional_tests.rs98.48% <100.00%> (+1.11%)⬆️
lightning/src/util/enforcing_trait_impls.rs86.82% <0.00%> (-2.66%)⬇️
lightning-invoice/src/de.rs79.55% <0.00%> (-1.18%)⬇️
lightning/src/ln/chanmon_update_fail_tests.rs97.73% <0.00%> (+0.03%)⬆️
lightning/src/util/test_utils.rs82.39% <0.00%> (+0.18%)⬆️
lightning/src/chain/channelmonitor.rs91.80% <0.00%> (+0.23%)⬆️
lightning/src/routing/scoring.rs94.31% <0.00%> (+1.04%)⬆️
lightning/src/routing/router.rs93.01% <0.00%> (+1.20%)⬆️
... and 8 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 35d4ebb...1818c4a. Read the comment docs.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-01-balance-underflow branch from 83588a3 to 584920cCompareJanuary 24, 2022 23:16
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Added a test, while adding it I discovered this is even harder to hit than I thought, you have to write a custom router on top and can only happen on the first HTLC after open cause otherwise the reserve value protects us against underflow. Would still be nice to land for 105, but its really hard to hit.

valentinewallace
valentinewallace previously approved these changes Feb 3, 2022
Comment threadlightning/src/ln/functional_unit_tests.rs Outdated
arik-so
arik-so previously approved these changes Feb 10, 2022
Given the balance is reported as "total balance if we went to chain
ignoring fees", it seems reasonable to include claimed HTLCs - if
we went to chain we'd get those funds, less on-chain fees. Further,
if we do not include them, its possible to have pending outbound
holding-cell HTLCs underflow the balance calculation, causing a
panic in debug mode, and bogus values in release.
This resolves a subtraction underflow bug found by the
`chanmon_consistency` fuzz target.

@arik-soarik-so left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks great

@arik-so
arik-so merged commit c931380 into lightningdevkit:mainFeb 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

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