v0.3.0 slightly modified the way modules are enabled in configure.ac.
We integrated v0.3.0 into frost on 2023-11-21, with b701c4abaf0e, but we forgot to make our FROST_SPECIFIC code in configure.ac conformant to the new convention.
We do it now.
This is not a breaking change: both old and new versions should still be correct.
Secp256k1 version 44c2452fd387 is the one that got into bitcoin v24.0 and v24.1.
It got there via https://github.com/bitcoin/bitcoin/commit/913b1f2a5eb2ee5cd02113791764b23ded4c1c94
Between secp256k1 0559fc6e41b6 and 44c2452fd387 the code that enabled the
various submodules got slightly changed.
This commit ports those changes to the frost module, too.
The upstream function secp256k1_ge_set_xo_var() in src/group_impl.h returns an
int signalling an error condition, but it is not marked with
SECP256K1_WARN_UNUSED_RESULT.
Thus, when compiling the Frost module, no warning was generated when
deserialize_frost_signature() had no check covering the case that
secp256k1_ge_set_xo_var() could fail.
The GCC static analyzer found two potential execution paths in
secp256k1_frost_verify() that would lead to undefined behaviour. For brevity,
the issues are not reported here, but only in the Github PR associated with
this commit (https://github.com/bancaditalia/secp256k1-frost/pull/5).
As of today (2023-05-29), in bitcoin-core secp256k1, secp256k1_ge_set_xo_var()
is still not marked SECP256K1_WARN_UNUSED_RESULT, see:
https://github.com/bitcoin-core/secp256k1/blob/908e02d596b66203788e8945b1f9c93ff28a4536/src/group_impl.h#L280
It may make sense to propose upstream to add that annotation.
In order to replicate the issues fixed by this commit, compile with:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The leak presented if compute_binding_factors() failed. In that case the error
reporting path would not deallocate the binding factor that were allocated just
above.
Found with gcc 13.1 via:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The analyzer found 3 leaks at main_impl.h:1403, all of them solved by this
change. The branch analysis of the first leak was:
```
In file included from src/secp256k1.c:767:
src/modules/frost/main_impl.h: In function 'secp256k1_frost_aggregate':
src/modules/frost/main_impl.h:1454:1: warning: leak of 'binding_factors.binding_factors' [CWE-401] [-Wanalyzer-malloc-leak]
1454 | }
| ^
'secp256k1_frost_aggregate': events 1-2
|
| 1365 | SECP256K1_API int secp256k1_frost_aggregate(const secp256k1_context *ctx,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to 'secp256k1_frost_aggregate'
[...]
| 1394 | binding_factors.binding_factors = (secp256k1_scalar *)
| 1395 | checked_malloc(&default_error_callback, num_signers * sizeof(secp256k1_scalar));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (10) calling 'checked_malloc' from 'secp256k1_frost_aggregate'
|
+--> 'checked_malloc': events 11-15
|
|src/util.h:118:31:
| 118 | static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
| | ^~~~~~~~~~~~~~
| | |
| | (11) entry to 'checked_malloc'
| 119 | void *ret = malloc(size);
| | ~~~~~~~~~~~~
| | |
| | (12) allocated here
[...]
'secp256k1_frost_aggregate': events 29-30
|
| 1403 | if (compute_binding_factors(&binding_factors, msg32, 32, num_signers, commitments) == 0) {
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (29) ...to here
|......
| 1454 | }
| | ~
| | |
| | (30) 'binding_factors.binding_factors' leaks here; was allocated at (12)
|
```
In the next commit, this will allow us to call it from inside
secp256k1_frost_sign().
No functional changes.
In this way we have a single way of clearing binding factors across the code
base. Before this change, secp256k1_frost_sign() was the only function that
rolled its own code.
No functional changes.
A reference per the pkg-config sytax can be found in "man 5 pc", or online at
https://man.freebsd.org/cgi/man.cgi?query=pc&sektion=5&n=1
EXAMPLES
An example .pc file:
# This is a comment
prefix=/home/kaniini/pkg # this defines a variable
exec_prefix=${prefix} # defining another variable with a substitution
libdir=${exec_prefix}/lib
includedir=${prefix}/include
Name: libfoo # human-readable name
Description: an example library called libfoo # human-readable description
Version: 1.0
URL: http://www.pkgconf.org
Requires: libbar > 2.0.0
Conflicts: libbaz <= 3.0.0
Libs: -L${libdir} -lfoo
Libs.private: -lm
Cflags: -I${includedir}/libfoo
As per C89 standard (https://www.open-std.org/JTC1/sc22/wg14/www/docs/n1256.pdf,
section 6.5.3 "Unary operators") we are only required to parenthesize a sizeof()
call if the argument is a type name, which is not the case here.
However, it is less surprising to err on the side of caution, and uniformly
enclose in parentheses every sizeof() invocation across the code base.
No functional changes.
Given that - at least for now - we are allocating memory dynamically, let's free
it as soon as it is no longer needed.
Valgrind shows no regressions.
Remove rho_input as output variable of compute_binding_factor.
Currently, rho_input is not used outside compute_binding_factor, so we avoid
keeping unuseful data on the stack.
Note: the official test vectors of FROST by IETF include tests on rho_input.
Eventually, rho_input will be introduced again.
Two test runs are performed:
1. Build with coverage enabled and no -Werror; run tests; generate summary of
code coverage
2. Build with coverage disabled and using -Werror; run tests
Enabling converage translates in compiling additional code; the different
build configurations aim at checking that code changes do not introduce
regressions.
In order to save time on dependency installation the tests are performed
sequentially in a single job.
This mimics what has been done in itcoin-core at
https://github.com/bancaditalia/itcoin-core/commit/a6d27e63c8ed22694df293163e51a8e3e8748a13
Update official secp256k1 library taking bitcoin changes (from v23 to v24) and accordingly update the frost module integration. Pull request #3 introduced this merge commit.
The previous name was a leftover from a previous version. Now a single workflow
performs both:
- tests with -Werror and no coverage
- tests with coverage and no -Werror
As of current secp version, enabling coverage spits out some warnings, and
prevents us to use -Werror everywhere.
This CI workflow builds the minimum code necessary to run the Frost test suite
and the Frost example, and runs them under Valgrind, exiting if it detects any
problems.
The example is run first because it takes way less time.
Closes #19
Closes #20
In v0.3.0, secp256k1 also added support for building via CMake. Let's add a CI
workflow to exercise it.
Co-authored-by: Antonio Muci <antonio.muci@bancaditalia.it>
Closes #21
Closes #23
Closes #18
Closes #24
Workflow verify-version-consistency was the last one written, and already used
checkout@v4.
Let's align all the remaining workflows to use the most recent action version.
No functional changes.
This should be a very low risk change, since the only CI job that directly uses
the base image is .github/workflows/verify-version-consistency.yml, which only
depends on bash.
All the other workflows run in containers and should be unaffected; we will
update the base image in the next commit.
Installed software on the two platforms:
- https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md
- https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md
v0.3.0 slightly modified the way modules are enabled. We integrated v0.3.0 into
frost on 2023-11-21, with b701c4abaf0e, but we forgot to update the new
convention our FROST_SPECIFIC code in configure.ac. We do it now.
This is not a breaking change: both old and new versions should still be correct.