Check correct merkle leaf size and validate txids on Electrum#4675
Check correct merkle leaf size and validate txids on Electrum#4675tnull wants to merge 3 commits into
Conversation
Electrum confirmation checks must reject transactions whose non-witness serialization is 64 bytes, since txids and Merkle leaves are computed from that serialization. Witness padding can otherwise move total_size above 64 without removing the inner-node ambiguity. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe
Esplora confirmation checks must use the non-witness transaction size for the 64-byte Merkle leaf guard. Witness padding can otherwise raise total_size without changing the serialization hashed into the txid and Merkle tree. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe
Electrum confirmations must reject transaction_get responses whose body does not compute the requested txid. Otherwise a malicious server can substitute an unrelated transaction and provide matching Merkle data for the substituted body. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe
|
I've assigned @wpaulino as a reviewer! |
| if tx.compute_txid() != txid { | ||
| log_error!(self.logger, "Retrieved transaction for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid); | ||
| return Err(InternalError::Failed); | ||
| } |
There was a problem hiding this comment.
This watched-output spend path adds the txid verification but, unlike the watched-transactions path above (line 292), it never applies the is_potentially_unsafe_merkle_leaf check before accepting the spending tx via get_confirmed_tx. Esplora avoids this asymmetry by placing the base-size check inside its shared get_confirmed_tx, covering both paths. The 64-byte merkle-leaf weakness is exactly an attack that lets a crafted node pass validate_merkle_proof, so a malicious server could claim a watched output is spent by a 64-byte (base size) transaction and have it accepted here. Consider moving the base-size check into Electrum's get_confirmed_tx so both call sites are protected.
|
I've re-reviewed the same commit (35e8dac) against my prior findings. The code is unchanged from my last pass — the output-spend asymmetry, stale comments, and brittle test are all still present and already flagged. No new issues surfaced on this pass. SummaryThe core fix is correct: Previously flagged (not re-posted):
No additional issues found. |
|
LGTM once the bot's comment is addressed |
tnull
force-pushed
the
2026-06-merkle-base-size
branch
from
57524e3 to
35e8dac
Compare
|
Updated to line-wrap commit messages. |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
4 participants
Just a few minor Electrum/Esplora fixes.
These findings were discovered by Project Loupe.