tapchannel+tapfreighter: add parcel height hint

[jtobin]

Should resolve #1848, at least future instances of such. More detail below.

Adds a HeightHint() method to the Parcel interface that allows a parcel to include an optional block height hint for its corresponding anchor transaction's confirmation, if this information is available (e.g. via the 'CommitTxBlockHeight' supplied by lnd).

If a parcel contains a height hint, then we use that in the chain porter's SendStateStorePreBroadcast state, falling back to the current block height otherwise. This is expected to resolve the issue described in #1848, in which assets were never found on-chain after a remote-initiated force close in a very particular environment.

My working hypothesis regarding the linked issue is that the node, which appears to have been running with a Neutrino backend, was offline when the force close occurred, catching up only after the force close transaction had been confirmed. (In the logs supplied with that issue, one can see that immediately prior to and after the 'Unilateral close of ChannelPoint..' entry, blocks are being received at a rate of about one per second.)

In this situation, the force close transaction appears to have proceeded through the chain porter's state machine normally, eventually reaching the SendStateStorePreBroadcast state, where its transfer parcel was augmented with a hint of the current block height for its anchor transaction. It then entered the SendStateWaitTxConf state with that hint, and, since the node's chain backend didn't support txindex, waited for transaction confirmation from that block forward. Since the force close had confirmed prior to that block, the parcel just became stuck in that state, never proceeding to a later state in which the transfer proofs would have been persisted to the local proof archive.

When the sweep transaction parcels proceeded through the porter's state machine, they thus hit an error during the SendStateStorePostAnchorTxConf state, since the proofs for the requisite force close transaction couldn't be found.

The fix here, which applies specifically to the remote-initiated force close case, uses the 'CommitTxBlockHeight' information that lnd supplies to 'importCommitTx' as a height hint if it's available, which is the height of the actual block that included the commitment transaction. This should fix future instances of this issue, but doesn't mitigate existing stuck force-close parcels.

[gemini-code-assist]

Summary of Changes

Hello @jtobin, 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 enhances the transaction confirmation process by introducing an optional block height hint within the Parcel interface. This allows the ChainPorter to use a more accurate starting block height for monitoring anchor transaction confirmations, particularly crucial for scenarios like remote-initiated force closes where a node might have been offline. The change specifically leverages lnd's commitment transaction block height information to prevent issues where transfer proofs might not be imported due to transactions confirming before the node caught up.

Highlights

• Parcel Height Hint Interface: Introduced a "HeightHint()" method to the Parcel interface, allowing parcels to optionally provide a block height for their anchor transaction's confirmation.
• Chain Porter Logic Update: The ChainPorter now prioritizes a parcel's provided height hint over the current block height when determining the starting point for transaction confirmation monitoring.
• Remote Force Close Fix: Specifically addresses a bug (resolves [bug]: Assets Not Found On-Chain After Normal Forced Closure to 'Closedchannel' State #1848) where Neutrino-backed nodes could fail to import transfer proofs for remote-initiated force closes if they were offline when the transaction confirmed.
• LND Integration: Utilizes lnd's CommitTxBlockHeight as the height hint for importCommitTx in the AuxSweeper for remote-initiated force closes.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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 introduces a HeightHint to asset transfer parcels to address a bug where nodes with a Neutrino backend could fail to import transfer proofs for remote-initiated force closes. The changes are well-structured and directly solve the described issue by allowing the chain porter to use a specific block height for transaction confirmation, rather than relying on the current block height. My feedback is mainly focused on updating function comments to reflect the new parameters, ensuring the codebase remains well-documented and easy to understand.

[coveralls]

Pull Request Test Coverage Report for Build 20354280750

Details

• 24 of 48 (50.0%) changed or added relevant lines in 6 files are covered.
• 37 unchanged lines in 11 files lost coverage.
• Overall coverage increased (+0.007%) to 56.969%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapchannel/aux_funding_controller.go	0	1	0.0%
tapchannel/aux_closer.go	0	3	0.0%
tapfreighter/parcel.go	11	16	68.75%
tapchannel/aux_sweeper.go	0	6	0.0%
tapfreighter/chain_porter.go	12	21	57.14%

Files with Coverage Reduction	New Missed Lines	%
tapchannel/aux_sweeper.go	1	1.41%
address/mock.go	2	96.2%
asset/asset.go	2	81.03%
asset/group_key.go	2	72.15%
commitment/tap.go	2	85.42%
fn/iter.go	2	62.07%
asset/mock.go	3	73.21%
tapdb/assets_common.go	3	78.34%
tapdb/multiverse.go	6	79.89%
tapdb/universe.go	6	80.58%

Totals
Change from base Build 20350951079:	0.007%
Covered Lines:	65540
Relevant Lines:	115046

---

💛 - Coveralls

[Roasbeef]

If a parcel contains a height hint, then we use that in the chain porter's SendStateStorePreBroadcast state, falling back to the current block height otherwise. This is expected to resolve the issue described in #1848, in which assets were never found on-chain after a remote-initiated force close in a very particular environment.

Have they attempted to reset the height hint cache on their lnd node? If this works for them, then that'll increase our confidence that this PR is on the right track.

[jtobin]

Have they attempted to reset the height hint cache on their lnd node? If this works for them, then that'll increase our confidence that this PR is on the right track.

Having taken a look at this, it seems that lnd will use the incorrect hint from tapd regardless:

(lnd/chainntnfs/txnotifier.go L645)

startHeight := ntfn.HeightHint                                     // incorrect supplied hint
hint, err := n.confirmHintCache.QueryConfirmHint(ntfn.ConfRequest) // cached hint
if err == nil {
	if hint > startHeight {                                        // if cached hint > supplied hint
		Log.Debugf("Using height hint %d retrieved from cache "+
			"for %v instead of %d for conf subscription",
			hint, ntfn.ConfRequest, startHeight)
		startHeight = hint                                         // use (incorrect) cached hint
	}
} else if err != ErrConfirmHintNotFound {                          // otherwise continue to
	Log.Errorf("Unable to query confirm hint for %v: %v",          // use incorrect supplied hint
		ntfn.ConfRequest, err)
}