Validate Esplora merkle proof against the block header's merkle root

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.

[tnull]

EsploraSyncClient::get_confirmed_tx parsed the SPV proof returned by the Esplora server but threw away the security check: the merkle root computed by PartialMerkleTree::extract_matches was discarded (let _ = …), and only the leaf-equality check (matches[0] == txid) remained. Anyone can construct a single-leaf partial tree advertising an arbitrary txid via PartialMerkleTree::from_txids(&[txid], &[true]), so this gate was vacuous.

Note: I recall that at implementation time we decided doing this check wasn't necessary/redundant. But, it can surely not hurt to be more strict here.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

[ldk-claude-review-bot]

The fix is straightforward and correct. Let me verify the full diff one more time to make sure I haven't missed anything.

The change is isolated to 6 lines in esplora.rs. The logic:

1. extract_matches result is now captured via .ok() → Option<TxMerkleNode>
2. Compared against Some(block_header.merkle_root)
3. If extract_matches fails (returns Err), .ok() produces None, which correctly fails the check
4. All prior validation conditions are preserved

No issues found in the code change itself. The one concern worth raising is the lack of a regression test.

No issues found.

Cross-cutting concern:

• This is a security-critical fix but has no unit/regression test demonstrating that a forged merkle proof (one where the partial tree computes to a different root than block_header.merkle_root) is now rejected. The existing integration tests in lightning-transaction-sync/tests/integration_tests.rs use a live electrsd and don't cover adversarial server responses. A targeted unit test constructing a MerkleBlock with a mismatched root would lock in this invariant and prevent future regressions. That said, mocking the Esplora client may not be trivial, so this is a suggestion rather than a blocker.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (1a26867) to head (b64efcd).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4596 +/- ##
==========================================
- Coverage 86.84% 86.22% -0.62% 
==========================================
Files 161 159 -2 Lines 109260 109170 -90 Branches 109260 109170 -90 ==========================================
- Hits 94882 94136 -746 - Misses 11797 12424 +627 - Partials 2581 2610 +29 

Flag	Coverage Δ
fuzzing-fake-hashes	?
fuzzing-real-hashes	?
tests	86.22% <ø> (+<0.01%)	⬆️

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.

[TheBlueMatt]

Backported to 0.1 in #4680.

[TheBlueMatt]

Backported to 0.2 in #4683.