Skip to content

Comments

Read response verification#15

Merged
notmandatory merged 2 commits intobitcoindevkit:masterfrom
brh28:read-verification
Jun 9, 2023
Merged

Read response verification#15
notmandatory merged 2 commits intobitcoindevkit:masterfrom
brh28:read-verification

Conversation

@brh28
Copy link
Collaborator

@brh28 brh28 commented Jun 6, 2023

Description

Validate the signature of the ReadResponse against the signature as described in #6. For Satscard, public key is for active slot. For Tapsigner, the "pubkey" provided in the response must be unzipped (XORed) to get the public key for the current derivation path.

Notes to the reviewers

  • Rather than changing the calc_ekeys_xcvc interface, the session_key is being recalculated outside.
  • I created a standalone unzip function that might be better off as a trait implement by a Vec pubkey, so there may be a follow up commit time-permitting.
  • I added a debug command to the cli for developer convenience, but can remove if needed

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK bc7889b

I tested and confirmed read response verification works for each card type. I also did a little side test and was able to convert the read response pubkey to a wpkh bitcoin address that matched what the SatsCard status addr field said to expect.

@notmandatory
Copy link
Member

I'm going to open an issue to discuss what sort of commands we should have for the cli vs the lib so we can decide which logic goes where.

@notmandatory notmandatory merged commit bc7889b into bitcoindevkit:master Jun 9, 2023
@notmandatory
Copy link
Member

I opened issue #16 and #17 to discussion lib and cli commands.

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