ElementsProject / libwally-core

Useful primitives for wallets
Other
280 stars 134 forks source link

Updates to ABI handling in preparation for semver #409

Closed jgriffiths closed 9 months ago

jgriffiths commented 11 months ago

Motivating issue: https://github.com/ElementsProject/libwally-core/issues/408

Initially wally was intended to always be included in the source tree along with the app that used it, or at least to be statically linked with the using app. Notably, wally can be built with or without Elements support, and whether it is enabled changed the ABI in two ways: (1) the exposed library structures have Elements-specific fields for Elements builds and (2) Elements-specific functions and internal functionality were only defined if the caller defined BUILD_ELEMENTS when compiling and including the headers from the library.

Note that wally itself is always statically linked with the libsecp256k1 that it uses; by default this is the Blockstream zkp branch which exposes further APIs used by Elements.

For the Python and Javascript packages available through PyPI and npm respectively, wally is always released with Elements support enabled. This allows users to know that all the functionality they may need will be available out of the box.

However, at least one distro (Gentoo) is now installing wally as a global shared library that is re-used by several components. Users may customize the library build and may decide to e.g. disable Elements support when installing. This poses a problem for apps that expect this functionality to run or can optionally run with it present; Because the ABI changes for each build type such apps may either corrupt memory (because expected structure sizes differ between the app and library) or will fail to start (because the linker cannot resolve the Elements calls the app expects).

This PR addresses these issues as follows:

  1. The library version number is now exposed at compile time via WALLY_MAJOR_VER/WALLY_MINOR_VER/WALLY_PATCH_VER/WALLY_BUILD_VER and at runtime by calling wally_get_build_version().
  2. Elements is now enabled by default when building. Users do not need to pass --enable-elements to configure or define BUILD_ELEMENTS when including library headers.
  3. Users can build without Elements support explicitly by passing --disable-elements to configure. This configuration is ABI compatible with non-Elements builds; all structures are identical and all Elements APIs are present (they return WALLY_ERROR when called, and their actual implementation is compiled out of the library). When built this way, wally can be installed as a global shared library and apps can gracefully fail if the version does not match their expectations or if they expect Elements functionality to be present and wally_is_elements_build() returns 0.
  4. For static linking and/or embedded development where Elements functionality is not required and memory/code size is critical, it can be completely compiled out of the library by passing --disable-elements --disable-elements-abi to configure, and defining WALLY_ABI_NO_ELEMENTS when including wally headers. Such builds are ABI incompatible with standard builds under (2) or (3) and must not be installed as global shared libraries in the general case except at the users risk (for example, a BTC-only dedicated device, O/S or docker image where all software is pre-installed).
  5. With this PR wally moves to release 1.0.0 and begins following semantic versioning as per https://semver.org/.

cc: @rustyrussell @k-matsuzawa @Sjors @whitslack - Please review and if possible test these changes and report any issues here.

Fixes https://github.com/ElementsProject/libwally-core/issues/408 Fixes https://github.com/ElementsProject/libwally-core/issues/388

jgriffiths commented 11 months ago

@whitslack note this change also exposes the transaction accessor functions which were the cause of many build failures with various configure options. I would really appreciate it if you could test the new options, thanks!

whitslack commented 11 months ago

@jgriffiths: Thanks for your work on this. I greatly appreciate it, both stabilizing the ABI w.r.t. build options and moving to semantic versioning.

I'll start working on a Gentoo ebuild for 1.0.0 today and will report back with my findings.

jgriffiths commented 11 months ago

Updated and rebased over https://github.com/ElementsProject/libwally-core/pull/410 which has been merged first.

whitslack commented 11 months ago

@jgriffiths: I'm seeing some test failures (at 0a915e9b0bef8b70d10e8d6a93b779e104f90101)…

PYTHONDONTWRITEBYTECODE=1 python3.12 test/test_map.py
.E.
======================================================================
ERROR: test_map (__main__.MapTests.test_map)
Test map functions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_map.py", line 85, in test_map
    self.assertEqual(m.contents.items[n-1].value_len, vl)
                     ~~~~~~~~~~~~~~~~^^^^^
ValueError: NULL pointer access

----------------------------------------------------------------------
Ran 3 tests in 0.004s

FAILED (errors=1)
PYTHONDONTWRITEBYTECODE=1 python3.12 test/test_psbt.py
......EE..F
======================================================================
ERROR: test_psbt (__main__.PSBTTests.test_psbt)
Test creating and modifying various PSBT fields
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_psbt.py", line 266, in test_psbt
    ret = wally_psbt_input_set_required_lockheight(psbt.contents.inputs[0], 499999999)
                                                   ~~~~~~~~~~~~~~~~~~~~^^^
ValueError: NULL pointer access

======================================================================
ERROR: test_redundant (__main__.PSBTTests.test_redundant)
Test serializing redundant finalized input information
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_psbt.py", line 360, in test_redundant
    ret = wally_psbt_input_set_redeem_script(psbt.contents.inputs[0], redeem, redeem_len)
                                             ~~~~~~~~~~~~~~~~~~~~^^^
ValueError: NULL pointer access

======================================================================
FAIL: test_valid (__main__.PSBTTests.test_valid)
Test deserializing and roundtripping valid PSBTs
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_psbt.py", line 100, in test_valid
    self.assertEqual(pre_hex, post_hex)
AssertionError: '0100000002710ea76ab45c5cb6438e607e59cc0376[268 chars]0000' != '0200000002710ea76ab45c5cb6438e607e59cc0376[268 chars]0000'
Diff is 645 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 11 tests in 0.042s

FAILED (failures=1, errors=2)
PYTHONDONTWRITEBYTECODE=1 python3.12 test/test_transaction.py
.......F.
======================================================================
FAIL: test_serialization (__main__.TransactionTests.test_serialization)
Testing serialization and deserialization
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_transaction.py", line 85, in test_serialization
    self.assertEqual(utf8(self.tx_serialize_hex(tx_copy)),
AssertionError: b'010[74 chars]000008b483045022100da43201760bda697222002f5626[321 chars]0000' != b'010[74 chars]0000000ffffffff0123ce0100000000001976a9142bc89[43 chars]0000'

----------------------------------------------------------------------
Ran 9 tests in 0.008s

FAILED (failures=1)

Interestingly, all tests pass when run on Python 3.11.5. The failures occur on Python 3.12.0rc1(p6). Releases 0.8.8, 0.9.0, 0.9.1, and 0.9.2 all exhibit the same test failures on Python 3.12. (I have not tested other releases.)

jgriffiths commented 11 months ago

@whitslack very strange! I assume this is your first time testing v3.12?

what does test map print if run with:

diff --git a/src/test/test_map.py b/src/test/test_map.py
index f74e721e..d8a00a58 100644
--- a/src/test/test_map.py
+++ b/src/test/test_map.py
@@ -81,6 +81,8 @@ class MapTests(unittest.TestCase):
             self.assertEqual(wally_map_get_item_length(m, i - 1), (WALLY_OK, vl - 1))
             # Adding an existing key ignores the new value without error.
             vl = vl if case == cases[-1] else vl - 1
+            print(n-1, m.contents)
+            print(m.contents.items, m.contents.num_items)
             self.assertEqual(m.contents.items[n-1].value_len, vl)

         # Find an integer key with no integers in the map

?

whitslack commented 11 months ago

I assume this is your first time testing v3.12?

It's possible. On my system my ebuild happens to test libwally-core using Python 3.11 when the doc USE flag is enabled because then the Python interpreter used at build time has to be one that has Sphinx, and I only install Sphinx for Python 3.11, which is currently Gentoo's default Python interpreter. If the doc USE is disabled, then there is no such requirement imposed, and the ebuild uses the newest installed Python interpreter, which is Python 3.12. I thought I had tested my libwally-core ebuild under all permutations of USE flags after I had installed a Python 3.12 release, but it is possible that I had not.

what does test map print if run with:

Under Python 3.11, I get:

PYTHONDONTWRITEBYTECODE=1 python3.11 test/test_map.py
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

OK
0 <util.wally_map object at 0x7fe6c220f5c0>
<util.LP_wally_map_item object at 0x7fe6c220f530> 1
1 <util.wally_map object at 0x7fe6c220f6e0>
<util.LP_wally_map_item object at 0x7fe6c220f5c0> 2
2 <util.wally_map object at 0x7fe6c220f530>
<util.LP_wally_map_item object at 0x7fe6c220f6e0> 3
2 <util.wally_map object at 0x7fe6c220f5c0>
<util.LP_wally_map_item object at 0x7fe6c220f530> 3
2 <util.wally_map object at 0x7fe6c220f6e0>
<util.LP_wally_map_item object at 0x7fe6c220f5c0> 3

Under Python 3.12, I get:

PYTHONDONTWRITEBYTECODE=1 python3.12 test/test_map.py
.E.
======================================================================
ERROR: test_map (__main__.MapTests.test_map)
Test map functions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/net-libs/libwally-core-1.0.0/work/libwally-core-release_1.0.0/src/test/test_map.py", line 86, in test_map
    self.assertEqual(m.contents.items[n-1].value_len, vl)
                     ~~~~~~~~~~~~~~~~^^^^^
ValueError: NULL pointer access

----------------------------------------------------------------------
Ran 3 tests in 0.004s

FAILED (errors=1)
0 <util.wally_map object at 0x7f2133afe250>
<util.LP_wally_map_item object at 0x7f2133afe4d0> 0
jgriffiths commented 11 months ago

@whitslack m.contents.num_items is not being updated under python 3.12, I think this needs debugging, I'll look into testing with that version.

whitslack commented 11 months ago

Incidentally, I exhaustively tested all allowed combinations of USE flags at 30c744cf07128afe786b5836ea19b2973cac8c1a (plus #414) using Python 3.11, and all builds were successful, so the only hurdle that I see remaining on Gentoo is this issue of test failures with Python 3.12. It's not a blocker, though, as I have already dropped python3_12 from the ebuilds' PYTHON_COMPAT lists, so users will not be able to install the wallycore Python module for Python 3.12, and the unit tests will not be run under Python 3.12, even if the user has a Python 3.12 interpreter installed.

jgriffiths commented 11 months ago

Thanks, I should have time tomorrow to review your remaining changes and will look into 3.12 asap after that.

whitslack commented 10 months ago

m.contents.num_items is not being updated under python 3.12, I think this needs debugging, I'll look into testing with that version.

@jgriffiths: All tests are passing under Python 3.12.0rc2. I guess there must have been a bug in 3.12.0rc1 that affected libwally-core.

jgriffiths commented 10 months ago

@whitslack great news. I'm slightly sidetracked working on wallet policies for Jade but will be back to this after.

jgriffiths commented 9 months ago

Merged elsewhere, closing.