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

hsmd: Added fields to hsm_sign_remote_commitment_tx to allow complete validation. #3363

Merged

Conversation

ksedgwic
Copy link
Collaborator

Added output_witscripts, remote_per_commit and option_static_remotekey to hsm_sign_remote_commitment_tx to allow complete validation.

@cdecker
Copy link
Member

cdecker commented Dec 24, 2019

@bitcoin-bot was offline for a couple of hours, he should be back now :-)

bitcoin/tx.c Show resolved Hide resolved
bitcoin/tx.h Show resolved Hide resolved
channeld/channeld.c Show resolved Hide resolved
channeld/commit_tx.c Show resolved Hide resolved
@cdecker
Copy link
Member

cdecker commented Jan 6, 2020

Some minor things that could be simplified, but overall I'm all for giving the hsmd the information it needs to verify independently 👍

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Only minor changes suggested, this is really good work!

(Sorry for the delayed feedback, I was on vacation and am now catching up!).

Orthogonal to this, I wonder if we should bite the bullet and use struct witscript everywhere for type-safety. That's a bigger change though, and may involve changing it to a danlging u8 rather than a pointer, to avoid adding allocations.

wire/fromwire.c Show resolved Hide resolved
wire/towire.c Show resolved Hide resolved
@ZmnSCPxj
Copy link
Collaborator

👍 type safety

@cdecker
Copy link
Member

cdecker commented Feb 3, 2020

I think we need a minor cleanup, but otherwise LGTM. 8c593e2 and c1e1634 need to be squashed into 5a47254. Otherwise we can squash on merge.

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.

4 participants