simplexum / python-bitcointx Goto Github PK

While checks for more SCRIPT_VERIFY_* flags were added, some flags are still not supported.

It would be great to have support for all flags.

The flags that are not handled are placed into UNHANDLED_SCRIPT_VERIFY_FLAGS set.

script_tests.json contains test cases with some of these flags, but these test cases were disabled via ["FIXME", "...comment..."] / ["FIXME_END", "...comment..."] construction, that is handled in test_scripteval.py

for better compatibility with bitcoin core script evaluator, more SCRIPT_VERIFY_* flags should be handled.

(Note that VerifyScript is not consensus-compatible, and probably will never be. A warning was added to README.md concerning this).

When code is added to core/scripteval.py to handle a flag, this flag should be removed from UNHANDLED_SCRIPT_VERIFY_FLAGS set, and test cases for it should be enabled/added at script_tests.json

taproot support is dependent on libsecp module that is experimental and can change its ABI (see #57 (comment))

and in general the ABI of libsecp is not a guaranteed thing, because they don't have releases yet

other projects just include particular version of libsecp source and build their own library, but that makes the package much havier and adds a lot of potential problems on installation related to compiling native code.

projects can just handle the compilation themselves and load their version of the library directly by supplying path to load_secp256k1_library(), via set_custom_secp256k1_path() or via LD_LIBRARY_PATH environment variable (on systems where it has effect)

This issue is for discussing the ways to handle the problem. What is the best way to handle this to enable the release that supports taproot ?

1. Bundle libsecp with bitcontx and compile on install (anyone willing to help writing the needed code for that ?)
2. Recommend using particular commit of libsecp with bitcointx, tell projects to handle supplying the correct version of the library themselves at runtime
3. Leave as it is now - the hack that checks for secp256k1_schnorrsig_sign_custom as an indicator that the library version is after some particular commit and hope no other changes will happen
4. Do not merge taproot support into master branch and not do release until there's stable ABI for schnorr signatures in libsecp

In this line:

https://github.com/Simplexum/python-bitcointx/blob/master/bitcointx/core/psbt.py#L918

the library returns None if the input type is p2sh but no redeemscript is provided for this input. But as per 5d46995 , instead a PSBT_InputSignInfo with is_final=False, num_new_sigs=0 is returned in cases where no key is found for signing.

This cropped up in testing of my SNICKER code in Joinmarket that queries a PSBT passed to the wallet that should have one signed (not owned by us) and one unsigned input, where we do a first check here that there is exactly one unsigned input (we earlier check there are exactly two inputs). We do this by passing an empty keystore to PartiallySignedTransaction.sign() here (I believe i saw this in your codebase somewhere and copied the idea, memory a bit hazy).
(To be clear this SNICKER code is not yet at all used in action, it's still in a testing phase).

So overall, I am not sure whether this is intended behaviour, or a mistake, or something that could/should be changed.

See bitcoin/bips#921 and bitcoin/bitcoin#22836.

pypy3 -u -m unittest bitcointx.tests.test_scripteval.Test_EvalScript.test_script_bitcoinconsensus

fails with _ctypes.basics.ArgumentError: Expected bytes of length one as character

while the arguments are clearly multi-byte (but even if it was bytes of length 1, this error should not be raised, in my understanding).

It seems that ctypes is a second-class citizen in PyPy land, and cffi is preferred, while python-bitcointx uses ctypes and there's no point in using more heavy ffi package.

Debugging these quirks is too costly.

Because of this, I removed PyPy from the tests in .travis.yml in 302f4f3, to indicate that python-bitcointx cannot be expected to work correctly with PyPy.

`
import bitcointx
import bitcointx.rpc
from bitcointx.core import *
from bitcointx.core.script import CScript, IsLowDERSignature
from bitcointx.core.key import CPubKey
from bitcointx.wallet import *

proxy = bitcointx.rpc.Proxy(service_url="_MAINNET_URL", timeout=10)
chash = proxy.getrawtransaction(lx("e7a3e246c6f2d582b089d7d6c2f925e8aae46ef0c0ce97d3dd3afe3016a44e97"))
scriptPubKey = chash.vout[0].scriptPubKey

actual = CBitcoinAddress.from_scriptPubKey(scriptPubKey)
expected = P2PKHBitcoinAddress.from_pubkey(scriptPubKey[1:66])
`

actual
P2PKHBitcoinAddress('1FfxRsEZwZkdHjuwBv815eqTWgEbQKpJF7')
expected
P2PKHBitcoinAddress('1HCeeU957J4NTXDer2fGvDsb7mVjU9TtLb')

bitcoin-cli getrawtransaction e7a3e246c6f2d582b089d7d6c2f925e8aae46ef0c0ce97d3dd3afe3016a44e97 1
{
"txid": "e7a3e246c6f2d582b089d7d6c2f925e8aae46ef0c0ce97d3dd3afe3016a44e97",
"hash": "e7a3e246c6f2d582b089d7d6c2f925e8aae46ef0c0ce97d3dd3afe3016a44e97",
"version": 1,
"size": 135,
"vsize": 135,
"weight": 540,
"locktime": 0,
"vin": [
{
"coinbase": "04ffff001d02a700",
"sequence": 4294967295
}
],
"vout": [
{
"value": 50.00000000,
"n": 0,
"scriptPubKey": {
"asm": "04a75502d3670b14b7e5640591191c05868da841699d52f9752a70dc891183a2b9a00f7a5893389f3b973a8ac5e30c20041ccbd99a3889b7ae7a705203d0991498 OP_CHECKSIG",
"hex": "4104a75502d3670b14b7e5640591191c05868da841699d52f9752a70dc891183a2b9a00f7a5893389f3b973a8ac5e30c20041ccbd99a3889b7ae7a705203d0991498ac",
"reqSigs": 1,
"type": "pubkey",
"addresses": [
"1HCeeU957J4NTXDer2fGvDsb7mVjU9TtLb"
]
}
}
],
"hex": "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0804ffff001d02a700ffffffff0100f2052a01000000434104a75502d3670b14b7e5640591191c05868da841699d52f9752a70dc891183a2b9a00f7a5893389f3b973a8ac5e30c20041ccbd99a3889b7ae7a705203d0991498ac00000000",
"blockhash": "000000004d6a6dd8b882deec7b54421949dddd2c166bd51ee7f62a52091a6c35",
"confirmations": 553173,
"time": 1231669673,
"blocktime": 1231669673
}

bitcointx/wallet.py
270 - pubkey = scriptPubKey[1:65]
270 + pubkey = scriptPubKey[1:66]

Hi,
In testing usage of the bitcointx.core.psbt module, I found the following issue:
In case I have created (or given) a PSBT with not all of its input owned by me (my wallet), I cannot call PartiallySignedTransaction.sign(keystore) and have it sign the inputs I own, if the input(s) I don't own are of type p2sh, witness (in this case, p2sh-p2wpkh but doesn't matter) and I don't have the redeem script specified.

I'm referring specifically to the check for rds here

I can see a reason for this: in some scenarios, it will be important for security to know the scripts of all of the inputs to be signed, so that I can verify that those others are actually not owned by me (this was a potential attack first described by Greg Sanders iirc: psbts with hardware wallets, where you check the balance of the spend based on your input contributions, but you may unknowingly sign the same transaction twice, with different inputs, each time thinking the other input is not yours).

However I can also see scenarios (including one that I particularly am trying to implement, SNICKER) where this security risk does not exist, and where it would be convenient to be able to simply call sign() on the psbt object and have it automatically recognize matches of the CTxOut utxo fields in the inputs to the keys in the keystore. In those cases, I'm thinking about: Alice creates a PSBT with a utxo chosen from the blockchain, as well as one or more of her own, and then signs the PSBT, signing all her own inputs and of course leaving the one she doesn't own, unsigned. Here there is no risk as Alice was the one who chose all the inputs. Then Bob the receiver, who owns the remaining utxo, will take the PSBT and finalize it later. He is also safe as all the inputs he doesn't own are already signed (it's a one-way flow from Alice to Bob).

Of course I may have simply misunderstood some detail of the workflow, but if so, hopefully the above allows you to correct me :)
I do understand that I could simply "manually" sign inputs one by one, but it feels like that shouldn't be necessary.

For example:

Bitcoin Testnet address tb1ppsdp29zhvd0m7rmvn07sqv53eesjllq2w9lk97vy9qck3y37hudsv8kvfp cannot pass the psbt.sign

I checked the code https://github.com/Simplexum/python-bitcointx/blob/master/bitcointx/core/psbt.py#L1027 , seems witness only support v0 keyhash & v0 scripthash

In testing wasabi-joinmarket compatibility, encountered this problem; read in a psbt here and then sign here (note: both participants in the payjoin are using p2wpkh, only), results in the above error. I did not previously encounter an error like this in testing, when I was using PSBTs with only WITNESS_UTXO fields.

This makes sense to me, because as I understand it, we now default to having the utxo field of PSBTs always be nonwitness (CTransaction) type, and the PSBT_Input.sign() function seems to use this field to decide whether to apply non-witness signing (and invalidates a case where the script pubkey is of witness type). This looks like a bug? (or did I miss something) (in case not obvious what I mean, to be explicit: the fact that the utxo field of a PSBT_Input is of type CTransaction does not mean the input is not witness type, but the code seems to say that it does).

bitcoin/bips#955

It was possible to create p2sh address with data size != 20 (fixed in 8c10668)

It should have been catched by tests.

When OpenSSL 3 is installed, such that ctypes.util.find_library('ssl') returns 'libssl.so.3', the test suite fails with:

======================================================================
ERROR: test_tx_valid (bitcointx.tests.test_transactions.Test_CTransaction)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/dev-python/python-bitcointx-1.1.3/work/python-bitcointx-python-bitcointx-v1.1.3/bitcointx/tests/test_transactions.py", line 296, in test_tx_valid
    VerifyScript(tx.vin[i].scriptSig, prevouts[tx.vin[i].prevout], tx, i, flags=flags)
  File "/var/tmp/portage/dev-python/python-bitcointx-1.1.3/work/python-bitcointx-python-bitcointx-v1.1.3/bitcointx/core/scripteval.py", line 1175, in VerifyScript
    raise VerifyScriptError("scriptPubKey returned false")
bitcointx.core.scripteval.VerifyScriptError: scriptPubKey returned false

======================================================================
FAIL: test_script (bitcointx.tests.test_scripteval.Test_EvalScript)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/dev-python/python-bitcointx-1.1.3/work/python-bitcointx-python-bitcointx-v1.1.3/bitcointx/tests/test_scripteval.py", line 168, in test_script
    VerifyScript(scriptSig, scriptPubKey, txSpend, 0, flags, amount=nValue, witness=witness)
  File "/var/tmp/portage/dev-python/python-bitcointx-1.1.3/work/python-bitcointx-python-bitcointx-v1.1.3/bitcointx/core/scripteval.py", line 1175, in VerifyScript
    raise VerifyScriptError("scriptPubKey returned false")
bitcointx.core.scripteval.VerifyScriptError: scriptPubKey returned false

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/var/tmp/portage/dev-python/python-bitcointx-1.1.3/work/python-bitcointx-python-bitcointx-v1.1.3/bitcointx/tests/test_scripteval.py", line 171, in test_script
    self.fail('Script FAILED: %r %r %r with exception %r\n\nTest data: %r' % (scriptSig, scriptPubKey, comment, err, test_case))
AssertionError: Script FAILED: CBitcoinScript([x('304402200060558477337b9022e70534f1fea71a318caf836812465a2509931c5e7c4987022078ec32bd50ac9e03a349ba953dfd9fe1c8d2dd8bdb1d38ddca844d3d5c78c11801')]) CBitcoinScript([x('038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508'), OP_CHECKSIG]) 'P2PK with too much R padding but no DERSIG' with exception VerifyScriptError('scriptPubKey returned false')

Test data: ['0x47 0x304402200060558477337b9022e70534f1fea71a318caf836812465a2509931c5e7c4987022078ec32bd50ac9e03a349ba953dfd9fe1c8d2dd8bdb1d38ddca844d3d5c78c11801', '0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 CHECKSIG', '', 'OK', 'P2PK with too much R padding but no DERSIG']

currently, to sign with libsecp256k1, you need to check if it is available, with is_libsec256k1_available(), and then enable it with use_libsecp256k1_for_signing(True)

This was done for compatibility, as libsecp256k1 may be not available. Creating the code for making the ECC library pluggable, as suggested in petertodd#123 (comment), will require time and resources that we are not ready to expend.

In practice, we always enable libsecp256k1 for signing. We could just switch to use it by default, but this would mean that if the library is not available, the code will crash with ImportError exception.

There are a bunch of things that can be done to improve the consistency, reliability, or adaptability of the library, but they often require API breakage.

One of the example is #19, where some of the classes have is_valid as property, and others have is_valid() as a method (same for a few other attributes). This is inconsistent, and there are a solution - make is_valid() the standard way. This will require to use special class for these attributes, to prevent usage of is_valid in boolean contexts (so that if pub.is_valid: instead of pub.is_valid(): will result in exception), but this can be done.

Other things include more composable chain parameters. This becomes more of an issue when there are things that you want to work with, beside Bitcoin, like the Elements sidechain. If you want to intermingle the bitcoin and sidechain code, with current design, you will have problems. For example, if you created CBitcoinAddress while Bitcoin chain parameters was in effect, but later switched to Elements, the str(addr) will change for that instance of an address - because base58 prefixes are globally switched when changing params.

To make things more composable, different classes for each chain parameters will be introduced, like, CBitcoinAddress, CBitcoinTestnetAddress, CElementsSidechainAddress, and one more general, CCoinAddress - which will be just a front-end class, and the instances it creates will be different based on which chain parameters are in effect. This will allow, for example, addresses created with CCoinAddress within with ChainParams('sidechain/elements'): to be Elements addresses, and to retain all their properties outside that block. (Note that the C prefix for the names of these classes is an artifact of that the library tries to be close to bitcoin core C++ code, which uses this prefix for the classes. I am not sure that it is right to retain this prefix for the address classes, but currently they are retained)

edit: Different chain parameters and rules need not necessarily be added to python-bitcointx package itself. For example, support for Elements sidechain can be implemented in separate python-elementstx package.

Because there will be major API breakage, I am considering to do more renaming/changes in the name of consistency, and maybe some deprecations in the name of simplifying maintanence.

Please add native-segwit utxo signing example.
I couldn't find any other python lib that would support signing this.

Do you plan to support BIPs 340/341/342 ? Additionally, are you open to having PRs on that? (Not that I plan to, right now, just figuring out future paths both for @JoinMarket-Org and possibly some other projects).

In light of potential large-fee-ransom attacks against autonomous singners of segwit transactions (details: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-August/014843.html https://twitter.com/FreedomIsntSafe/status/1268572922911887360), it is not wise to disallow full-CTransaction utxo fields in PSBT_Input for segwit inputs, because one of the workarounds for the attacks is supplying full previous transactions for segwit inputs in the same way as done for non-segwit inputs.

I am currently working on a change that would allow to leave CTransaction in utxo field of PSBT_Input, even if the input is segwit, and put CTxOut (probably extracted from CTransaction) into (new) witness_utxo field. The extraction routine will check that prevout.hash of the input in unsigned_tx and prev_tx.GetTxId() match.

@AdamISZ you might look into this re JoinMarket-Org/joinmarket-clientserver#536, in case you need to include protections from this attack into your code

See bitcoin-core/HWI#305 and openssl/openssl#16994.

Bitcoin Core solved it by writing own pure Python RIPEMD160 implementation (see bitcoin/bitcoin#23716), but it's meant to be used only for tests as it isn't constant time.

I was previously using sign() method of psbt.PartiallySignedTransaction with a keystore containing keys for only some inputs, and it was working fine, but I now have a use case where some of the other inputs (which I'm not signing and don't have keys for) are required (see BIP78) to not have the utxo field filled in. When I call sign() in this case, those inputs throw the above mentioned ValueError (which btw has a trivial typo: 'utxo is not set for of PSBT_Input at index {self.index}').

Currently I only have the ugly option of signing first, then deleting the utxo field. But unless I missed something, it should be possible to call sign() on the overall PSBT without it throwing an Exception (though I suppose, it could certainly be an option).

Hi,
Thought I'd ask this as I'm struggling with this and it's possible you'll have a good idea about it @dgpv .

In joinmarket we currently use the binding coincurve which uses cffi and then either (a) links to an existing system libsecp256k1 if it's available (can be disabled) or (b) uses a "local" build by sourcing a tar archive from the secp256k1 repo; this generates during setup a *.so file in the coincurve directory (so e.g. in my virtualenv i have the file /path/to/venv/lib/python3.6/site-packages/coincurve/_libsecp256k1.cpython-36m-x86_64-linux-gnu.so) and then cffi uses that.

I want to do the following: have python-bitcointx use a locally built libsecp256k1 instance so as to not overwrite any existing system installation and not to require sudo usage in installation. Building libsecp as part of the Joinmarket installation is fine, generally, as we require the same for libsodium anyway, so it's an easy add-on (I've already done it), just adding a --prefix to the ./configure arguments. But what I don't know is how I can get python-bitcointx to recognize a local installation (or, anywhere apart from system, i.e. /usr/local/lib, /usr/lib etc.); all I understand is that there is a load_secp256k1_library which calls ctypes.util.find_library, but from examining that code I can't see how there would be any configuration that could tell it to recognize a shared library in a different location. (LD_LIBRARY_PATH can do it, but that's hacky and not portable, I believe).

Do you have either (a) a thought on how I could achieve this as-is or (b) think it's possible to add a patch to make this feasible in python-bitcointx? Thanks.

It is possible to have Mypy complain if a case is missed when branching on an instance of an Enum, see mypy issue 6366.

This could be used in psbt.py , function check_witness_and_nonwitness_utxo_in_sync where all cases of PSBT_InKeyType are matched. Instead of raising AssertionError, _assert_never could be called, which would trigger a Mypy error on compile time. If Mypy isn't run, it still fails on runtime. So there is no disadvantage except the reader needing to learn the NoReturn type.

It could also be used in PSBT_Input.merge, but it seems that Mypy is not smart enough to detect that "(a and b) or (not a and b), (a and not b), (not a and not b)" is always true. It would probably work to introduce an enum with 4 cases for this or maybe it would work with a (bool, bool) tuple, but I don't know if Mypy is smart enough for this, and I don't know if you think it is worth it.

Currently travis tests do not install libbitcoinconsensus.

To install this lib, you need to build it from bitcoin source.

Need to write appropriate scripts to do this in travis environment.

Maybe there is a script somewhere that can build libbitcoinconsensus without the need to build the whole bitcoind (slow build), and that can be easily plugged in into .travis.yml ?

Trying to use the libbitcoinconsensus that Bitcoin Core's Guix binaries ship with is giving me this error.

ImportError: Cannot import consensus library: ./libbitcoinconsensus.so: undefined symbol: _ZTVN10__cxxabiv120__si_class_type_infoE

Is that expected behavior? Your docs say to build from source, but it's not obvious to me why upstream binaries shouldn't work. I'm running Debian Bullseye if it matters.

They have made release finally. https://github.com/bitcoin-core/secp256k1/releases/tag/v0.2.0

If you decode the script the straightforward way, not using raw_iter, you will not be able to distinguish between minmal and non-minimal pushes

Maybe add additional 'gotchas' section to the readme

But should check that they are in-sync

the system does not have this secp25k1.
Does it have a way to embed this library in pip manager?

It can be said that this is an oversight when implementing taproot support.

The original code from python-bitcoinlib simply returned the first byte of the script as witness version. This worked while the only valid version was zero (petertodd#298).

Correct way would be to do decode_op_n() on that first byte before returning it.

The problem that it is now already in the library and some code might depend on it returning 0x51

I need to decide how to handle this situation.

Do I simply fix it straightforwardly, and allow the behavior of witness_version() to change ? This might break existing code that relies on witness_version() returning 0x51 for taproot outputs. I have no way to know if there is such code out there.

I can add CScript.witness_version_value() that will return decoded version, but then having also withess_version() will be confusing.

Shall I remove witness_version() altogether to prevent any confusion and replace it wit witness_version_value() ?

Or shall I ignore the possibility that there's code that relies on witness_version() returning 0x51 for taproot outputs, and fix this straightforwardly ?

Can't decide.

previously when None is supplied, the utxo was unset

now, the existing utxo is just taken.

I think this should be treated as a bug, but the new semantics allowed to 'witness-ize' the utxo conveniently with psbt.set_utxo(None, i, force_witness_utxo=True). Maybe need to find better way to do this

bitcoin/bitcoin#29189

On a brief look through the code today, I can't see functions/flags that let a caller check if the state of a given PSBT_Input is finalized or not (or the PSBT as a whole). I know I can check final_script_sig and final_script_witness but it seems like it might be useful.

I see that this is based on:

• https://github.com/petertodd/python-bitcoinlib or
• https://github.com/dgpv/python-bitcoinlib

So which one should I use?

kthxbi

Since Core now got rid of CTxWitness in bitcoin/bitcoin#8589, it might make sense to at least provide a 'compatible' accessor to be able refer to tx.wit.vtxwinwit[i].scriptWitness via tx.vin[i].scriptWitness.

But we need to retain CTxWitness and old way of tx.wit.... to not break any existing code

From a Joinmarket user report, we saw this exception raised:

bitcointx.core.serialize.DeserializationValueBoundsError: non-canonical compact size for variable integer: 0x3e4a9d70215fc30a more than 0x2000000

This came from a CTransaction.deserialize call:

btc.CMutableTransaction.deserialize(
  File "...bitcointx/core/serialize.py", line 147, in deserialize
    r = cls.stream_deserialize(fd, **kwargs)
  File "...bitcointx/core/__init__.py", line 113, in wrapper
    return fn(*args, **kwargs)
  File "...bitcointx/util.py", line 330, in wrapper
    return fn(*args, **kwargs)
  File "...bitcointx/core/__init__.py", line 1113, in stream_deserialize
    wit = CTxWitness.stream_deserialize(f, num_inputs=len(vin),
  File "...bitcointx/util.py", line 330, in wrapper
    return fn(*args, **kwargs)
  File "...bitcointx/core/__init__.py", line 926, in stream_deserialize
    vtxinwit = tuple(CTxInWitness.stream_deserialize(f, **kwargs)
  File "...bitcointx/core/__init__.py", line 926, in <genexpr>
    vtxinwit = tuple(CTxInWitness.stream_deserialize(f, **kwargs)
  File "...bitcointx/util.py", line 330, in wrapper
    return fn(*args, **kwargs)
  File "...bitcointx/core/__init__.py", line 815, in stream_deserialize
    scriptWitness = script.CScriptWitness.stream_deserialize(f, **kwargs)
  File "...bitcointx/core/script.py", line 1158, in stream_deserialize
    stack = tuple(BytesSerializer.stream_deserialize(f, **kwargs)
  File "...bitcointx/core/script.py", line 1158, in <genexpr>
    stack = tuple(BytesSerializer.stream_deserialize(f, **kwargs)
  File "...bitcointx/core/serialize.py", line 328, in stream_deserialize
    datalen = VarIntSerializer.stream_deserialize(f, **kwargs)
  File "...bitcointx/core/serialize.py", line 309, in stream_deserialize
    raise DeserializationValueBoundsError(
bitcointx.core.serialize.DeserializationValueBoundsError: non-canonical compact size for variable integer: 0x3e4a9d70215fc30a more than 0x2000000

So, pretty clearly this could be caused by simply having a corrupted (somehow) serialized transaction, but my question is, is there any, however obscure, case where this could occur from a valid Bitcoin transaction?

Obviously diagnosing this particular case would ideally involve looking at the serialized tx that was passed in, but generally we don't ask users for any identifying data, so I'm just asking the question in abstract for now.

(Edit: I should mention that the call occurs here, note that it is the "hex" field of a gettransaction RPC call, which is why we would always expect it to be an actual/not corrupt or malformed serialized transaction:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/81982c729f4bb8f702b6c73f4685463c97cb6e4a/jmclient/jmclient/blockchaininterface.py#L294-L302

Here's the failure in the test run:

https://github.com/Simplexum/python-bitcointx/runs/4469632502?check_suite_focus=true#step:7:316

The code for the case of platform.system() == 'Darwin' tries to find libssl.35, but it seems that it is not available on the newest macos.

Should we just add some other library name to try ?

I don't have macos to check this. I could try to investigate the macos image used in github actions, but I'm not inclined to use my time for this right now.

Probably someone with access to the latest macos can give a hint for how to handle this.

We removed the network-related code from the original python-bitcoinlib, so now a situation possible that some code that uses it will try to import bitcoin.messages, for example, and fail,
if python-bitcointx is installed in PYTHONPATH.

We can change the name of the package to 'bitcointx'. This prevents possible conflicts, but also means that the code that want to switch to our derivative library will need to change their imports.

bitcoin.core -> bitcointx.core, etc.

There's probably very few people who use the network-related code, anyway, and if there are any, they probably use and maintain their own fork.

I'm not sure if we should proceed with the package rename.

We currently do RPC calls to bitcoind via RawProxy from bitcoin/rpc.py, but thinking about deprecating this, in favor of generic approach, based on python-requests.

bitcoin/rpc.py is a thin wrapper, and we only used RawProxy from there, bypassing any wrapper functions in Proxy class.

>>> from bitcointx.core import *
>>> import threading
>>> def f():
...     print(CTransaction())
... 
>>> print(CTransaction())
CBitcoinTransaction(, , 0, 2, CBitcoinTxWitness([]))
>>> t=threading.Thread(target=f)
>>> t.start()
>>> Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/lib64/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "<stdin>", line 2, in f
  File "/home/tmp/rgb/python-bitcointx/bitcointx/core/__init__.py", line 91, in __call__
    if _thread_local.mutable_context_enabled:
AttributeError: '_thread._local' object has no attribute 'mutable_context_enabled'

the thread-local storage needs to be initialized for each thread, and now it is not.

Most other classes have is_valid() as method, but in CPubKey it (and other is_ attributes) is a @property. This is inconsistent, but fixing this is dangerous: If we make is_valid in CPubKey a method, and some old code that expects is_valid to be a property will do if pub.is_valid: -- this condition will always be true.
This will lead to bugs.
Changing is_* methods in other classes to @property is also bad option: it will probably break a lot of code unnecessary, and may introduce same subtle bugs: if the code that is adapted for the case where all is_* is a @property, is then used with other libraries derived from python-bitcoinlib, where were no such changes.

Status quo is less dangerous: pub.is_valid() will just throw TypeError.

One option may be to return a wrapped bool object that will also act as callable, but the scale of this problem with inconsistent interface for CPubKey seems to be not that huge to require such hacky fix.

It might crash on OSX: petertodd#238

Summarized in this bitcoin stackexchange question:

https://bitcoin.stackexchange.com/questions/81551/allowed-der-encoding-format-violations-for-signatures-in-bitcoin-implications-f

Start of the discussion is here: #60 (comment)

The question is, should we remove the (historically present, was in python-bitcoinlib) ability to create CPubKey instances from invalid pubkey data ?

If we do so, we should also remove is_fullyvalid() methods to avoid potential problems described in #60 (comment)

This will mean quite major API breakage, though.

For regtest/testnet mode, bitcointx raises an error when attempting to sign an electrum-generated PSBT because of the presence of an xpub with prefix 0x043587cf (tpub) and does not accept an xpub with prefix 0x02575483 (Vpub).

https://electrum.readthedocs.io/en/latest/xpub_version_bytes.html#specification

I think the simplest solution here would be some xpub type coercion when the user calls the signer with a special flag? Or even type coercion based on --regtest/--mainnet flags.

That said, @darosior mentions:

hardcoding of specific script types inside the encoding of an extended public key is an unscalable and terrible layer violation