BIP-352: handle invalid privkey / pubkey sums for sending / scanning, add changelog

[theStack]

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:

Indeed, this should be in the BIP, ideally even in the pseudocode. If our code starts to reject "payments", then better be authorized by the standard.

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

[theStack]

cc @josibake @real-or-random

[josibake]

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.

[real-or-random]

but not sure what are common rules for BIP amendments and if additional changes are worth it for a corner-case.

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...)

[theStack]

Thanks for the feedback so far!

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.

Added a receiver test case with input data filled in from https://mempool.space/signet/tx/d73f4a19f3973e90af6df62e735bb7b31f3d5ab8e7e26e7950651b436d093313 and dummy data for key_material and address as those fields are not relevant for the case.

but not sure what are common rules for BIP amendments and if additional changes are worth it for a corner-case.

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.

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 (.get_bytes()) returns None. I thought that it's maybe worth it to update the secp256k1 implementation to one that follows best practices and has a nicer API (I think you were working on such a Python module, that'd probably be a good choice?), but that's of course outside of the scope of this PR.

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...)

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.

[real-or-random]

and then also add a bullet point to the section above, "Scanning silent payment eligible transactions"

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:

If each of the checks in ''[[#scanning-silent-payment-eligible-transactions|Scanning silent payment eligible transactions]]'' passes, the receiving wallet must:

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.

I thought that it's maybe worth it to update the secp256k1 implementation to one that follows best practices and has a nicer API (I think you were working on such a Python module, that'd probably be a good choice?), but that's of course outside of the scope of this PR.

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..)

[theStack]

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).

[real-or-random]

ACK 6304666 I haven't checked the test vector but the diff looks good

[jonatack]

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

[josibake]

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.

[josibake]

Any reason we can't reuse the private keys corresponding to the public keys in the recipient test case here?

[theStack]

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.

[jonatack]

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)

[theStack]

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).

[jonatack]

@theStack thanks!

[jonatack]

@josibake 👍 on a changelog, like in BIPS 1, 327, and 340, for instance.

[theStack]

@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):

• 1.0.0 (2024-05-08): Initial version, merged as BIP-352
• 1.0.1 (2024-06-xx): Add steps to fail if private key sum is zero (for sender) or public key sum is point at infinity (for receiver), add corresponding test vectors

[josibake]

Simple suggestion

LGTM! I don't think the pre-merge history is super relevant/useful here. Thanks for picking this up!

[theStack]

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.

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.

[josibake]

ACK 4c0c3d7

Test cases look good, left a nit regarding change log ordering, not blocking.

[josibake]

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

[theStack]

Good point, I agree! Done.

[jonatack]

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.

[jonatack]

ACK 496e429, only change since BIP author approval in #1620 (review) was taking feedback on change log order.