Fix signed comparison in `ElectrumClient`
What the developer wrote
Authored by Elias Rohrer
Fix signed comparison in `ElectrumClient`
`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>
What changed, and why it matters
This commit fixes a bug in how Electrum server transaction history is checked. The code was casting a signed (possibly negative) confirmation height into an unsigned number before checking whether it was valid. That meant unconfirmed or invalid entries could be misread as very large heights instead of being skipped, potentially leading to incorrect transaction confirmation data being used by the Lightning wallet.
Review whether the wrapped `u32::MAX` value could have caused any downstream code to fetch or trust a non-existent confirmation height. Consider adding a regression test with a negative `history.height` value. Ensure the fix is included in any release using Electrum-based transaction syncing.
Security signals we found
Integer signedness bug
Cast-before-validation pattern
Potential logic error in transaction confirmation handling
Evidence from the diff
In lightning-transaction-sync/src/electrum.rs, GetHistoryRes::height from the electrum-client crate is an i32. The original code cast it to u32 first and then checked <= 0, which is both logically wrong (a u32 can never be <= 0 except equal to 0) and dangerous because a negative height such as -1 wraps to u32::MAX. The patch moves the <= 0 check before the cast, so negative/unconfirmed heights are correctly skipped before conversion.
Changed components
lightning-transaction-sync/src/electrum.rsElectrumSyncClientElectrum transaction history filteringInspect captured patch +2 / −2
diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs
index 1905456..9d643f4 100644
--- a/lightning-transaction-sync/src/electrum.rs
+++ b/lightning-transaction-sync/src/electrum.rs
@@ -329,11 +329,11 @@ impl<L: Logger> ElectrumSyncClient<L> {
let mut filtered_history =
script_history.iter().filter(|h| h.tx_hash == **txid);
if let Some(history) = filtered_history.next() {
- let prob_conf_height = history.height as u32;
- if prob_conf_height <= 0 {
+ if history.height <= 0 {
// Skip if it's a an unconfirmed entry.
continue;
}
+ let prob_conf_height = history.height as u32;
let confirmed_tx = self.get_confirmed_tx(tx, prob_conf_height)?;
confirmed_txs.push(confirmed_tx);
}
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.