signer: create workaround for SignOutputRaw quirk #203
Conversation
This commit fixes a long-standing issue with how SignOutputRaw populates the key descriptor before calling into lnd. If the public key is available, _only_ the public key is populated and the key locator (index+family) is not. That works well for any keys the wallet is aware of. But if a wallet is restored from seed, it will not know any addresses/keys apart from index 0 of each family/account. And a lookup by public key only will fail. To fix that, we add a new method SignOutputRawKeyLocator that has an updated behavior that also sends along the key locator if we're certain it is fully known. Because changing any behavior in this area of the code might lead to breaking existing behavior some clients like Loop or Pool rely on, we explicitly don't change the original method but rather add a new one.
|
Will help solve the problem described in lightninglabs/taproot-assets#1208. |
| // index will work. But for a freshly initialized wallet (e.g. restored | ||
| // from seed), we won't know any indexes greater than 0, so we _need_ to | ||
| // also specify the key locator and not just the public key. | ||
| fullDescriptor := func( |
There was a problem hiding this comment.
nit: instead of these two closures, we could just have one: descriptor(d keychain.KeyDescriptor, partial bool) and check for partial when filling the keydescriptor.
There was a problem hiding this comment.
I wanted to make this as clear and obvious as possible, with as much comment on why we're doing it that way, so I went with two closures on purpose. Just so nobody else spends debug hours on this (because this is already the second or third time I ran into this, once in Loop and once in Pool).
Correct that the public key look up will fail, however this should be accounted for with the existing logic: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L324-L363 This path is taken when we need to derive the private key based on the keyloc or keydesc. Checking out the diff now. |
| // derived, we know that only using the public key for that very first | ||
| // index will work. But for a freshly initialized wallet (e.g. restored | ||
| // from seed), we won't know any indexes greater than 0, so we _need_ to | ||
| // also specify the key locator and not just the public key. |
There was a problem hiding this comment.
This is where I'm confused. If we only specify the public key, then we'll derive all private keys from 0 to 10k for that key family, checking the public key each time.
With the way the logic works, there're 3 main paths:
- You specify no public key, it checks the cache: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L266-L280
- Index is set, or no public key, it tries to derive from scratch: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L300-L322
- Only pubkey, it scans: https://github.com/lightningnetwork/lnd/blob/94f7ed46deab346cc19ff1e6aa00cecdfe3eb775/keychain/btcwallet.go#L324-L363
When we hit disk it eventually calls into deriveKeyFromPath: https://github.com/btcsuite/btcwallet/blob/66a3aeef6e78751e644a3d03fd856e982e0db4ac/waddrmgr/scoped_manager.go#L742-L769. DIsk wise, this only need the account to exist, as then it'll decrypt the master key and derive the addr as normal: https://github.com/btcsuite/btcwallet/blob/66a3aeef6e78751e644a3d03fd856e982e0db4ac/waddrmgr/scoped_manager.go#L385-L416
There was a problem hiding this comment.
We don't land in any of the code paths you linked... The problem is that the signrpc.SignOutputRaw RPC uses input.Signer.SignOutputRaw which is implemented by BtcWallet.SignOutputRaw which then calls into BtcWallet.fetchPrivKey which has a completely different logic.
| // index will work. But for a freshly initialized wallet (e.g. restored | ||
| // from seed), we won't know any indexes greater than 0, so we _need_ to | ||
| // also specify the key locator and not just the public key. | ||
| fullDescriptor := func( |
There was a problem hiding this comment.
What is resolved by setting both? From my reading of the code, only one or the other is read.
This commit fixes a long-standing issue with how SignOutputRaw populates the key descriptor before calling into lnd.
If the public key is available, only the public key is populated and the key locator (index+family) is not. That works well for any keys the wallet is aware of.
But if a wallet is restored from seed, it will not know any addresses/keys apart from index 0 of each family/account. And a lookup by public key only will fail.
To fix that, we add a new method SignOutputRawKeyLocator that has an updated behavior that also sends along the key locator if we're certain it is fully known.
Because changing any behavior in this area of the code might lead to breaking existing behavior some clients like Loop or Pool rely on, we explicitly don't change the original method but rather add a new one.
Pull Request Checklist
in
lnd_services.goare updated.macaroon_recipes.goif your PR adds a new method that is calleddifferently than the RPC method it invokes.