Skip to content

Fix signed comparison in ElectrumClient#4590

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
tnull:2026-05-fix-electrum-comparison
May 5, 2026
Merged

Fix signed comparison in ElectrumClient#4590
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
tnull:2026-05-fix-electrum-comparison

Conversation

@tnull
Copy link
Copy Markdown
Contributor

@tnull tnull commented May 5, 2026

GetHistoryRes::height from electrum-client is a signed integer. Here we first check for <= 0 before casting to u32.

`GetHistoryRes::height` from electrum-client is a *signed* integer. Here
we first check for `<= 0` *before* casting to `u32`.

Signed-off-by: Elias Rohrer <dev@tnull.de>
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 5, 2026

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

I've reviewed the full diff and examined all related height as u32 casts in the file. The fix is correct:

  • Before: history.height (a signed i32) was cast to u32 before the <= 0 check, so negative values (e.g., -1 for unconfirmed-with-unconfirmed-inputs) would wrap to large positive numbers, bypassing the guard entirely.
  • After: The comparison happens on the signed value, and the as u32 cast only occurs when height > 0.

The other height as u32 casts in the file (lines 97, 253) operate on HeaderNotification::height which is usize (unsigned), so they don't have the same issue. The cast at line 380 is already guarded by the pre-existing check at line 354.

No issues found.

@ldk-reviews-bot
Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@TheBlueMatt TheBlueMatt merged commit ccdfddd into lightningdevkit:main May 5, 2026
20 of 22 checks passed
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.23%. Comparing base (2313bd5) to head (8b383bb).
⚠️ Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4590      +/-   ##
==========================================
- Coverage   87.08%   86.23%   -0.86%     
==========================================
  Files         161      159       -2     
  Lines      109255   109103     -152     
  Branches   109255   109103     -152     
==========================================
- Hits        95147    94083    -1064     
- Misses      11627    12410     +783     
- Partials     2481     2610     +129     
Flag Coverage Δ
fuzzing-fake-hashes ?
fuzzing-real-hashes ?
tests 86.23% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants