Add Taproot support #60
Conversation
bitcointx/core/key.py
Outdated
| return True | ||
|
|
||
| def create_tap_tweak(self: T_XOnlyPubKey, *, | ||
| merkle_root: bytes = b'', |
There was a problem hiding this comment.
requiring merkle_root to always be specified as kwarg may feel a bit excessive for the method with just one argument, but I think this is OK since a lot of other functions/methods use merkle_root kwarg, when there are other args, and this way it feels more uniform
| get_script_with_control_block() method to be available | ||
| 'leaf_version' can be supplied for all the script in the tree to | ||
| have this version |
There was a problem hiding this comment.
I decided to not add leaf_version field to CScript, at least for now. It is not clear to me if the single different leaf version for one script among other scripts will be a normal occurence, even when we'll have many leaf versions. It seems more possible that the whole trees will consist of scripts with particular leaf version. And you can always have a 'tree' of just one script to designate a leaf version for just that script.
| return ints | ||
|
|
||
|
|
||
| class VarBytesSerializer(Serializer[bytes]): |
There was a problem hiding this comment.
This was a duplicate of BytesSerializer
| class XOnlyPubKey(bytes): | ||
| """An encapsulated X-Only public key""" | ||
|
|
||
| __fullyvalid: bool |
There was a problem hiding this comment.
I realise that this is not specific to the new taproot/BIP340 code (you already had it in CPubKey, but what is the significance of storing this __fullyvalid bool? Is it somehow useful to have a pubkey object that is from an invalid encoding?
There was a problem hiding this comment.
- The Core makes that distinction for some reason
- It is used in tests to supply invalid pubkeys and check that the code fail with invalid pubkeys
This distinction was present in python-bitcoinlib.
My guess is that Core does this for speed so you can create CPubKey instance from the chunk of bytes quickly and check for validity later.
Maybe it makes sense to throw ValueError on invalid encoding by default, and introduce accept_invalid kwarg for the tests ? This would be breakage of the API, but I doubt this breakage will affect much real-world code except the test code.
There was a problem hiding this comment.
Address classes already have accept_invalid kwarg for from_pubkey() method, but I guess one could accidentally use invalid pubkeys in other places (like, constructing scripts) where they could miss checking the is_fullyvalid().
There was a problem hiding this comment.
OTOH, if we introduce that change, and then new code that uses bitcointx starts dropping is_fullyvalid() checks, but afterwards somehow is used with the old version of the lib, then it could pass invalid pubkeys where it should not. This is somewhat alleviated by the fact that the code may list the version requirement for the library, of course.
There was a problem hiding this comment.
I think only two reasonable paths: leave as is, or completely remove, anything else would have the kind of problems you describe in that last comment. Even if I think the 'remove' option is preferable, the taproot patch is not really the right time to make such a big change.
Address classes already have accept_invalid kwarg for from_pubkey() method
It's interesting that this option exists (even though of course default False). I suppose it might be related to burn addresses? (your performance hypothesis makes sense too, i guess). Generally I'd lean towards making no public API support for such things, since the risk of using them incorrectly outweighs the very small amount of usage (burn addresses are very rarely used for anything).
But no action item here, imo: this is orthogonal to the taproot patch.
There was a problem hiding this comment.
I suppose it might be related to burn addresses?
Its main use (for addreses) is in tests (tests.test_wallet.test_from_invalid_pubkeys)
There was a problem hiding this comment.
and that test_from_invalid_pubkeys was present in python-bitcoinlib since long time ago. So maybe this is really just an option to allow to create burn addresses.
There was a problem hiding this comment.
But no action item here, imo: this is orthogonal to the taproot patch.
I created a separate issue: #61
There was a problem hiding this comment.
Hmm, since XOnlyPubKey is a completely separate class from CPubKey, I think we can make it raise ValueError on invalid data by default, without any breakage. I think we should keep is_fullyvalid() in XOnlyPubKey though, to make it compatible with CPubKey.
| elif len(keydata) == 33: | ||
| ensure_isinstance(keydata, CPubKey, | ||
| 'x-only pubkey data of 33 bytes') | ||
| keydata = keydata[1:33] |
There was a problem hiding this comment.
Maybe I missed something obvious, but: why would we create an XOnlyPubkey from just a removal of the parity byte of a CPubKey? I get it that this might be fine in some contexts, but isn't there the danger that the caller is dealing with an existing private/pubkey pair that is not actually valid for XOnly, because it has the wrong parity?
There was a problem hiding this comment.
From BIP340:
the standard 33-byte compressed public key format consists of a byte indicating the oddness of the Y coordinate, plus the full X coordinate.
so we just drop the 'oddness' byte, and are left with 'full X coordinate'.
The code from Core:
/** Construct an x-only pubkey from a normal pubkey. */
explicit XOnlyPubKey(const CPubKey& pubkey) : XOnlyPubKey(Span<const unsigned char>(pubkey.begin() + 1, pubkey.begin() + 33)) {}
There was a problem hiding this comment.
My concern is the scenario: I have a pre-existing key and for whatever reason I want to make "the taproot address for the same key", I think this is actually wrong-headed, but if you do it, you have some original privkey k, whose public key has coordinates x_k, y_k, if y_k is odd then when you change the pubkey from 03deadbeef.. to deadbeef you are actually encoding the pubkey corresponding to -k, not k (true because on the curve P + -P = O), so a person ignorantly doing this will find themselves unable to sign against this pubkey (although they have the trivial solution of flipping the sign of their privkey, admittedly).
There was a problem hiding this comment.
OK I guess this isn't really anything to discuss, it's already the case that the signing algorithm is 'take private key and if it's not an even y, then flip its sign', so this is already addressed/understood.
There was a problem hiding this comment.
There's another potential problem here to discuss: data taken from invalid CPubKey can actually constitute a valid XOnlyPubKey:
>>> CPubKey(x('0478d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71')).is_fullyvalid()
False
>>> XOnlyPubKey(x('78d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71')).is_fullyvalid()
True
I think the safe route would be to not accept invalid CPubKey here, those who really need to create XOnlyPubKey from invalid CPubKey can simply strip the first byte themselves.
There was a problem hiding this comment.
Right. But I mean coming back to my first comment, I was only concerned that you would get "the wrong" key from parity stripping, but then as per convo I realized that concern doesn't make sense, because of the way signing works.
But what you raise here, i.e. that someone can start with a completely invalid key and after conversion, end with a valid one ... I mean, I honestly don't want to try to figure out the situations where that could cause a problem. The whole thing just feels kind of wrong for cryptographically or financially sensitive code: if you ever know that input is not correct (not fully valid), stop there, don't leave it around, creating a 'maybe valid in future' object.
I do get that this is coming directly from something in Core, but the KISS principle seems to say 'never create pubkey objects from invalid pubkey serializations'.
So in short I agree with your final sentence :)
There was a problem hiding this comment.
I went and made the change to not accept invalid CPubKey as arguments to XOnlyPubKey(): 01f5b3f
dgpv
force-pushed
the
taproot
branch
4 times, most recently
from
1d3a8fc to
5ea6ade
Compare
bitcointx/core/key.py
Outdated
| # Not that it matters much in python where we don't have control over | ||
| # memory and the keydata is probably spread all over the place anyway, | ||
| # but still, do this to be close to the original source | ||
| ctypes.memset(keypair_buf, 0, 64) |
There was a problem hiding this comment.
keypair_buf is 96 bytes, should clear them all
|
After starting to work on taproot support for python-elementstx, I added a bunch of changes here, related to the fact that Elements has different parameters for tagged hashes and has different default leaf version. I moved hashers and default leaf version to Moved tap-tweak-related code from methods of XOnlyPubKey to standalone functions: d317448 I split sign_schnorr into |
|
I was able to pay into a P2TR address (the plain keypath spend type) and spend out of it on regtest. It's a good sanity check to start. Obviously there is a certain amount of just "messing about" in here, but just for insight I've copied it out. More comments below. A few comments: Initially, I found myself rather confused about For example I think the docstring here still isn't quite enough for a user to be absolutely sure what it's doing: I think it should state that the actual recommendation for provably-no-script path (as justified in cite note 22) is being followed (or maybe just: that the default procedure Meanwhile I do think this docstring for Another aspect of this: that You can also see I originally didn't notice the I think it was pretty smooth and intuitive apart from that, inasmuch as, it's very similar to the pre-existing workflow. But then of course I only tried the simplest possible version of the scriptpubkey to spend. At some point I will try using script path stuff, but it's less of a priority for me as I don't have any use case for now. All the same I will read the script part of the patch a bit more and add any further comments from what I see. Btw search |
|
I've noticed that XOnlyPubKey did not have
because this is internal function, I felt that it is ok to use |
|
Great. On that last point, of course, well understood that it is not for external callers; the reason it crops up is because I'm trying to read it carefully to understand exactly what happens when I call the API-level functions. And that's exactly what I don't need to do now because the docstrings are a lot more detailed :) (well, it's always interesting to read the code more, of course! but anyway). |
|
(last force-push to 6270e62 was just to fix docstring of |
Interesting. For what it's worth, my opinion: I don't think this library needs to do that. If there are indeed use cases for duplicate leaves at different points in the tree, then it'd be up to the people creating, reading, signing such transactions (i.e. one step up the software stack from this library) to decide how to deal with it. I suppose my argument is: there are an infinite number of ways in which a leaf might be dangerous to a user, and this is only one; clearly if a user doesn't know the full tree, then there might be a spending path that they should not accept (so agreeing with roconnor's "probably good advice regardless"). |
|
(rebased on current master) |
| tapleaf_hash: Optional[bytes] = None, | ||
| codeseparator_pos: int = -1, | ||
| annex_hash: Optional[bytes] = None | ||
| ) -> bytes: |
There was a problem hiding this comment.
tapleaf_hash, annex_hash, and SIGVERSION_TAPSCRIPT are currently not covered by tests
|
Synced the taproot branch with https://github.com/Simplexum/python-bitcointx/tree/segwitv1_addrs |
|
rebased on current master |
|
merged |
|
Thank you @dgpv for all your efforts.
The most crucial thing needed now, is some examples, I would say. Even very simple, but fully working when sent to network. I tried one for keypath spending similar to this comment by @AdamISZ. It produces a transaction hex, but when sent to a network, an error is returned like this: My code is attached below, any comments to resolve the issue would be appreciated. I think this, once resolved, can be added to examples: import bitcointx as btc
btc.allow_secp256k1_experimental_modules()
from binascii import unhexlify
from bitcointx.core import CTransaction
from bitcointx.wallet import P2TRBitcoinTestnetAddress
from bitcointx.core import CMutableOutPoint
from bitcointx.core import CMutableTxIn
from bitcointx.wallet import CCoinAddress
from bitcointx.core import CMutableTxOut
from bitcointx.core import CMutableTransaction
from bitcointx.core import CTxOut
from bitcointx.core.script import SignatureHashSchnorr
from bitcointx.wallet import CCoinKey
from bitcointx.core import CMutableTxInWitness
from bitcointx.core.script import CScriptWitness
from bitcoin.rpc import Proxy
def parse_raw_tx(tx_hex):
parsed_tx = {
'version': tx_hex[:8],
'Input Count': tx_hex[8:10],
'Input 1 Previous Output Hash': tx_hex[10:74],
}
return parsed_tx
print(btc.select_chain_params("bitcoin/testnet"))
def test_tx_from_raw_byte_keys():
priv = b"\x01"*32
alice_priv_key = CCoinKey.from_secret_bytes(priv)
print(alice_priv_key.xonly_pub)
print(alice_priv_key.pub)
alice_address = P2TRBitcoinTestnetAddress.from_xonly_pubkey(alice_priv_key.xonly_pub)
print("Alice address: ", alice_address)
print(P2TRBitcoinTestnetAddress.from_pubkey(alice_priv_key.pub))
# here i used a faucet to fund the address.
tx_id = "86e68fc34258372e96626841c3b05ae6b478d20fe85a769bca91bf1b13f8c83c"
prev_tx_hex = "02000000000101bdf4080aaefe9d274f5f91ca1250d81cd05e62d67641bbf52e71e246ee8557e40000000000feffffff02abbb0f0000000000160014c885ac9ecdee154b1ac3a372ebc1184601b9da20204e0000000000002251208c5db7f797196d6edc4dd7df6048f4ea6b883a6af6af032342088f436543790f0247304402205de0c18eab95151c1a2908b247c4c5197894a4587ed0326cba06976183f7b058022067251f579e85f027368056cd1fda0a0baef9a5dac69ee6736e2d84f95ee3fa410121034bcc56646b1e8f619522c3db1feebd64bac532a5e09389e4805caf55cf38d3da167b2100"
prev_tx_bytes = unhexlify(prev_tx_hex)
prev_tx = CTransaction.deserialize(prev_tx_bytes)
print("Previous tx: ", prev_tx)
print("Previous tx parsed: ", parse_raw_tx(prev_tx_hex))
print("Available balance: ", prev_tx.vout[1].nValue)
output_index_in_prev_tx = 1
balance = prev_tx.vout[output_index_in_prev_tx].nValue
fee = 2000
amount = balance - fee
priv2 = b"\x02"*32
bob_priv_key = CCoinKey.from_secret_bytes(priv2)
bob_addr = P2TRBitcoinTestnetAddress.from_xonly_pubkey(bob_priv_key.xonly_pub)
print("Bob address: ", bob_addr)
# this is the format we use in JM, but not needed here:
outs = [{"address": str(bob_addr), "value": amount}]
print("outs:", outs)
intxid = unhexlify(tx_id)
outpoint = CMutableOutPoint(intxid[::-1], output_index_in_prev_tx)
vin = [CMutableTxIn(prevout=outpoint, nSequence=0xffffffff)]
print("vin:", vin)
sPK = CCoinAddress(outs[0]["address"]).to_scriptPubKey()
print("sPK:", sPK)
vout = [CMutableTxOut(outs[0]["value"], sPK)]
print("vout:", vout)
tx = CMutableTransaction(vin, vout, nVersion=2)
cto1 = CTxOut(balance, alice_address.to_scriptPubKey())
print("cto1:", cto1)
spent_outputs = [cto1]
print("SignatureHashSchnorr:", SignatureHashSchnorr(tx, 0, spent_outputs))
sighash = SignatureHashSchnorr(tx, 0, spent_outputs)
sig = alice_priv_key.sign_schnorr_tweaked(sighash)
print(len(sig))
tx.wit.vtxinwit[0] = CMutableTxInWitness(CScriptWitness([sig]))
print("tx:", tx)
connection = Proxy(service_url='http://api.blockcypher.com/v1/btc/test3/txs/push')
res = connection.sendrawtransaction(tx)
print(res)
if __name__ == '__main__':
test_tx_from_raw_byte_keys()
|
|
@HRezaei just in case it helps, I'm guessing you didn't see this example I did around that time, which tested script path and which broadcast OK: https://gist.github.com/AdamISZ/6233d9d9b8d25483dc5d39cc6b9892a7 |
Thanks @AdamISZ, from bitcoin.rpc import Proxy
connection = Proxy(service_url='http://api.blockcypher.com/v1/btc/test3/txs/push')
res = connection.sendrawtransaction(tx2)
print(res) |
Presumably you don't mean that literally? That transaction of course wouldn't broadcast, it's already been mined a month or two back :) But in that case I wonder what you do mean? Re: using a proxy, I'd just use a signet or testnet (highly prefer signet! testnet3 is awful for many reasons) Core instance and use |
|
You're right @AdamISZ, |
3 participants
This adds a bunch of new APIs and it is really important for me to hear opinions on the convenience/ergonomics of these APIs. Please don't hesitate to comment if you think something might be made more convenient for the developer.
Of course the correctness/safety is the most important, but convenience is close to that in importance, as it can reduce the chance for mistakes