fix height hint Zero issue in utxonursery

[ziggie1984]

Fixes: #8383

please try-out this patch if you can @robtex

[gemini-code-assist]

Summary of Changes

Hello @ziggie1984, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical issue within the utxonursery (a legacy sweeper component) where certain HTLCs, referred to as crib outputs, could have an expiry height of 0 due to a past bug. This 0 value would cause problems with the chain notifier, preventing these outputs from being swept. The changes introduce a robust fallback mechanism that intelligently determines a valid, non-zero height hint for these problematic outputs by leveraging the channel's close height, thereby ensuring all HTLCs are correctly processed and preventing potential fund lock-ups.

Highlights

• Zero Height Hint Patch: Implemented a new function, patchZeroHeightHint, to address an edge case where crib outputs (HTLCs) might have an expiry height of 0 due to a historical bug. This function provides a fallback mechanism to derive a valid height hint for the chain notifier using the channel's close height, ensuring the hint is always greater than 0.
• Integration with Sweeper Logic: Modified the sweepCribOutput function in the utxonursery to utilize the new patchZeroHeightHint function. This ensures that all crib outputs, including those with a 0 expiry, are correctly processed and registered for confirmation monitoring.
• Comprehensive Testing: Added a new test suite, TestPatchZeroHeightHint, to thoroughly validate the new height hint patching logic. This includes various scenarios such as normal non-zero classHeight, different closeHeight values relative to confDepth when classHeight is 0, and error handling for fetching closed channel summaries.
• Release Notes Update: Included an entry in the release notes for version 0.20.0 to document the fix for the utxonursery issue where HTLCs with a locktime of 0 were not being swept, providing transparency about the resolved bug.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a bug in the legacy utxonursery where HTLCs with a zero timelock would not be swept due to an invalid height hint. The fix introduces a fallback mechanism to derive a valid height hint from the channel's close height, ensuring these outputs can be processed correctly. The changes are well-implemented, include comprehensive unit tests for the new logic, and update the release notes accordingly. The code quality is high, and I have no further suggestions for improvement.

[ziggie1984]

That was the error for the reviewer:

2025-10-06 14:51:37.587 [INF] UTXN: Publishing CLTV-delayed HTLC output using timeout tx (txid=62ff2f1f063082090be8aecd42bba5ae29b4b1b1a00e1f13806b9d99f8087e00): (*wire.MsgTx)(0xc007f7fc00)({
 Version: (int32) 2,
 TxIn: ([]*wire.TxIn) (len=1 cap=1) {
  (*wire.TxIn)(0xc003b4ccc0)({
   PreviousOutPoint: (wire.OutPoint) 81e84010f3eb56e10eb808b00711c46cf1a96368015f83a21223c979c7b846f9:1,
   SignatureScript: ([]uint8) {
   },
   Witness: (wire.TxWitness) (len=5 cap=5) {
    ([]uint8) {
    },
    ([]uint8) (len=72 cap=72) {
     00000000  30 45 02 21 00 87 d8 f4  39 83 5a 5d 71 46 17 fb  |0E.!....9.Z]qF..|
     00000010  b4 c1 5b aa fd 91 89 9d  5c 30 82 fe 58 61 94 6c  |..[.....\0..Xa.l|
     00000020  dd 58 af b3 38 02 20 19  33 74 07 a1 b8 c8 6d 43  |.X..8. .3t....mC|
     00000030  a1 49 24 e9 a0 08 8e 4e  d7 e0 94 de d3 99 e6 40  |.I$....N.......@|
     00000040  67 98 f4 92 43 27 92 01                           |g...C'..|
    },
    ([]uint8) (len=71 cap=71) {
     00000000  30 44 02 20 0d b3 01 32  c5 c3 b9 97 a2 bd 66 a1  |0D. ...2......f.|
     00000010  aa c5 2a 55 6a 42 1f 2d  30 4d 6f 51 0a 66 b6 d7  |..*UjB.-0MoQ.f..|
     00000020  0d ae 4d 57 02 20 44 d0  f9 62 38 81 f2 8d be f4  |..MW. D..b8.....|
     00000030  57 48 7e 0a 36 dd 7f 0c  30 fb c9 1c a4 9e ba 25  |WH~.6...0......%|
     00000040  64 25 04 7b 4d 39 01                              |d%.{M9.|
    },
    ([]uint8) {
    },
    ([]uint8) (len=133 cap=133) {
     00000000  76 a9 14 69 d9 29 69 40  47 52 24 bd 20 64 c3 52  |v..i.)i@GR$. d.R|
     00000010  10 f7 44 08 b5 18 72 87  63 ac 67 21 03 41 0a f5  |..D...r.c.g!.A..|
     00000020  bb e6 aa b1 37 6b 79 26  df 99 3d 7f 8d a7 6f 39  |....7ky&..=...o9|
     00000030  90 4c 1c 21 02 59 fe 61  72 72 6c 66 4c 7c 82 01  |.L.!.Y.arrlfL|..|
     00000040  20 87 64 75 52 7c 21 02  9c 84 44 ae a5 0f 39 df  | .duR|!...D...9.|
     00000050  08 19 f6 99 d9 72 36 ee  b5 53 ab c6 e8 8d a6 32  |.....r6..S.....2|
     00000060  5f 0e 2d 14 4a 1a 0f 3b  52 ae 67 a9 14 b9 33 f0  |_.-.J..;R.g...3.|
     00000070  45 99 7c 8c 49 52 af 17  92 c8 a4 bd 01 5a 29 be  |E.|.IR.......Z).|
     00000080  c4 88 ac 68 68                                    |...hh|
    }
   },
   Sequence: (uint32) 0
  })
 },
 TxOut: ([]*wire.TxOut) (len=1 cap=1) {
  (*wire.TxOut)(0xc007f275a0)({
   Value: (int64) 15072,
   PkScript: ([]uint8) (len=34 cap=34) {
    00000000  00 20 e2 f3 5d 12 f8 c8  d0 14 cd bb ab 65 65 c8  |. ..]........ee.|
    00000010  12 05 67 e2 aa bd eb 89  ef 05 bb c7 9e 89 e9 d5  |..g.............|
    00000020  1e 9d                                             |..|
   }
  })
 },
 LockTime: (uint32) 0
})

2025-10-06 14:51:37.730 [ERR] BTCN: Broadcast attempt failed: Transaction already in block chain
2025-10-06 14:51:37.862 [WRN] BTWL: Transaction 62ff2f1f063082090be8aecd42bba5ae29b4b1b1a00e1f13806b9d99f8087e00 not accepted by mempool: txn-already-known
2025-10-06 14:51:37.862 [INF] BTWL: Inserting unconfirmed transaction 62ff2f1f063082090be8aecd42bba5ae29b4b1b1a00e1f13806b9d99f8087e00
2025-10-06 14:51:37.998 [INF] BTWL: 62ff2f1f063082090be8aecd42bba5ae29b4b1b1a00e1f13806b9d99f8087e00: tx already confirmed
2025-10-06 14:51:38.130 [ERR] UTXN: Failed to sweep first-stage HTLC (CLTV-delayed) output 62ff2f1f063082090be8aecd42bba5ae29b4b1b1a00e1f13806b9d99f8087e00:0
2025-10-06 14:51:38.130 [ERR] UTXN: Failed to sweep outputs at height=0: a height hint greater than 0 must be provided
2025-10-06 14:51:38.130 [ERR] UTXN: Failed to reload class: a height hint greater than 0 must be provided

[ziggie1984]

The patch worked as foreseen.

The node operator had however one node which had corrupted data entries, if this happens we error out which is the best way imo otherwise we do not recognise this behaviour.

2025-10-07 16:42:53.098 [ERR] UTXN: Failed to sweep first-stage HTLC (CLTV-delayed) output 1ce1ca1ebcf883ce95cd134291d510710447c477daadc6de90b5eb4155170b20:620756992
2025-10-07 16:42:53.098 [ERR] UTXN: Failed to sweep outputs at height=0: cannot determine height hint for crib output with expiry=0: unable to find closed channel summary
2025-10-07 16:42:53.098 [ERR] UTXN: Failed to reload class: cannot determine height hint for crib output with expiry=0: unable to find closed channel summary
2025-10-07 16:42:53.098 [INF] UTXN: UTXO nursery shutting down...

We removed those entries manually, they can still be swept with chantools.

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a fix for a crash in utxonursery when handling outputs with a zero expiry height, which is a remnant of a historical bug. The core of the change is the new patchZeroHeightHint function, which provides a fallback mechanism to determine a valid height hint for the chain notifier.

The logic for the fallback mechanism is sound, and the addition of comprehensive unit tests is commendable. However, I've identified a critical issue in the implementation that could lead to the same crash it intends to fix, along with corresponding adjustments needed in the tests.

My review is based on the LND Style Guide provided in the repository.

[ziggie1984]

@gemini-code-assist can you check if this current approach solves all the priority problems from before.

[gemini-code-assist]

Hello @ziggie1984,

Thank you for the update and for sharing the detailed error logs. It's very helpful to see the real-world impact of the changes.

Regarding your question, "can you check if this current approach solves all the priority problems from before?":

Based on the logs you provided, the patch successfully addresses the initial critical issue where a classHeight (expiry) of 0 would lead to a generic error: a height hint greater than 0 must be provided. The new patchZeroHeightHint function now correctly attempts to derive a valid height hint using the channel's close summary.

However, your second log snippet highlights an important edge case:

[ERR] UTXN: Failed to sweep outputs at height=0: cannot determine height hint for crib output with expiry=0: unable to find closed channel summary
[ERR] UTXN: Failed to reload class: cannot determine height hint for crib output with expiry=0: unable to find closed channel summary

This indicates that while the patch provides a robust fallback mechanism, it still relies on the availability of the closed channel summary. If this summary cannot be found (e.g., due to corrupted data entries, as you mentioned), the system will still error out, albeit with a more specific and informative message about the missing channel summary. This is an improvement in error reporting, as it pinpoints the root cause more accurately.

So, to directly answer your question:

• The primary priority problem (generic failure to sweep due to expiry=0 and the chain notifier's height > 0 requirement) is solved by introducing the intelligent fallback logic.
• A secondary priority problem (or rather, an underlying data consistency issue) is now clearly identified: the inability to find the closed channel summary for certain crib outputs. The current approach correctly surfaces this specific data problem, preventing incorrect processing and allowing for manual intervention or further investigation, as you've done with chantools.

This approach ensures that the system doesn't proceed with invalid height hints and provides clear diagnostics when necessary channel data is missing. It's a significant step forward in handling these problematic legacy outputs.

[ziggie1984]

Decided to still error out in case the closeSummary is borked.

[robtex]

Fixes: #8383

please try-out this patch if you can @robtex

Confirmed this helped two really old nodes up and running again.
Both nodes have had some patches before.

Thanks a lot!!!

[Roasbeef]

Tests need to be updated after the latest change.

[Roasbeef]

Latest logic looks good, but the tests need patching.

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses an issue where a zero height hint in the utxonursery could cause failures. The fix introduces a new function, patchZeroHeightHint, to provide a fallback height hint by using channel closing information. The changes are logical and well-supported by a comprehensive set of new unit tests that cover various edge cases. My review focuses on improving the clarity and accuracy of comments within the new function to enhance code maintainability, aligning with the style guide's emphasis on readable code.

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses an issue where the utxonursery could fail to sweep HTLCs with a zero expiry height. The core of the fix, the patchZeroHeightHint function, introduces a robust fallback mechanism for determining a valid height hint by using the channel's close summary. The logic is well-considered, handling various edge cases such as a zero channel close height and ensuring the derived hint is sane with respect to SegWit activation and confirmation depth. The accompanying tests are comprehensive and validate the new logic paths thoroughly.

I have a couple of suggestions to enhance the clarity and maintainability of the error messages within the new function. Overall, this is a solid and well-tested fix.

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a fix for an issue where the utxonursery would fail to sweep HTLCs that have a zero expiry height, a situation caused by a historical bug. The fix introduces a fallback mechanism in patchZeroHeightHint to determine a valid height hint using the channel's close summary. The changes are well-tested with a new comprehensive unit test.

My main feedback is regarding a hardcoded mainnet constant for the SegWit activation height, which will cause issues on other networks. This should be made dynamic based on the channel's network parameters. The tests will also need to be updated accordingly.

[gemini-code-assist]

The segwitActiveHeight is hardcoded to the mainnet value. This will cause issues on other networks like testnet, where SegWit activated at a different height. This constant should be removed. Instead, the SegWit activation height should be determined dynamically within patchZeroHeightHint based on the channel's network parameters, which can be derived from the ChainHash available in the ChannelCloseSummary.

[ziggie1984]

decided to remove this check to have a cleaner design rather than hardcoding mainnet values, this should never happen in the first place anyways.

[yyforyongyu]

LGTM ⛵

[yyforyongyu]

Normally this should be impossible right?

[ziggie1984]

correct

[yyforyongyu]

Feel like this should be impossible as the outputs can only be spent after the closing tx is confirmed, but also not very important as we are just scanning a few more blocks.

[ziggie1984]

took the logic from other parts in the utxo nursery where we would do that, so tried to keep it (maybe there is a race I don't know about haha)