Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export sizeof(secp256k1_surjectionproof) as SECP256K1_SURJECTIONPROOF_RAW_SIZE #37

Closed

Conversation

Projects
None yet
4 participants
@dgpv
Copy link
Contributor

commented Dec 24, 2018

the rationale is explained in #36 - knowing the struct size used in particular version of the library enables to alloc the struct without access to secp256k1_surjectionproof.h

For example, with python:

>>> import ctypes
>>> secp256k1 = ctypes.cdll.LoadLibrary('.libs/libsecp256k1.so')
>>> surjectionproof_size = ctypes.c_int.in_dll(secp256k1, 'SECP256K1_SURJECTIONPROOF_RAW_SIZE').value
>>> surjectionproof_size
8264
>>> proof = ctypes.create_string_buffer(surjectionproof_size)

# secp256k1.secp256k1_surjectionproof_initialize(secp256k1_blind_context, proof, ...)

@dgpv dgpv force-pushed the dgpv:add_surjectionproof_size_symbol branch 2 times, most recently from b912c57 to 2f69f4b Dec 24, 2018

@dgpv dgpv changed the title export sizeof(secp256k1_surjectionproof) as SECP256K1_SURJECTIONPROOF_SIZE export sizeof(secp256k1_surjectionproof) as SECP256K1_SURJECTIONPROOF_RAW_SIZE Dec 24, 2018

@instagibbs

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2018

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

If the library authors decide that this is better done with a function that returns the size, I will submit a PR for that. I am preparing to create a release of python-bitcointx with support for Elements sidechain, but having a requirement to apply the patch to the secp256k1-zkp source before using python-bitcointx for Elements is not very convenient. I would appreciate a comment from the authors about how they think this should be handled - I will submit the code according to that.

@apoelstra

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

@apoelstra

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

We had some comment on IRC and weren't clear on the motivation for this; how are you using this struct at all without the include file?

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

We had some comment on IRC and weren't clear on the motivation for this

I ported the transaction blinding code from C++ in Elements (https://github.com/ElementsProject/elements/blob/7f6351782a1555696e69d5ffc5304005a07876cd/src/blind.cpp#L224)
to python (https://github.com/Simplexum/python-bitcointx/blob/31f95df76ae163a5139ef0bf6a9ec556c9c09cfd/bitcointx/sidechain/elements.py#L777)

In SurjectOutput(), the secp256k1_surjectionproof structure is allocated on the stack, and then passed to secp256k1_surjectionproof_initialize(). To port this code cleanly to python, I needed to know the size of the structure.

how are you using this struct at all without the include file?

By using the patched library, the patch is in this PR.

In this PR, this is done by exporting a special symbol that is statically assigned with
const int SECP256K1_SURJECTIONPROOF_RAW_SIZE = sizeof(secp256k1_surjectionproof);

Here is the lines in python code that does the allocation using this symbol: https://github.com/Simplexum/python-bitcointx/blob/31f95df76ae163a5139ef0bf6a9ec556c9c09cfd/bitcointx/sidechain/elements.py#L1473

@dgpv dgpv force-pushed the dgpv:add_surjectionproof_size_symbol branch from 2f69f4b to d8babd7 Feb 24, 2019

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

rebased on current tip of secp256k1-zkp branch.

@jonasnick

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

If I understand correctly @apoelstra is asking because #36 can be interpreted as you're not using the include file. But it seems more like you are using it but you're concerned that the include file might be inconsistent with the (dynamically loaded) library. If that's the case I think it makes more sense to ship it with your code, link it statically or otherwise ensure that include file and library are consistent (as is the case with other libraries not only libsecp). If the files are inconsistent you might have more problems than just a different surjectionproof size.

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

TL;DR: Having build-time-defined value only in header file forces projects to use static linking or bundle the lib source with a project. This makes sense when the library is in consensus-critical code path, but not in other uses.


It is normal to use dymanically loaded library without the header, especially if you are using the library through FFI of some higher-level language. Linux distos usually ship headers in a separate *-dev package or the like. If you have certain values defined in a header file (that are part of the API), you just take this constants and copy to your higher-level language library code. For example, secp256k1_generator struct in include/secp256k1_generator.h is guaranteed to be 64 bytes in size (as per the comments in the file), so I can use SECP256K1_GENERATOR_SIZE = 64 in python code.

If there is some value that should not be copied verbatim from the API header file, there is usually a function to get the needed value (if it is calculated at runtime), or exported symbol with the said value (if it is defined at build-time), or some other way, but the idea is that you do not need the header file at runtime. Otherwise, the purpose of dynamic library is lost for any languages beside C/C++ (or maybe a couple of other langs that can parse C headers natively) and for systems without a compiler installed. Everyone would need to include their copy of the library with known build-time values. This might be OK for commercial software distribution, where you probably shipping everything as a bundle, but if this is a dynamic library that is going to be included in distros, then the functions that are part of the API should be usable without headers, in my opinion. (and for example secp256k1 is inclued in many distros, with headers in separate secp256k1-dev package in debian)

Let's say I add C extension to python-bitcointx package with the only purpose of getting this value from the header. Then:

  1. to install python-bitcointx on your system you would need C compiler installed - which is not always true even for linux systems
  2. you install python-bitcointx with this C extenstion that gets built at the time of installation, and then upgrade secp256k1-zkp on your system, and this particular value is changed in new version - you will probably get memory corruption bugs without knowing whats going on - because the extension was not rebuilt.

So the only options without having this value exposed at runtime seems to be:

  1. Just allocate a lot of space and hope sizeof(secp256k1_surjectionproof) won't ever be larger than 100K bytes, for example (terrible option)
  2. Include full secp256k1-zkp library with python-bitcointx package, build it at package installation time, and use this particular copy (bad option - requires compiler, and if there's bug in secp256k1-zkp you now have to upgrade the included version, in addition to system-installed version)
  3. Include pre-compiled library (very bad option - shipping binary executables that are hard to audit, and the same problem with upgrades)
  4. Require a patch that exposes the value to be applied to the secp256k1-zkp before it can be used by python-bitcointx (almost as bad as option 2, but you can have the system-installed library patched. This will still be manual and inconvenient process)

These breakage problems are not unique to python module, but is also present for C/C++ projects that do not include the library along with the distribution or statically link with it. If the code is not compatible with dynamic library, dynamic loader should not be able to load the library into the executable (version mismatch or symbol not found etc). If dynamic linker is able to link two incompatible code chunks, this will happen sooner or later. So having such a (non-totally-constant) value only in header file forces projects to use static linking or bundle the lib source with a project. This makes sense when the library is in consensus-critical code path, but not in other uses.

@jonasnick

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Sorry for the late reply and thanks for making your case so clearly. There was some discussion about your approach in #secp256k1 on freenode and I suggest you bring up your proposal there because I'm not knowledgeable enough in this area to mediate.

Fwiw I've had to overcome a similar problem when writing a rust bindings for MuSig including the the MuSig session type. Instead of allocating as many bytes as the size of a session, in rust it's possible to recreate the struct with the same layout as in C (see https://github.com/jonasnick/rust-secp256k1-zkp/blob/dea9e50b41bd70aae6cd705892f06e722755976d/src/ffi.rs#L39, that's what #[repr(C)] does). This is definitily more tedious, but has the advantage that the whole thing can be allocated on the stack (which is a design goal due to hardware wallet compatibility). I'm not sure if the same can be achieved with python.

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

There was some discussion about your approach in #secp256k1 on freenode and I suggest you bring up your proposal there

OK, noted.

in rust it's possible to recreate the struct with the same layout as in C

This is only marginally better than just measuring the size of the struct by printf("%d\n", sizeof(secp256k1_surjectionproof); and using the resulting value in your code. This is because the value can change at any moment in the future, and you will need to always track the changes. The size depends on the constant and compilation option (SECP256K1_SURJECTIONPROOF_MAX_N_INPUTS, #ifdef VERIFY).
There is no guarantee that the max_n_inputs constant won't be changed, other constants/#ifdefs would not be added, or new fields would not be added to this struct, to accomodate some new features/requirements. You would need to track that and adjust your code. At this point, getting new size and just using it would be simpler.

And then you have to make sure that old version of your code (which uses smaller size for the struct) is never used with new version of the library. Otherwise you will get memory corruption.

This approach works only if the exact binary struct layout is strictly defined, and is part of the protocol/API.

dgpv added some commits Dec 24, 2018

make SECP256K1_SURJECTIONPROOF_RAW_SIZE both exported symbol and #define
This will allow to use the same name both for stack allocations
and to get this size via external symbol, bo be used in FFI scenarios

@dgpv dgpv force-pushed the dgpv:add_surjectionproof_size_symbol branch from d8babd7 to 353c33c Apr 17, 2019

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Changed the approach a bit: in the header, I do

#define SECP256K1_SURJECTIONPROOF_RAW_SIZE sizeof(secp256k1_surjectionproof)

while in secp256k1.c, I #undef that name, and then declare the const variable that would be exported:

#undef SECP256K1_SURJECTIONPROOF_RAW_SIZE
SECP256K1_API const int SECP256K1_SURJECTIONPROOF_RAW_SIZE = sizeof(secp256k1_surjectionproof);

This allows for the name SECP256K1_SURJECTIONPROOF_RAW_SIZE to be used both for stack-based allocations from C code, and for dynamic allocations via external symbol from various FFI.

@dgpv

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

After conversation on #secp256k1 channel it was determined that better approach is to introduce create/destroy functions for this struct, and the create function would also call initialize function, so the caller won't need to do extra call to init the struct. The code that wants to allocate the struct on the stack would still be able to do so, and would need to call initialize function directly, as before.

this PR is therefore superseded by #55

I'm closing this PR.

@dgpv dgpv closed this Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.