Default selection of field/scalar inverse implementation #331

1. 
   chfast commented at 8:55 pm on October 1, 2015: none
2. Default selection of field/scalar inverse implementation. 9cc0ad046a
3. 
   apoelstra commented at 3:02 pm on October 2, 2015: contributor

   Right now isn’t the same logic in configure.ac? I wouldn’t mind moving it from there, but I also don’t see a strong reason to .. what is the motivation for this patch?
4. 
   chfast commented at 3:09 pm on October 2, 2015: none

   Might be. That’s actually my point. I’m not using autoconf for building the project so logic in configure.ac is not useful to me. I would like to move as much of that logic to C code as possible. What do you think?
5. 
   apoelstra commented at 3:16 pm on October 2, 2015: contributor

   I have no strong feelings either way. I think that once #290 gets in these variables should go away (since we’ll always have a nontrivial num.h implementation), but we don’t really have a timeline for that right now. In the meantime I’ll defer to whoever decided to add these variables in the first place, as opposed to say, using USE_NUM_GMP and friends directly.

   Regardless of concept ACK/NAK, I think you should change this patch to (a) remove the now-redundant logic from configure.ac, and (b) eliminate USE_FIELD_INV_BUILTIN and USE_FIELD_INV_NUM and just use !defined(USE_NUM_NONE) directly in the inverse functions.

6. 
   chfast commented at 3:19 pm on October 2, 2015: none

   Thanks. I will wait for other opinions.

   I can change as you suggested, but in such case we will loose USE_NUM_GMP + USE_*_INV_BUILTIN configs. I don’t care, but it might be good for testing.

7. 
   gmaxwell commented at 11:45 pm on October 3, 2015: contributor

   I’m not fond of where this is done (in the middle of function implementations and repeated), but moreover– I don’t think it’s a great idea. There is a list of things the build system must set. Having one more or less doesn’t really change anything. We could document more clearly what must be set, and how– and I think that would accomplish a lot more towards both making things easy and making sure things are setup sensibly.
8. 
   sipa commented at 2:47 pm on October 11, 2015: contributor

   @theuni Opinion?
9. 
   theuni commented at 4:58 pm on October 12, 2015: contributor

   I’m really not a fan of that logic. I’ve had very bad experiences in the past with overriding autoconf variables. It leads to spaghetti very quickly.

   If anything, I’d prefer to add a wrapper for libsecp256k1-config.h that takes advantage of autoconf vars when possible, and assumes a lack of support unless explicitly defined outside of autotools. Something like:

    0diff --git a/configure.ac b/configure.ac
    1index 786d8dc..0f046f7 100644
    2--- a/configure.ac
    3+++ b/configure.ac
    4@@ -275,14 +275,6 @@ esac
    5 case $set_bignum in
    6 gmp)
    7   AC_DEFINE(HAVE_LIBGMP, 1, [Define this symbol if libgmp is installed])
    8-  AC_DEFINE(USE_NUM_GMP, 1, [Define this symbol to use the gmp implementation for num])
    9-  AC_DEFINE(USE_FIELD_INV_NUM, 1, [Define this symbol to use the num-based field inverse implementation])
   10-  AC_DEFINE(USE_SCALAR_INV_NUM, 1, [Define this symbol to use the num-based scalar inverse implementation])
   11-  ;;
   12-no)
   13-  AC_DEFINE(USE_NUM_NONE, 1, [Define this symbol to use no num implementation])
   14-  AC_DEFINE(USE_FIELD_INV_BUILTIN, 1, [Define this symbol to use the native field inverse implementation])
   15-  AC_DEFINE(USE_SCALAR_INV_BUILTIN, 1, [Define this symbol to use the native scalar inverse implementation])
   16   ;;
   17 *)
   18   AC_MSG_ERROR([invalid bignum implementation])
   19diff --git a/src/libsecp256k1-vars.h b/src/libsecp256k1-vars.h
   20new file mode 100644
   21index 0000000..646b81d
   22--- /dev/null
   23+++ b/src/libsecp256k1-vars.h
   24@@ -0,0 +1,18 @@
   25+#ifndef LIBSECP256K1_VARS_H
   26+#define LIBSECP256K1_VARS_H
   27+
   28+#if defined HAVE_CONFIG_H
   29+#include "libsecp256k1-config.h"
   30+#endif
   31+
   32+#if defined(HAVE_LIBGMP)
   33+#define USE_NUM_GMP 1
   34+#define USE_FIELD_INV_NUM 1
   35+#define USE_SCALAR_INV_NUM 1
   36+#else
   37+#define USE_NUM_NONE 1
   38+#define USE_FIELD_INV_BUILTIN 1
   39+#define USE_SCALAR_INV_BUILTIN 1
   40+#endif
   41+
   42+#endif
   

   Then, all current code would include “libsecp256k1-vars.h” instead of “libsecp256k1-config.h”. Non-autotools projects would -DHAVE_LIBGMP to indicate use of the lib.

10. 
    sipa commented at 10:06 pm on November 1, 2015: contributor

    Closing this, but feel free to implement @theuni’s suggestion.
11. sipa closed this on Nov 1, 2015

    ---

Contributors
chfast apoelstra gmaxwell sipa theuni