-
Notifications
You must be signed in to change notification settings - Fork 2.2k
wallet: fix imported tapscript address behavior in watch-only mode #10119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: fix imported tapscript address behavior in watch-only mode #10119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @guggero, I'm Gemini Code Assist1! 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 introduces a new integration test case designed to reproduce and investigate a specific issue where imported Tapscript addresses behave unexpectedly when LND is operating in watch-only mode with a remote signer. The new test specifically focuses on the scenario of funding a Partially Signed Bitcoin Transaction (PSBT) using a UTXO from such an imported address.
Highlights
- New Tapscript PSBT Funding Test: I've added a new test function,
testTaprootImportTapscriptFullKeyFundPsbt, which simulates importing a Taproot address where only the full key is known, and then attempts to fund and sign a Partially Signed Bitcoin Transaction (PSBT) using a UTXO associated with this imported address. This test is crucial for verifying wallet behavior with imported Tapscript outputs. - Remote Signer Tapscript Import Test: I've introduced a new remote signer integration test,
testRemoteSignerTapscriptImport, which wraps the new Tapscript PSBT funding test (and other existing Tapscript import tests) to run them specifically in a remote signer environment. This is intended to expose the reported discrepancy in behavior. - PSBT Funding Test Refactoring: I've refactored the existing
testFundPsbtinto a reusable helperrunFundPsbtto allow its inclusion in other test scenarios, specifically within the remote signer tests to cover pay-join scenarios.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 or fill out our survey to provide feedback.
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
-
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. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a new integration test to demonstrate an issue with imported tapscript addresses in watch-only mode. The changes involve refactoring an existing test to reuse its logic and adding new test cases for remote signing and taproot script imports.
My review focuses on improving code quality and adherence to the project's style guide. I've identified a couple of areas for improvement:
- A new test function is missing a comment, which is required by the style guide.
- There's significant code duplication between a new test function and an existing one, which could be refactored to improve maintainability.
Overall, the changes are clear and focused on demonstrating the issue. Addressing the feedback will help maintain the codebase's quality.
Referenced style guide elements:
- All functions must have comments that start with the function name and describe the function's purpose and assumptions (lines 15-16).
|
Did quite some digging and found out that, well, this This is now fixed in btcsuite/btcwallet#1022 |
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM🦾
0053cfb to
e9c304e
Compare
|
Updated and rebased after merging btcsuite/btcwallet#1022. |
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This makes sure there is no general issue with running the pay-join FundPsbt test case in a remote signer setup.
This demonstrates that the "imported" account behaves differently for a wallet that's watch-only vs. normal. The testTaprootImportTapscriptFullKeyFundPsbt test case succeeds in a normal run but fails in a remote-signing setup. For some reason, an imported tapscript address ends up in the "default" account when remote-signing but in the "imported" account for a normal wallet.
8231716 to
d1f7995
Compare
Depends on btcsuite/btcwallet#1022.
Demonstrates the base issue that causes lightninglabs/lightning-terminal#1123 and #10120.
Not yet sure what the issue is, perhaps this demonstration case can help find it.
@yyforyongyu you're digging deep into the wallet, perhaps you have an idea?
To see the difference between running the test in a normal setup and in a remote-signing one, you can run:
Normal:
$ make itest icase=taproot_import_scripts --- PASS: TestLightningNetworkDaemon (5.76s) --- PASS: TestLightningNetworkDaemon/tranche00/118-of-302/btcd/taproot_import_scripts (4.57s) PASSWith a remote signer:
$ make itest icase=remote_signer-tapscript_import lnd_taproot_test.go:1161: p2tr outpoint: e05bd624e15ca2227390dfac5f25ff8a86d2272bf4f598e514b0bc56a1cf016c:1 harness_assertion.go:2617: Error Trace: /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/lntest/harness_assertion.go:2617 /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/itest/lnd_taproot_test.go:1166 /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/itest/lnd_remote_signer_test.go:240 /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/itest/lnd_remote_signer_test.go:250 /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/lntest/harness.go:315 /home/guggero/projects/go/src/github.com/lightningnetwork/lnd/itest/lnd_test.go:130 Error: Received unexpected error: tx with hash 6c01cfa156bcb014e598f5f42b27d2868aff255facdf907322a25ce124d65be0 not found Test: TestLightningNetworkDaemon/tranche00/227-of-302/btcd/remote_signer-tapscript_import Messages: outpoint txid_bytes:"l\x01ϡV\xbc\xb0\x14\xe5\x98\xf5\xf4+'҆\x8a\xff%_\xacߐs\"\xa2\\\xe1$\xd6[\xe0" output_index:1 not found in WatchOnly's walletEDIT: This behavior is fixed with btcsuite/btcwallet#1022, so we update to that PR here.