BIP-0322: clarify motivation/purpose, add prefix, clarify Proof of Funds format, describe PSBT based signing#2141
BIP-0322: clarify motivation/purpose, add prefix, clarify Proof of Funds format, describe PSBT based signing#2141guggero wants to merge 13 commits intobitcoin:masterfrom
Conversation
|
Thank you @guggero for picking this up.
@kallewoof thoughts? (If helpful, see BIP3 here for details about the Deputy role.) |
murchandamus
left a comment
There was a problem hiding this comment.
Partial review of the first three commits.
Sounds good to me! |
f3ffd34 to
7809e72
Compare
|
I updated the prefixes to be fixed-size ( @murchandamus I don't really understand the failing CI check... Am I supposed to change the background color of the row in the README? |
|
I've implemented the proposed changes (prefix, Proof of Funds format) in the |
d9d4bf3 to
1ee1a21
Compare
murchandamus
left a comment
There was a problem hiding this comment.
Finished my first pass. I think it’s great that you want to pick up this proposal and bring it up to speed. Strong concept ACK. You also seem to have buy-in from @kallewoof, which is great.
Given the age of the proposal and the breaking change, I would heartily recommend that you write to the mailing list about the changes you are proposing in this PR.
murchandamus
left a comment
There was a problem hiding this comment.
Oh one more observation:
This re-formats the document for easier editing and diff viewing. Wiki syntax is weird for lists and line wraps break them. Simple lists were changed to <ul> or <ol> tags but complex lists remain as they are to not bloat the diff too much.
This fixes small inconsistencies or incomplete definitions based on previous, already merged changes.
This addresses two discussion items: - The list of UTXOs should not be interpreted as a "proof that no other UTXOs for an address exist". - The BIP only addresses "signer receives funds with the address" and not "signer sent a previous transaction" use case.
Since the term "signature" can be pretty overloaded dependin on the context, we clarify what it actually means in this BIP.
This commit proposes a fix for the problem that an offline verifier previously was not able to even verify the witness stack of additional inputs. By providing the full finalized PSBT, a verifier has all the input data necessary to run the script through the validation engine. We require the PSBT to be finalized to make sure it contains the final script witness or final script sig but no extra potentially privacy-sensitive fields. The Non-Witness and Witness UTXO fields are explicitly allowed for finalized PSBTs, which makes the format perfect for the use case.
This commit proposes a new PSBT input field type for transporting the message to be signed to different signers in a multisig signing use case.
This commit updates the test vectors to reflect all the changes in the previous commits and also introduces new test vectors for the Proof of Funds variant.
As described in BIP-0003, the comments section is no longer required. Instead we add all relevant discussions.
81b5a1c to
78c96cf
Compare
78c96cf to
3300b28
Compare
|
Thank you all for your inputs and reviews so far! As you might already have received in your respective inboxes, my message to the mailing list has been published: https://groups.google.com/g/bitcoindev/c/qd6BNz9gxCk |
jonatack
left a comment
There was a problem hiding this comment.
Given the work here, would it make sense for @guggero to be an author? @kallewoof
murchandamus
left a comment
There was a problem hiding this comment.
I gave the document another full read, it looks good to me. I left one more suggestion.
There was a problem hiding this comment.
I went over the commits one more time, LGTM. It might be good to have a PSBT expert take a look. Are you otherwise still waiting for review or is this ready to go from your perspective?
Edit: Ava Chow took a look and stated "seems fine" above in #2141 (comment).
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
I didn't consult with @kallewoof about my suggested changes before opening the PR, as I wanted to show a complete picture before starting any discussions. And I also want to give the mailing list thread a bit of time for discussions to unfold. There already is a proposal for a change to move the PSBT field from an input field to a global field. Detailed review is definitely welcome but a merge would be a bit premature at this point IMO. |
This comment was marked as low quality.
This comment was marked as low quality.
|
Okay great, let’s revisit in a week or so, on April 28th or later. |
This PR is an attempt to push this PR forward, hopefully closer to a
Completestate.If it helps the process and @kallewoof agrees, I'm volunteering to be named as a deputy for this BIP.
The PR addresses the following discussion items:
TODO (will follow up with these items soon):
btcdpull request to reflect above changes, update test vectors in this PR (prefix and Proof of Funds implementation)@murchandamus I went through the different discussions you linked and also both Bitcoin Core PRs and tried to extract all discussion items that I felt were not yet addressed.
If anyone feels like a previous discussion item is missing here and not yet addressed by my changes, please comment below!