Skip to content

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

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
sanket1729:psbt_partial_sig_update
Feb 17, 2022
Merged

Bug: Change type of pbst partial sig from secp key to bitcoin key#836
apoelstra merged 2 commits intorust-bitcoin:masterfrom
sanket1729:psbt_partial_sig_update

Conversation

@sanket1729
Copy link
Copy Markdown
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
Copy Markdown
Member

tcharding commented Feb 17, 2022

FWIW looks good to me.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Feb 17, 2022

Perhaps add a test?

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Feb 17, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

dr-orlovsky commented Feb 17, 2022

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
Copy Markdown
Member

apoelstra commented Feb 17, 2022

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
Copy Markdown
Member Author

sanket1729 commented Feb 17, 2022 via email

@sanket1729
Copy link
Copy Markdown
Member Author

sanket1729 commented Feb 17, 2022

@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
Copy Markdown
Collaborator

dr-orlovsky commented Feb 19, 2022

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

This was referenced Feb 24, 2026
Closed
Closed
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