CScript.witness_version() returns 0x51 instead of 1 for taproot

[dgpv]

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.

[dgpv]

Maybe I should do v1.1.4.post0 with this fix...

[dgpv]

instead of v1.1.4.post0, I'm thinking about releasing v1.1.5 just for this fix, because while it is a fix that should be put out right away, it is a breaking change for the code that depends on incorrect behavior, if such code exists somewhere.

[dgpv]

I am going to release v1.1.5 with this fix shortly, unless there are objections.

@kristapsk @AdamISZ @jonasnick plz check

[kristapsk]

It looks JoinMarket doesn't call witness_version() anywhere in code (checked with grep -R witness_version src/).

[kristapsk]

@dgpv You could add commit with fix to some branch, then I can test JoinMarket against it, just in case.

[dgpv]

While at it, I decided to change the way secp256k1 library handle and library capabilities is accessed.

This allowed the path change to take effect even if set_custom_secp256k1_path() is called after
bitcointx.core is imported (but before any of the functions that use library handle is called)

This change too can cause breakage in the code that used secp256k1_* variables from
bitcointx.core.secp256k1. Three functions are introduced in this module instead:
get_secp256k1_handle(), get_secp256k1_contexts() and get_secp256k1capabilities().
Those interested in using them (which is expected to be very few), please look into the source
code of the module.

The changes (including of course the witness_version() fix) are in the https://github.com/Simplexum/python-bitcointx/tree/fix_witness_version

If you can test this changes, that would be great

[kristapsk]

While at it, I decided to change the way secp256k1 library handle and library capabilities is accessed.

This allowed the path change to take effect even if set_custom_secp256k1_path() is called after bitcointx.core is imported (but before any of the functions that use library handle is called)

This change too can cause breakage in the code that used secp256k1_* variables from bitcointx.core.secp256k1. Three functions are introduced in this module instead: get_secp256k1_handle(), get_secp256k1_contexts() and get_secp256k1capabilities(). Those interested in using them (which is expected to be very few), please look into the source code of the module.

The changes (including of course the witness_version() fix) are in the https://github.com/Simplexum/python-bitcointx/tree/fix_witness_version

If you can test this changes, that would be great

This currently breaks JoinMarket, will look at it. If others want to take a look, here's JM changes to install this branch of python-bitcointx. kristapsk/joinmarket-clientserver@2aa06a3

[dgpv]

Can you tell me how it breaks ? Maybe I can adjust the code to make it more backward-compatible

[kristapsk]

Can you tell me how it breaks ? Maybe I can adjust the code to make it more backward-compatible

ImportError while importing test module '/home/runner/work/joinmarket-clientserver/joinmarket-clientserver/test/unified/test_segwit.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/unified/test_segwit.py:5: in <module>
    from common import make_wallets
test/unified/common.py:14: in <module>
    from jmclient import open_test_wallet_maybe, BIP32Wallet, SegwitWallet, \
src/jmclient/__init__.py:4: in <module>
    from .support import (calc_cj_fee, choose_sweep_orders, choose_orders,
src/jmclient/support.py:5: in <module>
    from .configure import get_bondless_makers_allowance
src/jmclient/configure.py:13: in <module>
    import jmbitcoin as btc
src/jmbitcoin/__init__.py:18: in <module>
    from jmbitcoin.secp256k1_main import *
src/jmbitcoin/secp256k1_main.py:9: in <module>
    from bitcointx.core.secp256k1 import secp256k1_context_verify
E   ImportError: cannot import name 'secp256k1_context_verify' from 'bitcointx.core.secp256k1' (/home/runner/work/joinmarket-clientserver/joinmarket-clientserver/jmvenv/lib/python3.11/site-packages/bitcointx/core/secp256k1.py)

[dgpv]

Ah, yes, this is the expected breakage. It seems that your project uses secp256k1 module directly. In the current code (edit: this is changed already, look in messages below), to get the library handle, you need to call get_secp256k1_handle(), and to get the contexts, you call get_secp256k1_contexts(), which returns the dataclass with two fields: sign and verify, which contain ctypes pointers to the contexts created with the library handle that get_secp256k1_handle() returns.

I am now thinking that maybe combining this into one function is better - just return the class that will contain the handle, contexts, and capabilities together. The fact that load_secp256k1_library() returns the handle that was not stored inside the module, while get_secp256k1_contexts() return the contexts of the handle that was stored is inconsistent. It makes sense to combine these into one, and remove the inconsistency

I will soon update the branch with changes. Yes, there will be breakage, but this is for the better.

[dgpv]

I have updated the code in fix_witness_version branch. I have added Secp256k1 class with lib, ctx, cap fields.

Please look into this commit cda4b6b to understand how to work with secp256k1 from python-bitcointx from now on.

Any comments on the changes are welcome.

[dgpv]

I have released version 1.1.5 with the fixes and changes. Closing this.

[AdamISZ]

@kristapsk I'll make a branch to update JM for the new API from this version of python-bitcointx. This only affects PoDLE calculation in secp256k1_main.py, I believe.