Skip to content
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

[bug]: Remote Signer: FundPsbt: change output lacking master fingerprint (XFP) #8626

Closed
kaloudis opened this issue Apr 6, 2024 · 8 comments · Fixed by #8630
Closed

[bug]: Remote Signer: FundPsbt: change output lacking master fingerprint (XFP) #8626

kaloudis opened this issue Apr 6, 2024 · 8 comments · Fixed by #8630
Assignees
Labels
bug Unintended code behaviour P1 MUST be fixed or reviewed psbt
Milestone

Comments

@kaloudis
Copy link
Contributor

kaloudis commented Apr 6, 2024

Background

In trying to craft a transaction using an input from an external watch-only account, it appears that the change output lacks a master fingerprint (XFP). The input has the XFP marked correctly.

This causes problems when signing with a Coldcard - error message: Change Fraud. Output#1: Deception regarding change output. BIP-32 path doesn't match actual address

Your environment

  • lnd v0.17.4-beta
  • Mobile: Android + iOS
  • Neutrino backend
  • Coldcard Q1

PSBT in question:

cHNidP8BAHECAAAAAV98V4mBxyrr/qXNORSKhIvksrvlgRh8Oe0Sb23dj2D6AQAAAAAAAAAAAqGGAQAAAAAAFgAUW1KNJgFHE1i9lES2sS7Z1f14wuuECAUAAAAAABYAFACfF8zGqqFe/Rd08dmI0Of5BS4YAAAAAAABAMEBAAAAAAEBa3aSSZB4qsumwCnFugiUBKjLRtTfhcGg49419lAMY80BAAAAAP////8C19xEAAAAAAAiUSBViObCV7ib3hvrxOQyA5KlNI0cgIwR0BSyLlW0jRclB7KPBgAAAAAAFgAUiv+GA89mInZrHtu+o1OiiP4LGksBQI0HvjLPjGlVI9FtO8ku3ehKj6WObPgnzZhN79zJNKBq5c0z34aNVaxhyg4eHg3ER4Ma+ogqDS2NmEUYbtzfN4gAAAAAAQEfso8GAAAAAAAWABSK/4YDz2Yidmse276jU6KI/gsaSwEDBAEAAAAiBgNchw+CoUkI5FdtT6GQpGGQvbIjcLTWYO2oaUkCp8A2uhjBPkswVAAAgAAAAIAAAACAAAAAAAAAAAAAACICA8OLuzBL8TC63N1IcPCBsrTwseEfv/4P9nsR7K4Y+anwGAAAAABUAACAAAAAgAAAAIABAAAAAgAAAAA=

Steps to reproduce

  • Import xpub from external account using ImportAccount endpoint. Make sure master fingerprint (XFP) is defined in the process.
  • Deposit funds to external account
  • Craft transaction using FundPsbt using input from the external account
  • Attempt to sign PSBT with Coldcard

Expected behaviour

Change output has defined XFP that matches the wallet.

Actual behaviour

Change output has null XFP

@kaloudis kaloudis added bug Unintended code behaviour needs triage labels Apr 6, 2024
guggero added a commit to guggero/lnd that referenced this issue Apr 8, 2024
Fixes lightningnetwork#8626.
This commit updates btcwallet to the latest version that correctly
includes the MasterKeyFingerprint for all generated addresses, including
change addresses derived from imported watch-only accounts.
@guggero
Copy link
Collaborator

guggero commented Apr 8, 2024

Thanks for the report. It looks like we indeed forgot to set the master fingerprint in some situations on newly derived addresses. Was able to reproduce and fix locally with #8630.
Can you confirm please?

@kaloudis
Copy link
Contributor Author

kaloudis commented Apr 8, 2024

The XFP (master fingerprint) is now being added correctly. Thanks Oli!

We have another issue now though. It seems that we're hitting a mismatch on the pubkey.

Pubkey 0303b42b33356ebae804c43c2910666d5b6a4c141e86f501d4a8d597a27a91b796 != the result of (m=C13E4B30)/84'/0'/0'/1/5

PSBT in question:

70736274ff01007102000000015f7c578981c72aebfea5cd39148a848be4b2bbe581187c39ed126f6ddd8f60fa0100000000000000000203d90000000000001600140ef9e657df381ddd8c71b3cbfa76b78d5368d48e22b6050000000000160014d0c7156f33486fc049854c1caeb446763fb67d7a00000000000100c1010000000001016b7692499078aacba6c029c5ba089404a8cb46d4df85c1a0e3de35f6500c63cd0100000000ffffffff02d7dc4400000000002251205588e6c257b89bde1bebc4e4320392a5348d1c808c11d014b22e55b48d172507b28f0600000000001600148aff8603cf6622766b1edbbea353a288fe0b1a4b01408d07be32cf8c695523d16d3bc92edde84a8fa58e6cf827cd984defdcc934a06ae5cd33df868d55ac61ca0e1e1e0dc447831afa882a0d2d8d9845186edcdf37880000000001011fb28f0600000000001600148aff8603cf6622766b1edbbea353a288fe0b1a4b010304010000002206035c870f82a14908e4576d4fa190a46190bdb22370b4d660eda8694902a7c036ba18c13e4b305400008000000080000000800000000000000000000022020303b42b33356ebae804c43c2910666d5b6a4c141e86f501d4a8d597a27a91b79618c13e4b30540000800000008000000080010000000500000000

@kaloudis
Copy link
Contributor Author

kaloudis commented Apr 8, 2024

It seems we also have an incorrect path for the input of 0h/0h/0/0 instead of 1h/0h/0/0 - testnet path not being taken into account here?

@guggero
Copy link
Collaborator

guggero commented Apr 9, 2024

Ah, I know what this is... Unfortunately, the btcwallet doesn't use the coin type for testnet/regtest/signet at all for the default address types (BIP-44, 49, 84, 86), only for the lnd internal keys (purpose 1017').
Can you test if this works correctly when you derive the watch-only account from m/84'/0'/0' directly? Or does the cold card then complain about wanting testnet?

@kaloudis
Copy link
Contributor Author

kaloudis commented Apr 9, 2024

Ah, I know what this is... Unfortunately, the btcwallet doesn't use the coin type for testnet/regtest/signet at all for the default address types (BIP-44, 49, 84, 86), only for the lnd internal keys (purpose 1017'). Can you test if this works correctly when you derive the watch-only account from m/84'/0'/0' directly? Or does the cold card then complain about wanting testnet?

I think it needs to be in testnet mode, otherwise the addresses are calculated incorrectly.

Seems like we need to fix the derivation path used when importing tpubs via the ImportAccount API? Derivation path still shows as m/84'/0'/0' with a tpub

I will try to do some testing on mainnet to see if the issue persists there

@kaloudis
Copy link
Contributor Author

Hitting the same issue when using a testnet seed with Seedsigner.

@guggero
Copy link
Collaborator

guggero commented Apr 12, 2024

Yeah, it probably won't work with any hardware wallet, since btcwallet internally uses the wrong derivation path for testnet. Did you try on mainnet, since you gave a tACK on the PR?
I don't think we can just fix the testnet derivation calculation in btcwallet without also breaking a lot of other things on testnet...

guggero added a commit to guggero/lnd that referenced this issue Apr 12, 2024
Fixes lightningnetwork#8626.
This commit updates btcwallet to the latest version that correctly
includes the MasterKeyFingerprint for all generated addresses, including
change addresses derived from imported watch-only accounts.
@kaloudis
Copy link
Contributor Author

kaloudis commented Apr 13, 2024

No problem on mainnet! I have tested it and was able to sign with the hardware wallet.

I created a separate issue to track the testnet derivation path issue: #8645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unintended code behaviour P1 MUST be fixed or reviewed psbt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants