lnwallet: fix InternalKeyForAddr for imported addr #9750
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
d3fec7f to
bbc834c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in InternalKeyForAddr by handling imported Taproot addresses and improves error logging by including the actual type in the error message. It also refactors the related cooperative close tests to share nodes for efficiency and updates the release notes accordingly.
- Fix bug with Taproot addresses for imported keys.
- Enhance error logging to display the correct address type.
- Refactor and optimize cooperative close tests by reusing nodes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lnwallet/interface.go | Update InternalKeyForAddr with exception for imported addresses. |
| itest/lnd_coop_close_external_delivery_test.go | Refactor test cases to reuse nodes and adjust address handling. |
| docs/release-notes/release-notes-0.19.0.md | Update release notes to include the fix for imported Taproot addresses. |
Comments suppressed due to low confidence (1)
itest/lnd_coop_close_external_delivery_test.go:19
- [nitpick] Consider using consistent capitalization for node names (e.g., 'Bob' instead of 'bob') to improve overall readability and consistency with 'Alice'.
bob := ht.NewNodeWithCoins("bob", nil)
There was a problem hiding this comment.
Very nice commit structure! Left some questions and comments.
Previously when handling external taproot addresses, we would exit here due to the address cannot be found as lnd is not aware of the internal key.
Lines 639 to 654 in c9fe051
However when these addresses are imported, it will pass this check and lead to the failure in the itest. I think we might as well just return early if we know a given address is imported.
| alice := ht.NewNodeWithCoins("Alice", nil) | ||
| bob := ht.NewNodeWithCoins("bob", nil) | ||
| ht.ConnectNodes(alice, bob) | ||
| alice, bob *node.HarnessNode, upfrontShutdown bool, |
There was a problem hiding this comment.
I think it's better to start new nodes to keep each test independent. We can actually do sth similar to
lnd/itest/lnd_wallet_import_test.go
Line 32 in c9fe051
So sth like below, which keeps blocks mined small, also nice for the itest scheduler to load balance.
var coopCloseWithExternalTestCases = []*lntest.TestCase{
{
Name: "set P2WPKH delivery address at open",
TestFunc: func(ht *lntest.HarnessTest) {
testCoopCloseWithExternalDeliveryImpl(
ht, true, false,
lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
)
},
},
{
Name: "set P2WPKH delivery address at close",
TestFunc: func(t *lntest.HarnessTest){
...
},
},
...
}There was a problem hiding this comment.
Thanks! I used this test structure in the reworked itest commits.
| FullKeyOnly: true, | ||
| }, | ||
| } | ||
| res := alice.RPC.ImportTapscript(req) |
There was a problem hiding this comment.
Do keys imported via ImportPublicKey also have this issue? I think not since the internal keys are unknown to lnd?
There was a problem hiding this comment.
I added more test cases for "coop close with external delivery" to safeguard against this in the future:
- P2TR address imported with ImportPublicKey
- P2WPKH address imported with ImportPublicKey
lnwallet/interface.go
Outdated
| // do not implement waddrmgr.ManagedPubKeyAddress. | ||
| // See RPC ImportTapscript. | ||
| // Fix https://github.com/lightninglabs/loop/issues/923 | ||
| if walletAddr.Imported() { |
There was a problem hiding this comment.
If we are sure that we don't have the internal key for any imported wallet addresses, we might as well just check walletAddr.Imported() before the interface check, or even earlier.
There was a problem hiding this comment.
Yeah I think we can move this check to reside before the type assertion (but after the nil check).
There was a problem hiding this comment.
I moved the check before the type assertion but after the nil check.
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
bbc834c to
1485fae
Compare
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
4f5d898 to
e4b000b
Compare
lnwallet/interface.go
Outdated
| // do not implement waddrmgr.ManagedPubKeyAddress. | ||
| // See RPC ImportTapscript. | ||
| // Fix https://github.com/lightninglabs/loop/issues/923 | ||
| if walletAddr.Imported() { |
There was a problem hiding this comment.
Yeah I think we can move this check to reside before the type assertion (but after the nil check).
| pk := [33]byte{2, 3, 4} | ||
| if upfrontShutdown { | ||
| // Make new address for second sub-test. | ||
| pk[1]++ |
There was a problem hiding this comment.
Is this actually doing anything? There's no re-assignment here.
There was a problem hiding this comment.
Ah TIL, kind of surprising behavior: https://go.dev/play/p/W07kpFn0B7M
There was a problem hiding this comment.
Yeah, this operation changes the value in the array in-place.
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
e4b000b to
1266c57
Compare
| tt, false, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY, | ||
| ) | ||
| }) | ||
| var coopCloseWithExternalTestCases = []*lntest.TestCase{ |
| @@ -76,7 +75,10 @@ var excludedTestsWindows = []string{ | |||
| // more investigation is needed. | |||
| "channel force close-anchor restart", | |||
| "channel force close-simple taproot restart", | |||
| } | |||
| }, extractNames( | |||
There was a problem hiding this comment.
hmmm, how about we add this to L103? Seems easier this way, also keep this excludedTestsWindows simple
// The name for a bubtest case has the format
// "subtestName-caseName", to exclude a whole subtest, we match
// against "subtestName".
name := strings.Split(tc.Name, "-")[0]
if excludedSet.Contains(name) {
excludedSet.Remove(name)
continue
}
There was a problem hiding this comment.
This might give us false positives with names like channel force close-anchor restart...
There was a problem hiding this comment.
"channel force close" is also a prefixed test.
In itest/list_on_test.go:
allTestCases = appendPrefixed(
"channel force close", allTestCases, channelForceCloseTestCases,
)Removing "channel force close" from excludedSet should not be an issue, since it is not present there. It would be a no-op.
There was a problem hiding this comment.
Hmm, I actually prefer the original approach (extractNames). It is explicit and clear that this is a group of prefixed tests.
| }, | ||
| }, | ||
| { | ||
| Name: "set imported P2TR address (ImportTapscript) at open", |
| @@ -53,20 +109,102 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ | |||
| // balance irrespective of whether the delivery address is in LND's wallet or | |||
| // not. Some users set this value to be an address in a different wallet and | |||
| // this should not affect our ability to accurately report the settled balance. | |||
| // | |||
| // If importTapscript is set, it imports a Taproot script and internal key to | |||
| // alice LND using ImportTapscript to make sure it doesn't interfere with | |||
| @@ -76,7 +75,10 @@ var excludedTestsWindows = []string{ | |||
| // more investigation is needed. | |||
| "channel force close-anchor restart", | |||
| "channel force close-simple taproot restart", | |||
| } | |||
| }, extractNames( | |||
There was a problem hiding this comment.
This might give us false positives with names like channel force close-anchor restart...
Include the variable of interest (walletAddr), not the outcome of the check (pubKeyAddr) which is always nil.
An address imported using ImportTapscript doesn't provide a private key so it can't satisfy waddrmgr.ManagedPubKeyAddress interface. So we don't return an error for imported addresses. Fix lightninglabs/loop#923
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
1266c57 to
4aac7d3
Compare
More sub-tests are needed in "coop close with external delivery" itest, which would break the limit for 50 blocks mined. Turning it into prefixed itest. Also used new exclusion approach in test "remote signer" where all the prefixed tests are excluded.
Make sure that an address imported to LND via ImportTapscript or ImportPublicKey can be used as a delivery address in coop close. New test cases: - P2TR address imported with ImportTapscript - P2TR address imported with ImportPublicKey - P2WPKH address imported with ImportPublicKey Safeguard against lightninglabs/loop#923
starius
force-pushed
the
fix-InternalKeyForAddr-for-imported-addresses
branch
from
4aac7d3 to
dc2cad9
Compare
|
@guggero @yyforyongyu I also used |
Change Description
If a Taproot address is added to LND using the
ImportTapscriptRPC, LND previously failed to perform a cooperative close to that address.The issue was originally reported in the Loop repository: lightninglabs/loop#923
The root cause was in the
lnwallet.InternalKeyForAddrfunction. This function assumed that all Taproot addresses implement theManagedPubKeyAddressinterface, implying that a private key is available. However, this assumption does not hold for externally imported addresses, which is why the function returned an error.This PR updates the logic to add an exception for imported Taproot addresses.
In a separate commit, I also improved the error logging. Previously, the error message was missing the actual type:
unable to fetch internal key: expected pubkey addr, got <nil>Now, the actual type is included, e.g.:
unable to fetch internal key: expected pubkey addr, got *waddrmgr.taprootScriptAddressI added new test cases to the itest
coop close with external deliveryto reproduce the issue and verify the fix.Additionally, I refactored the test to reuse Alice and Bob across cases. This reduced the number of blocks mined, also cutting test runtime by roughly two-thirds. This change was necessary because, after adding new cases, the block count exceeded the allowed limit of 50.
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.