Avoid re-locking reused UTXO futures

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.

[tnull]

UtxoLookup implementations may cache and return the same async future for repeated requests for a short channel id. When a replacement channel announcement arrived for an in-flight lookup, the async path held the future state while comparing the existing pending entry, which could point to that same state.

Drop the state guard before checking or replacing the pending entry so repeated lookups can update the pending announcement without re-entering the mutex.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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]

No issues found.

The implementation changed since my prior review pass (it now compares through the already-held guard via Arc::ptr_eq rather than dropping/re-locking the guard), but the new logic is correct:

• The deadlock is avoided by not re-locking future.state when the pending entry is Arc::ptr_eq to it; the comparison reads replacement_messages (the held guard) directly.
• At compare time async_messages.channel_announce still holds the previous announcement (the new one is written after the check_replace_previous_entry call at utxo.rs:475), so it correctly compares the incoming message against the existing pending one.
• The else branch's unsafe_well_ordered_double_lock_self only runs for a genuinely different mutex, preserving the new→old lock order.
• resolve_single_future runs only under the internal lock held throughout, so no interleaving can lose updates.
• The regression test is valid (shared SCID 0 and fixed bitcoin keys make good_script match the replacement).

One non-blocking observation (not a regression, and strictly better than the deadlock being fixed): with a reused future for the same SCID but a different announcement, there is a single channel_announce slot, so the second announcement overwrites the first and resolve_single_future (utxo.rs:538) processes only the survivor. The nearby comment at utxo.rs:367 ("both results will be handled") only holds when the two requests have distinct future states. This is acceptable for the same-SCID case since the funding output is fixed and only one set of bitcoin keys can match the on-chain script.

[tnull]

Updated to line-wrap commit messages.

[TheBlueMatt]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

[ldk-reviews-bot]

👋 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.

[tnull]

Not entirely sure anyone would ever implement it this way, but it seems the correct fix is to handle the duplicate with an Arc::ptr_eq check in check_replace_previous_entry, not unlock+relock.

Now updated with this approach. Let me know if you find that preferable, or if we should even consider not fixing this?

[TheBlueMatt]

thanks

[TheBlueMatt]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

[tnull]

IMO we should skip backporting this. I don't believe any of our implementations of the lookup interface will ever return the same future and implementing that interface by caching pending lookups and returning copies of the future seems like a lot of work that implementers aren't gonna do. LMK if you disagree @tnull

Fine by me.