WIP implementation of BIP322 #140
Conversation
There was a problem hiding this comment.
Good start to the thing. I left some comments. Thanks for the contribution @wip-abramson !
buidl/message.py
Outdated
| return True | ||
|
|
||
|
|
||
| def sign_message(format: MessageSignatureFormat, private_key: PrivateKey, address: str, message: str): |
There was a problem hiding this comment.
Nothing else in the library uses Type hints like this. I would prefer you specify in the documentation.
Also, please make clear what you are getting back when you call this function. What sort of signature is it? How do I verify it? What's the signature encoded in?
There was a problem hiding this comment.
I have removed type hints and added documentation in comments above the function
buidl/message.py
Outdated
|
|
||
| # return base64_encode(signature.der()) | ||
|
|
||
| def sign_message_bip322(format: MessageSignatureFormat, private_key: PrivateKey, address: str, message: str): |
There was a problem hiding this comment.
Generally in sign message functions, there should be some sort of sanity check about whether it validates. If it doesn't then we can throw an error.
There was a problem hiding this comment.
Isn't that what is implemented on line 195 - 197? Or did you have something else in mind
buidl/message.py
Outdated
|
|
||
| def sign_message_bip322(format: MessageSignatureFormat, private_key: PrivateKey, address: str, message: str): | ||
|
|
||
| assert(format != MessageSignatureFormat.LEGACY) |
There was a problem hiding this comment.
A helpful error message would be nice.
There was a problem hiding this comment.
Added the following error message to the assert
BIP 322 Signatures must be represented in either the Simple of Full Formats
| tx_outputs = [tx_output] | ||
| network="mainnet" | ||
| # is to_sign always a segwit? | ||
| segwit=True |
There was a problem hiding this comment.
I have run into some issues with the current hard coded segwit=True that is set of the to_sign transaction. Currently, I am attempting to create a bip322 p2sh multisig.
@jimmysong is there a recommended way for me to test whether this to_sign transation should have it's segwit flag set to True or not.
It is to do with the scriptPubKey on the tx_out of the to_sign transaction right?
|
I'd also like to propose either:
This is because currently these fail for bip322 Txs because no fee is specified (None is required) |
This P.R. adds basic support for bip322 signatures following a similar API to the current open P.R. in Bitcoin core (See bitcoin/bitcoin#24058)
There are a few areas that still need some work:
Open to suggestions for how to restructure this to align with the existing library.
Additionally, would be interested in thoughts around this thread - https://github.com/bitcoin/bitcoin/pull/24058/files#r872561862. I was expecting to be able to reproduce the same signature as provided in the bip. But ended up producing a different, valid signature for the same address, message pair. Does this indicate that there is a mismatch between how RFC6979 is implemented between buidl-python and bitcoin core?
Finally, I want to acknowledge Digital Contract Design and @rxgrant for funding my work on this P.R. to date.
Any time you have to review this much appreciated, thanks!