8000 Bug: Change type of pbst partial sig from secp key to bitcoin key by sanket1729 · Pull Request #836 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
Dismiss alert

Bug: Change type of pbst partial sig from secp key to bitcoin key #836

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
Merged
merged 2 commits into from
Feb 17, 2022
Merged

Bug: Change type of pbst partial sig from secp key to bitcoin key #836

merged 2 commits into from
Feb 17, 2022

Conversation

sanket1729
Copy link
Member
@sanket1729 sanket1729 commented Feb 17, 2022 •
edited by RCasatta
Loading

This changes the type of secp signature from secp256k1::PublicKey to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.

@sanket1729
Bug: Change type of pbst partial sig from secp key to bitcoin key
69c6eb6 
This changes the type of secp signature from secp256k1::Signature to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in rust-bitcoin#591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
@sanket1729 sanket1729 added the bug label Feb 17, 2022
@sanket1729 sanket1729 added this to the 0.28.0 milestone Feb 17, 2022
@tcharding
Copy link
Member

FWIW looks good to me.

@Kixunil
Copy link
Collaborator
Kixunil commented Feb 17, 2022

Perhaps add a test?

@sanket1729
Copy link
Member Author

Added a breaking test. Cross-tested with bitcoin core. Commits can be re-ordered to see that the test fails during decoding the psbt.

b-cli decodepsbt cHNidP8BADMCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wD/////AAAAAAAAQgIEuw1dDMo257nID2O8BMEkC6u4O80oA+96yLbir1lCkdrsKB6FbJjSEMWrFN/Vgodh+O59X0XKIa0+TEtBt0ejoEcwRAIgT2fir7dhQtRPrliiSV0zo0GdqibNDbjQTzRStjKJrA8CIBB2Kp+2fpTMXK2QJvbcmf9/Bw9CeNMPvH0Mhp3TjH/nAQA=
{
  "tx": {
    "txid": "1ae127a5e2012b2f57080ad045de8a91616111d0a31ccc5a78fc056348a59f48",
    "hash": "1ae127a5e2012b2f57080ad045de8a91616111d0a31ccc5a78fc056348a59f48",
    "version": 2,
    "size": 51,
    "vsize": 51,
    "weight": 204,
    "locktime": 0,
    "vin": [
      {
        "coinbase": "",
        "sequence": 4294967295
      }
    ],
    "vout": [
    ]
  },
  "global_xpubs": [
  ],
  "psbt_version": 0,
  "proprietary": [
  ],
  "unknown": {
  },
  "inputs": [
    {
      "partial_signatures": {
        "04bb0d5d0cca36e7b9c80f63bc04c1240babb83bcd2803ef7ac8b6e2af594291daec281e856c98d210c5ab14dfd5828761f8ee7d5f45ca21ad3e4c4b41b747a3a0": "304402204f67e2afb76142d44fae58a2495d33a3419daa26cd0db8d04f3452b63289ac0f022010762a9fb67e94cc5cad9026f6dc99ff7f070f4278d30fbc7d0c869dd38c7fe701"
      }
    }
  ],
  "outputs": [
  ]
}

@sanket1729
Add a breaking test
4e19973 
This commit can be re-ordered before the fix to see that the test fail
during psbt decoding
@sanket1729 sanket1729 force-pushed the psbt_partial_sig_update branch from 026b1fe to 4e19973 Compare February 17, 2022 10:49
Kixunil
Kixunil approved these changes Feb 17, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 4e19973

apoelstra
apoelstra approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 4e19973

@apoelstra apoelstra merged commit 61f20b7 into rust-bitcoin:master Feb 17, 2022
@dr-orlovsky
Copy link
Collaborator

Post-merge nACK: the issue is not solved. Please refer to bitcoin/bips#1100

In practice, non-compressed keys in PSBTs are not used, non-BIP32 compliant and should be prohibited at the spec level.

@apoelstra
Copy link
Member

In the linked issue the PSBT maintainer disagrees with your stance. I also disagree. The point of PSBT is to enable wallet interoperability. I don't understand the benefit of banning legal key formats, when the cost is entirely localized to one field, whose type just needs to be changed from one keytype to another.

@sanket1729
Copy link
Member Author
sanket1729 commented Feb 17, 2022 via email

@sanket1729
Copy link
Member Author

@dr-orlovsky: Let me elaborate a bit: Pbsts have two fields that deal wth key.

  1. PSBT_IN_BIP32_DERIVATION: This still has a mapping that uses secp keys. There is nothing in conflict with PSBT: Require compressed keys for BIP32 key fields bitcoin/bips#1100
  2. PSBT_IN_PARTIAL_SIG: This is the field that is changed in the PR. This has a mapping from bitcoin::PublicKeys to partial signatures. This should support using uncompressed keys.

@dr-orlovsky
Copy link
Collaborator

Oh, I see now, sorry. Still slowed down after last weeks cold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
5 participants
@sanket1729 @tcharding @Kixunil @dr-orlovsky @apoelstra
0