btcutil: Add library functions for BIP-0352 silent payment _send_ support#2244
btcutil: Add library functions for BIP-0352 silent payment _send_ support#2244guggero wants to merge 8 commits into
Conversation
95b791d to
f8bf376
Compare
Pull Request Test Coverage Report for Build 20168757637Details
💛 - Coveralls |
2a9a931 to
996c4d3
Compare
996c4d3 to
28b7587
Compare
88fa441 to
8a4fe70
Compare
|
This is great work and very welcome. However, BIP-375 specifies exclusion for PSBTv0, so this will not be compatible with other PSBT libraries/wallets. Imho it would be best to focus on adding support for PSBTv2 first (#2328). |
8a4fe70 to
dc6f721
Compare
dc6f721 to
a2a18af
Compare
|
Hey, just wanted to drop a quick note as an LND user who's been following this PR and the overall silent payments effort. I understand that PSBTv2 support (#2328) would be needed for full BIP375 compliance and interoperability with external signers/hardware wallets. But even without that, having SP send support land in btcutil would already be a huge step forward, since it would unblock LND PR lightningnetwork/lnd#9398 and allow LND users to send to silent payment addresses using LND's internal wallet and signer. Would love to see this land. Thanks for all the great work on this @guggero! |
| B := ScalarMult(privateKey.Key, scanKey) | ||
| G := Generator() | ||
|
|
||
| proof, err := DLEQProof(privateKey, B, G, r, nil) |
There was a problem hiding this comment.
CreateShare passes the computed ECDH share (a·scanKey) as the B argument to DLEQProof. DLEQProof then computes C = a·B, so this proof is for a²·scanKey, not for the returned share. A verifier using (A=privateKey.PubKey(), B=scanKey, C=share) will reject it. This should pass scanKey to DLEQProof and return the separately computed share.
There was a problem hiding this comment.
Great catch! Fixed and added unit test.
| } | ||
|
|
||
| // Let rand = hash_BIP0374/nonce(t || cbytes(A) || cbytes(C)). | ||
| rand := chainhash.TaggedHash( |
There was a problem hiding this comment.
Current BIP374 includes the optional message in the nonce derivation: hash_BIP0374/nonce(t || cbytes(A) || cbytes(C) || m'). This implementation omits m, which can leak a if the same aux randomness is reused with different messages.
There was a problem hiding this comment.
Yes, this was updated later on in the BIP. Fixed and did a full review on the latest version of the BIP.
| // These types are supported in any case. | ||
| return true, script, nil | ||
|
|
||
| case txscript.ScriptHashTy: |
There was a problem hiding this comment.
This accepts every P2SH output as silent-payment compatible. BIP352 only includes P2SH-P2WPKH in shared-secret derivation; arbitrary P2SH/multisig inputs should be excluded or otherwise identified precisely, otherwise sender and receiver can derive different secrets.
There was a problem hiding this comment.
Correct. Passing in the public key now so we can at least assert the address wasn't made from an incorrect (e.g. uncompressed key).
| return nil, errors.New("invalid bech32 version") | ||
| } | ||
|
|
||
| regrouped, err := bech32.ConvertBits(data[1:], 5, 8, false) |
There was a problem hiding this comment.
DecodeAddress slices data[1:] and later reads data[0] before checking that data is non-empty. A checksum-valid Bech32m string with an empty data part can panic here instead of returning an error.
There was a problem hiding this comment.
Yes, good catch. Added a fuzz test as well and ran it for a few hours on a beefy machine.
| func DLEQVerify(A, B, C, G *btcec.PublicKey, proof [64]byte, m *[32]byte) bool { | ||
| // Fail if any of is_infinite(A), is_infinite(B), is_infinite(C), | ||
| // is_infinite(G). | ||
| if !A.IsOnCurve() || !B.IsOnCurve() || !C.IsOnCurve() { |
There was a problem hiding this comment.
The comment quotes BIP374's requirement to fail if any of A, B, C, or G is infinite/off-curve, but the code only checks A, B, and C. Since G is caller-supplied, it should be validated too.
There was a problem hiding this comment.
Makes sense, fixed.
|
|
||
| idx := 0 | ||
| grouped := make([][]Address, len(groups)) | ||
| for _, group := range groups { |
There was a problem hiding this comment.
GroupByScanKey builds the returned slice by ranging over a map, so group order is randomized. Since AddressOutputKeys documents that recipients are ordered by output index and returns a flat result slice, this can make the returned output order nondeterministic for multiple scan-key groups.
There was a problem hiding this comment.
Very good point, addressed and unit tested.
yyforyongyu
left a comment
There was a problem hiding this comment.
Fired some agents to review it while catching up with the bip🤓
|
I currently use Is it maybe a good idea to add a more generic |
Hmm, that's a valid point. But I'm not sure what would be best here. Adding anything to the existing So perhaps a simple |
|
Thanks a lot for the review, @yyforyongyu! I addressed all your points and did a full (agentic) review against the three updated BIPs and made sure all updated test vectors also pass. Should be quite solid now. |
Yeah, that would help already! Anything to make the process easier is very much appreciated. |
Okay, I've added |
Fixes an independent issue discovered while reviewing and testing the code that failed to detect duplicate xPubs because the wrong variable (keyData holding the nil value of the global unsigned TX, instead of the lowercase keydata that's the loop variable).
Adds library/API functionality for Silent Payment send support.
Implements the following BIPs (or pull requests to BIPs):
Goal is to address send support first as that's rather straightforward. Receive support is much more involved as it requires scanning the chain in a specific manner.