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.

Sign up for GitHub

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

Jump to bottom

rpcwallet: fix remote signing issues #7130

Merged
merged 6 commits into from
Nov 12, 2022
Merged

rpcwallet: fix remote signing issues #7130

merged 6 commits into from
Nov 12, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Nov 8, 2022 •
edited
Loading

Fixes #7024.
Fixes #7022.

yyforyongyu
yyforyongyu approved these changes Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM👍 Thanks for the quick fix! Just FYI the new itest failed due to flakes and is unrelated to this PR. The linter issue should be gone once the btcd is in. And maybe you could rebase on #7126 to see how the old itest goes?

keychain/btcwallet.go Show resolved Hide resolved
@Roasbeef Roasbeef modified the milestone: v0.15.5 Nov 9, 2022
@guggero guggero added this to the v0.15.5 milestone Nov 9, 2022
@guggero guggero mentioned this pull request Nov 9, 2022
Closed
@guggero
Copy link
Collaborator Author

guggero commented Nov 9, 2022

Pushed up an updated version that uses the latest dependencies. The itest won't be green before #7126 is merged but I rand the remote signing itests locally to confirm the targeted fixes are correct.

@guggero guggero requested a review from Roasbeef November 9, 2022 23:28
@guggero
Copy link
Collaborator Author

guggero commented Nov 10, 2022

I tacked on a commit (29d86a51) that fixes another Taproot related issue in a remote signing setup (related to Pool account deposits).

@morehouse
Copy link
Collaborator

Fixes #7022.

Maybe this fixes the original cause of #7022, but it seems we should leave that issue open to track work on double-checking change outputs. Better safe than sorry, and it would keep users funds safe if we introduce a bug like this again.

@Roasbeef
Copy link
Member

Better safe than sorry, and it would keep users funds safe if we introduce a bug like this again.

Yeah there're def some subtle interactive here, it took quite a bit to get to the bottom of all this.

@morehouse have you seen: btcsuite/btcwallet#825? This PR updates the btcwallet dep that includes that fix. I have another idea re also storing the serialized address, then decoding that before returning the addr to the caller to check if the checksum fails or not. Then txscript.PayToAddrScript can call this extra method or validate again there. I think this is about the best we can do in that situation.

@Roasbeef
Copy link
Member

Ok merged the itest PR, can you rebase this now @guggero, then the new set of itests can run on top of it.

@guggero
Copy link
Collaborator Author

guggero commented Nov 11, 2022

Fixes #7022.

Maybe this fixes the original cause of #7022, but it seems we should leave that issue open to track work on double-checking change outputs. Better safe than sorry, and it would keep users funds safe if we introduce a bug like this again.

@morehouse I'm not sure what you mean. This PR includes a bump in the btcwallet version that includes btcsuite/btcwallet#825. Which is what you mean if I understand you correctly.

@guggero
mod+keychain+lntest: bump btcd to version with tweak fix
e3536dd 
This commit bumps to the latest version of btcd that fixes a key
mutation issue when signing for Taproot outputs.
@guggero
mod: bump btcwallet to version with addr validation
082d68d 
This commit bumps the btcwallet dependency to the version that includes
the address validation that asserts we can sign for an address before we
use it.
@guggero
itest: add test coverage for Taproot tweak key mutation
b3f6fe3
@guggero
rpcwallet: don't re-use sign desc in loop
ed2e542 
This commit fixes an issue with signing for mixed inputs in a remote
signing setup where the re-use of the sign descriptor would lead to the
sign method not being reset correctly if a p2wkh input is following a
p2tr input.
@guggero
rpcwallet: pass prev output fetcher into ComputeInputScript
5bd3e91 
This commit fixes signing of Taproot inputs when some of the other
inputs of the transaction are not known to the wallet (such as a Pool
account for example).
If we want to sign for a Taproot (change) input when depositing into a
Pool account the wallet won't know the Pool account input. So we need to
make sure we pass it along inside the PrevOutputFetcher (which contains
the UTXO information extracted from the PSBT).
@guggero
docs: add release notes for v0.15.5-beta
8b522aa
@morehouse
Copy link
Collaborator

@morehouse have you seen: btcsuite/btcwallet#825? This PR updates the btcwallet dep that includes that fix.

Ack, thanks for explaining.

@morehouse I'm not sure what you mean. This PR includes a bump in the btcwallet version that includes btcsuite/btcwallet#825. Which is what you mean if I understand you correctly.

Yep, sorry I missed the fix over there.

@Roasbeef Roasbeef merged commit f6ae63f into lightningnetwork:master Nov 12, 2022
@guggero guggero deleted the remote-signing-fixes branch November 14, 2022 08:35
@guggero guggero mentioned this pull request Nov 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yyforyongyu yyforyongyu yyforyongyu approved these changes

@Roasbeef Roasbeef Awaiting requested review from Roasbeef

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.15.5
4 participants
@guggero @morehouse @Roasbeef @yyforyongyu