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

PSBTs, Meet the HSM #3762

Merged
merged 13 commits into from
Jun 23, 2020
Merged

Conversation

niftynei
Copy link
Contributor

@niftynei niftynei commented Jun 10, 2020

Updates the HSM withdrawal to take a PSBT and utxo set. We sign the inputs that we've got utxos for, and return a psbt with the attached signatures.

This 'fully' integrates the PSBTs with our HSM for the first time, and gets us ready for a signpsbt command (coming soon).

Note that this is built atop #3740
(new content begins at
965014d), and requires ElementsProject/libwally-core#203 to be merged in order to pass tests (as there's a number of P2SH-P2WPKH bugs in the existing libwally-psbt implementation)

Changelog-None

@niftynei niftynei requested a review from cdecker as a code owner June 10, 2020 00:05
@niftynei niftynei force-pushed the nifty/psbt-hsm-withdraw branch from 0ffd4fb to 984d4a6 Compare June 12, 2020 20:57
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Travis doesn't really seem to be happy with this PR, other than that I have only minor comments.

bitcoin/tx.c Outdated Show resolved Hide resolved
bitcoin/psbt.c Outdated Show resolved Hide resolved
bitcoin/psbt.h Outdated Show resolved Hide resolved
bitcoin/psbt.h Outdated Show resolved Hide resolved
hsmd/hsmd.c Outdated Show resolved Hide resolved
hsmd/hsmd.c Show resolved Hide resolved
@niftynei
Copy link
Contributor Author

Travis doesn't really seem to be happy with this PR, other than that I have only minor comments.

Travis is unhappy because we require ElementsProject/libwally-core#203 to be merged -- there's a number of P2SH-P2WPKH bugs in the existing libwally-psbt implementation, that we now hit.

@niftynei niftynei mentioned this pull request Jun 16, 2020
@niftynei niftynei force-pushed the nifty/psbt-hsm-withdraw branch from 984d4a6 to 54f513e Compare June 17, 2020 21:28
niftynei added 11 commits June 22, 2020 12:40
Re-uses code from what was the bitcoin_tx_to_psbt_b64
This lets us parse invalid/bad psbt data from user input without
crashing
We're not using the change_outnum for withdraw tx's (and the way
we were calculating it was broken as of the addition of 'multiple
outputs'). This removes the change output knowhow from withdraw_tx
entirely, and pushes the responsibility up to the caller to
include the change output in the output set if desired.

Consequently, we also remove the change output knowhow from hsmd.
PSBT's dont' serialize / unserialize if there's any sig info set on the
global transaction
instead return a boolean indicating the success/failure of a sig set
will either use a temporary psbt (and not munge the passed in psbt)
or will finalize in place -- finalization erases most of the signature
metadata from the psbt struct
our code generators expect the serialization name to match the struct
type
this will allow us to add inputs that aren't ours to a tx that we sign
and finalize
@niftynei niftynei force-pushed the nifty/psbt-hsm-withdraw branch from 54f513e to 4513600 Compare June 22, 2020 18:08
@niftynei niftynei force-pushed the nifty/psbt-hsm-withdraw branch from 4513600 to 37811fd Compare June 22, 2020 18:29
@cdecker
Copy link
Member

cdecker commented Jun 23, 2020

Reviewed based on this range-diff

ACK 37811fd

@cdecker cdecker merged commit a66415a into ElementsProject:master Jun 23, 2020
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