BIP-0322: clarify motivation/purpose, add prefix, clarify Proof of Funds format, describe PSBT based signing#2141
Conversation
jonatack
added
Proposed BIP modification
|
Thank you @guggero for picking this up.
@kallewoof thoughts? (If helpful, see BIP3 here for details about the Deputy role.) |
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Partial review of the first three commits.
Sounds good to me! |
guggero
force-pushed
the
bip-0322-finalization
branch
2 times, most recently
from
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 |
guggero
force-pushed
the
bip-0322-finalization
branch
from
d9d4bf3 to
1ee1a21
Compare
murchandamus
left a comment
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
murchandamus
left a comment
There was a problem hiding this comment.
Oh one more observation:
murchandamus
added
PR Author action required
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.
guggero
force-pushed
the
bip-0322-finalization
branch
2 times, most recently
from
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 |
|
Okay great, let’s revisit in a week or so, on April 28th or later. |
guggero
force-pushed
the
bip-0322-finalization
branch
from
518dc68 to
a1f4350
Compare
|
I've updated the PR to define a new IMO this is good to go now, though more review would still be nice. |
|
Thanks for the update. I’m putting this on my queue to read again. |
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
ACK. Looks good to me. If you are not currently expecting more review and are finished with your planned work, I would suggest that we publish this update whenever you greenlight that.
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.
guggero
force-pushed
the
bip-0322-finalization
branch
from
a1f4350 to
40dc3e1
Compare
Thank you for the re-review! Sounds good to me. I don't have any more work planned and all current review comments are addressed. I'm not aware of any in-progress review. |
I've been following the discussion even if I haven't commented. And I personally don't think I have any rights to any of this at this point, seeing as I failed to get this pushed through to adoption myself. I will comment if something looks off to me, of course, but silence is implicit approval. |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
It's good that you addressed backward compatibility, as IIUC software already implementing the draft version of BIP322 could break when encountering the new 1-byte prefix. I don't know the current state of existing implementations.
A couple nits follow; they along with a minor grammar round-up could be done in a follow-up.
(If @kallewoof agrees, I think it would make sense @guggero for you to be co-author of BIP322 for your work.)
What is the easiest way for implementers to run the test vectors? By running btcutil/bip322/test_vectors_test.go in btcsuite/btcd#2521?
| resembles a Bitcoin transaction (except that it contains an invalid input, so it cannot be spent on | ||
| any real network). | ||
|
|
||
| The Proof of Funds variant allows demonstrating control of a set of UTXOs. |
There was a problem hiding this comment.
As this is the first mention of the Proof of Funds variant, perhaps introduce it in some way, and/or link to where it is defined below for more details.
There was a problem hiding this comment.
Slightly adjusted the wording and linked to the section that explains the variant (in the follow-up PR).
| |} | ||
|
|
||
| <sup>1</sup>: Possible on a technical level but should NOT be used anymore in the context of this BIP.<br/> | ||
| <sup>1</sup>: Possible on a technical level but should NOT be used anymore in the context of this |
There was a problem hiding this comment.
nit, might be helpful to briefly describe why
There was a problem hiding this comment.
Hmm, to be honest, I'm not 100% sure why this was defined the way it is in the Legacy section, which was already present before my changes. So I just linked to that section to make the footnote more clear in that it refers to that rule.
Sounds good to me. |
|
@jonatack thanks for the review! I've created the follow-up PR here: #2155
Anything specific you noticed or should I just do a general grammar pass?
Yes, checking out and running |
Thanks, will test!
I'll comment on your new PR #2155 with a link to a diff. |
| ** Mark Complete | ||
| ** Breaking change: Add human-readable prefix to encoded signature | ||
| ** Breaking change: format of "Proof of Funds" signatures to be base64-encoded finalized PSBT | ||
| ** Add new PSBT input field for PSBT-based signing |
There was a problem hiding this comment.
| ** Add new PSBT input field for PSBT-based signing | |
| ** Add new PSBT global field for PSBT-based signing |
Noting here that this entry wasn't updated with the change during PR discussion to PSBT_GLOBAL_GENERIC_SIGNED_MESSAGE; fixed in #2162.
6 participants
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!