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

fix: get input data based on address type #74

Closed

Conversation

bucko13
Copy link
Contributor

@bucko13 bucko13 commented Mar 2, 2023

Some eslint fixes got caught in this but basically the input data for adding to a psbt needs to be a bit smarter about how it gets the input data for ledger v2 which is pickier about when it requires nonWitnessUtxo vs witnessUtxo data. (See here from the bip174 docs).

Unfortunately while this creates a valid PSBT that ledger will sign, the signatures created after getting converted to psbtv2 and getting passed to the ledger are still wrong so there's still some tweaking left to do.

Can test with: unchained-capital/caravan#294

Comment on lines +210 to +218
const addressType = multisigAddressType(input.multisig);

// The PSBT class in bitcoinjs-lib won't accept both nonWitnessUxo and witnessUtxo
// but ledger v2 needs nonWitnessUtxo data in the PSBT for any transactions that aren't
// native segwit. So we need to pick which one to give based on the address type
// https://github.com/bitcoinjs/bitcoinjs-lib/issues/1595
const utxoToVerify =
addressType === P2WSH ? { witnessUtxo } : { nonWitnessUtxo };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is basically the only relevant change

@Shadouts Shadouts self-requested a review March 2, 2023 18:37
Copy link
Contributor

@Shadouts Shadouts left a comment

Choose a reason for hiding this comment

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

I know we talked about this out-of-band, but there are still some signature test failures on this which is to be expected since the psbt contents are changing.

@Shadouts
Copy link
Contributor

Shadouts commented Mar 2, 2023

I went to go see about the sigs myself, but I'm still getting invalid data error 0x6a80

@bucko13
Copy link
Contributor Author

bucko13 commented Mar 2, 2023

@Shadouts thats weird…

@bucko13
Copy link
Contributor Author

bucko13 commented Mar 6, 2023

So, the correct behavior for this is that if it's P2SH-P2WSH then BOTH nonWitnessUtxo and witnessUtxo should be included in the PSBT. Unfortunately bitcoinjs-lib doesn't allow you to do this though. I've filed a bug. Hopefully it'll be addressed and we can fix the input serialization in the correct way such that signatures are created correctly. Without this change, ledger will fail b/c it has incorrect data. With this change, ledger will succeed but will produce incorrect signatures so it's not quite ready for primetime.

@bucko13
Copy link
Contributor Author

bucko13 commented Sep 8, 2023

Not required anymore for now. Closing.

@bucko13 bucko13 closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants