-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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.
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
BIP-352: handle invalid privkey / pubkey sums for sending / scanning, add changelog #1620
BIP-352: handle invalid privkey / pubkey sums for sending / scanning, add changelog #1620
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, @theStack ! If you are able to add a test case, that would be great! If not, no worries, I can add one later this week.
I'm not sure if there are common rules, but I think it's good practice to update BIPs to clarify such cases. And then if the text is changed, then the Python code should also be changed. I don't want to make it too complicated, but it may be a good idea to include a changelog, like we did in BIP327: https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki#change-log (this one looks long because we started versioning even before the BIP was merged into the repo here...) |
51e7ffb
to
1ceb0b6
Compare
Thanks for the feedback so far!
Added a receiver test case with input data filled in from https://mempool.space/signet/tx/d73f4a19f3973e90af6df62e735bb7b31f3d5ab8e7e26e7950651b436d093313 and dummy data for
Agree, done. Note that with the secp256k1 module used here (it's the one Bitcoin Core used in the test framework before bitcoin/bitcoin#26222 was merged), there seems to be no obvious way to check if a public key corresponds to point at infinity, so it's checked indirectly if its serialization (
Seems reasonable to me, curious to get opinions from the BIP authors about the details (verbosity, concrete versioning used etc.), happy to pick up suggestions. |
nit: I don't think anyone will misinterpret this, but it's this is a tiny bit confusing because of the text just above the algorithm:
It may be a cleaner not to add the bullet point. The checks in the bullet points seem to be rather syntactical checks, so I think it's okay to omit this one.
Yep, we'll hopefully have something soon because we also use it in our draft for the DKG BIP, which we want to post to the mailing list in the next weeks. edit: I think it won't hurt to add an "if" and a test case also on the sender side. This can also be hit with specifically crafted secret keys, but it's not entirely crazy that someone else generates secret keys for you (e.g., in some special custodial protocol or secret keys printed on physical coins, etc..) |
1ceb0b6
to
6304666
Compare
Removed the bullet point as suggested, and added a corresponding part for the sender side ("if a = 0, fail"). Also added that check in the reference implementation, though that code part is currently unreachable, as the pubkey summing is done first in the course of the input hash calculation, which seems a bit odd/unintuitive (see PR #1622). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6304666 I haven't checked the test vector but the diff looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light LGTM ACK
$ ./reference.py send_and_receive_test_vectors.json
Simple send: two inputs
Simple send: two inputs, order reversed
Simple send: two inputs from the same transaction
Simple send: two inputs from the same transaction, order reversed
Outpoint ordering byte-lexicographically vs. vout-integer
Single recipient: multiple UTXOs from the same public key
Single recipient: taproot only inputs with even y-values
Single recipient: taproot only with mixed even/odd y-values
Single recipient: taproot input with even y-value and non-taproot input
Single recipient: taproot input with odd y-value and non-taproot input
Multiple outputs: multiple outputs, same recipient
Multiple outputs: multiple outputs, multiple recipients
Receiving with labels: label with even parity
Receiving with labels: label with odd parity
Receiving with labels: large label integer
Multiple outputs with labels: un-labeled and labeled address; same recipient
Multiple outputs with labels: multiple outputs for labeled address; same recipient
Multiple outputs with labels: un-labeled, labeled, and multiple outputs for labeled address; same recipients
Single recipient: use silent payments for sender change
Single recipient: taproot input with NUMS point
Pubkey extraction from malleated p2pkh
P2PKH and P2WPKH Uncompressed Keys are skipped
Skip invalid P2SH inputs
Recipient ignores unrelated outputs
No valid inputs, sender generates no outputs
Input pubkeys sum up to the point at infinity, receiver skips tx
All tests passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Since we are now checking for a = 0
, I think its worth adding a test case for the sender side, as well.
Regarding the changelog, I think this is a great suggestion and especially helpful when adding new test cases. I'd suggest adding one here and just reusing the semantic versioning system from BIP327.
(I realize this is a bit out of scope for this change, but I didn't see a way to open an issue on the BIPs repo for this discussion)
More generally, seems worth it to formalizing the changelog format and versioning scheme as part of BIP2 (or whichever process BIP makes sense), e.g. something like "BIP should include a change log. If a change log is added it must use MAJOR.MINOR.PATH
versioning scheme, where MAJOR
is.. " etc.
I see a few BIPs have change logs in various formats, which got me thinking it would be nice to formalize it for consistency and also to avoid each BIP needing to either a) copy the text from another BIP explaining the change log or b) re-invent their own versioning scheme each time.
if not a_sum.valid: | ||
# Input privkeys sum is zero -> fail | ||
return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we can't reuse the private keys corresponding to the public keys in the recipient test case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. However, I just noticed that my script for generating the example "pubkeys-sum-up-to-point-at-infinity" tx didn't print the private keys, so I don't have them anymore 😅 🤦♂️ will fix that and run it again to re-create the receiver test-case, and a sender one that matches. Shouldn't take too long hopefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. However, I just noticed that my script for generating the example "pubkeys-sum-up-to-point-at-infinity" tx didn't print the private keys, so I don't have them anymore 😅 🤦♂️ will fix that and run it again to re-create the receiver test-case, and a sender one that matches. Shouldn't take too long hopefully.
Was this done, e.g. with "data corresponds to the newly generated tx fe788cf6578d547819def43d79e6c8f0153d4885f5a343d12bd03f34507aabd6)" in #1620 (comment)?
(edit, it seems so yes, but feel free to confirm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. However, I just noticed that my script for generating the example "pubkeys-sum-up-to-point-at-infinity" tx didn't print the private keys, so I don't have them anymore 😅 🤦♂️ will fix that and run it again to re-create the receiver test-case, and a sender one that matches. Shouldn't take too long hopefully.
Was this done, e.g. with "data corresponds to the newly generated tx fe788cf6578d547819def43d79e6c8f0153d4885f5a343d12bd03f34507aabd6)" in #1620 (comment)?
(edit, it seems so yes, but feel free to confirm)
Yes, confirming (sorry for the late reply, missed that comment a few days ago).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theStack thanks!
@josibake 👍 on a changelog, like in BIPS 1, 327, and 340, for instance. |
@josibake: Makes sense. Will rebase (necessary after #1622, thanks for the quick review and merge!), add a sending test-case and the changelog using semantic versioning system in a bit. Simple suggestion (assuming that we are not interested in the pre-merge history for now):
|
LGTM! I don't think the pre-merge history is super relevant/useful here. Thanks for picking this up! |
The test vector data was generated with a Python script (see https://github.com/theStack/bitcoin/blob/bc15ea8d0f282908b912dbf62bba816ecd82424d/contrib/silentpayments/submit_input_pubkeys_infinity_tx.py), leading to the following output: --------------------------------------------------------------------------------------------------------- Privkey 1: a6df6a0bb448992a301df4258e06a89fe7cf7146f59ac3bd5ff26083acb22ceb Privkey 2: 592095f44bb766d5cfe20bda71f9575ed2df6b9fb9addc7e5fdffe0923841456 Pubkey 1: 02557ef3e55b0a52489b4454c1169e06bdea43687a69c1f190eb50781644ab6975 Pubkey 2: 03557ef3e55b0a52489b4454c1169e06bdea43687a69c1f190eb50781644ab6975 scriptPubKey 1: 00149d9e24f9fab4e35bf1a6df4b46cb533296ac0792 scriptPubKey 2: 00149860538b5575962776ed0814ae222c7d60c72d7b Address 1: tb1qnk0zf706kn34hudxma95dj6nx2t2cpujz7j5t5 Address 2: tb1qnps98z64wktzwahdpq22ug3v04svwttm7gs8wn -> Funding tx submitted: 3a286147b25e16ae80aff406f2673c6e565418c40f45c071245cdebc8a94174e Taproot output address for spending tx: tb1pqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqkgkkf5 -> Spending tx submitted: fe788cf6578d547819def43d79e6c8f0153d4885f5a343d12bd03f34507aabd6 ---------------------------------------------------------------------------------------------------------
…t at infinity The input data for the test vector is taken from the signet transaction fe788cf6578d547819def43d79e6c8f0153d4885f5a343d12bd03f34507aabd6 which spends two P2WPKH inputs with negated pubkeys (x, y) and (x, -y) from the funding transaction 3a286147b25e16ae80aff406f2673c6e565418c40f45c071245cdebc8a94174e (see also bitcoin-core/secp256k1#1519 (comment) and the output from the script in the previous commit message). Co-authored-by: josibake <[email protected]>
3316418
to
4c0c3d7
Compare
Done. The commits are now divided up into sending and scanning parts (with test vectors directly included; data corresponds to the newly generated tx fe788cf6578d547819def43d79e6c8f0153d4885f5a343d12bd03f34507aabd6), and a changelog addition mostly copied from BIP327 (without the sentence about MAJOR version zero). Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 4c0c3d7
Test cases look good, left a nit regarding change log ordering, not blocking.
bip-0352.mediawiki
Outdated
The <code>MINOR</code> version is incremented whenever the inputs or the output of an algorithm changes in a backward-compatible way or new backward-compatible functionality is added. | ||
The <code>PATCH</code> version is incremented for other changes that are noteworthy (bug fixes, test vectors, important clarifications, etc.). | ||
|
||
* '''1.0.0''' (2024-05-08): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: In my experience, change logs usually start with the latest version first (which is also how the BIP327 change log is structured). Aside from consistency, I also prefer the latest first because as implementors most people are interested in the most recent changes and not necessarily interest in the chronology of changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I agree! Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change log LGTM. Verified the dates are when the change was merged.
In another PR, it may (or may not) be worthwhile to update BIP 2 with the change log format description, update the other change logs to it, and link to that BIP 2 section in each BIP rather than duplicating it.
The first paragraph is taken from BIP-327, with the sentence about MAJOR version zero removed, as it's not relevant here (we don't track the pre-merge history).
4c0c3d7
to
496e429
Compare
ACK 496e429, only change since BIP author approval in #1620 (review) was taking feedback on change log order. |
See the discussion in the corresponding secp256k1 PR (bitcoin-core/secp256k1#1519 (comment), bitcoin-core/secp256k1#1519 (comment), bitcoin-core/secp256k1#1519 (comment)), where it was recommended to include this corner-case in the BIP by real-or-random:
Note that this PR right now only includes the absolute minimum change; happy to include further changes if demanded (reflect this in the Python implementation, add test case), but not sure what are common rules for BIP amendments and if additional changes are worth it for a corner-case. For an additional test vector, one could take the example tx that has been published on signet: d73f4a19f3973e90af6df62e735bb7b31f3d5ab8e7e26e7950651b436d093313