This is a replacement for #1046. It contains only the changes that (I think) are unlikely to be debated and don't interfere with #995. I'd like to get this in quickly to get a green badge again. (Using 2 CPUs instead of one for valgrind should do the trick.)
I'll open another PR on top of this to optimize the matrix. This will deserve more discussion and we should get #995 merged first.
Also simplify #ifdefs in generated file.
This has been overlooked in #988.
For more background on the "greedy" flag, see https://github.com/bitcoin/bitcoin/pull/23797.
3 | @@ -4,10 +4,10 @@ env: 4 | # Specific warnings can be disabled with -Wno-error=foo. 5 | # -pedantic-errors is not equivalent to -Werror=pedantic and thus not implied by -Werror according to the GCC manual. 6 | WERROR_CFLAGS: -Werror -pedantic-errors 7 | - MAKEFLAGS: -j2 8 | + MAKEFLAGS: -j4
Does the CI server actually have 4 CPUs?
We set cpu: 1 or cpu: 2 depending on the task. But the greedy flag will occasionally give us more CPUs when available. Increasing this to -j4 makes sure we actually can make use of this.
87 | @@ -84,10 +88,7 @@ task: 88 | 89 | task: 90 | name: "i686: Linux (Debian stable)" 91 | - container: 92 | - dockerfile: ci/linux-debian.Dockerfile 93 | - cpu: 1 94 | - memory: 1G 95 | + << : *LINUX_CONTAINER
Is this a yaml trick?
Yes, we use this in other places, too.
Found it: https://yaml.org/type/merge.html This is interesting, I did not know yaml has this sort of things
Yeah, core uses it all over the place, so we're in good company.
134 | @@ -134,8 +135,9 @@ task: 135 | ## - rm /tmp/.com.apple.dt.CommandLineTools.installondemand.in-progress 136 | ## 137 | brew_valgrind_pre_script: 138 | + - brew update 139 | - brew config 140 | - - brew tap --shallow LouisBrunner/valgrind 141 | + - brew tap LouisBrunner/valgrind
Why not shallow?
Shallow doesn't exist anymore in recent brew versions, see the commit message.
Manually checked that the ECMULT_TABLE_SIZE changes are correct according to (1 << (w-2))
utACK 965f9480c417254318a3093936ee9748391b49b1 The C file changes are simple and correct, and it's great to test different window sizes
75 | - env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETEST: no, BENCH: no}
76 | - env: {CPPFLAGS: -DDETERMINISTIC}
77 | - env: {CFLAGS: -O0, CTIMETEST: no}
78 | - - env: { ECMULTGENPRECISION: 2 }
79 | - - env: { ECMULTGENPRECISION: 8 }
80 | + - env: { ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
Better to check the edges? so ECMULTWINDOW: 2?
Indeed, let me just change to checking 2 (as edge case) and 4, which is a more reasonable value because it still needs almost no memory and may be a candidate for a "lowmem" setting here we may introduce in the future. (Rust bindings do the same, see https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/build.rs#L45)
fixed with force-push
The preinstalled brew is very old and tries to download prebuilt bottles
from a server which is no longer available. Because that will fail, brew
falls back to building our dependencies (e.g., autotools) from source,
which takes very long.
This commit makes sure that brew is updated before we start the build.
We also need to remove the `--shallow` argument from `brew tap`. It
doesn't exist in recent brew versions.
... and increase the memory only for UBSan, ASan, LSan builds. Those are
the ones who need more memory.
ACK b4ac1a1d5f4d51b9836ac310b78bc9d4256580c2
utACK b4ac1a1d5f4d51b9836ac310b78bc9d4256580c2
Posthumous ACK b4ac1a1d5f4d51b9836ac310b78bc9d4256580c2
56 | @@ -57,6 +57,8 @@ static void print_two_tables(FILE *fp, int window_g) { 57 | } 58 | 59 | int main(void) { 60 | + /* Always compute all tables for window sizes up to 15. */ 61 | + int window_g = (ECMULT_WINDOW_SIZE < 15) ? 15 : ECMULT_WINDOW_SIZE;
78 | @@ -77,15 +79,15 @@ int main(void) {
79 | fprintf(fp, "#include \"ecmult.h\"\n");
80 | fprintf(fp, "#include \"precomputed_ecmult.h\"\n");
81 | fprintf(fp, "#define S(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p) SECP256K1_GE_STORAGE_CONST(0x##a##u,0x##b##u,0x##c##u,0x##d##u,0x##e##u,0x##f##u,0x##g##u,0x##h##u,0x##i##u,0x##j##u,0x##k##u,0x##l##u,0x##m##u,0x##n##u,0x##o##u,0x##p##u)\n");
78 | @@ -77,15 +79,15 @@ int main(void) { 79 | fprintf(fp, "#include \"ecmult.h\"\n"); 80 | fprintf(fp, "#include \"precomputed_ecmult.h\"\n"); 81 | fprintf(fp, "#define S(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p) SECP256K1_GE_STORAGE_CONST(0x##a##u,0x##b##u,0x##c##u,0x##d##u,0x##e##u,0x##f##u,0x##g##u,0x##h##u,0x##i##u,0x##j##u,0x##k##u,0x##l##u,0x##m##u,0x##n##u,0x##o##u,0x##p##u)\n"); 82 | - fprintf(fp, "#if ECMULT_TABLE_SIZE(ECMULT_WINDOW_SIZE) > %ld\n", ECMULT_TABLE_SIZE(ECMULT_WINDOW_SIZE));
78 | @@ -77,15 +79,15 @@ int main(void) { 79 | fprintf(fp, "#include \"ecmult.h\"\n"); 80 | fprintf(fp, "#include \"precomputed_ecmult.h\"\n"); 81 | fprintf(fp, "#define S(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p) SECP256K1_GE_STORAGE_CONST(0x##a##u,0x##b##u,0x##c##u,0x##d##u,0x##e##u,0x##f##u,0x##g##u,0x##h##u,0x##i##u,0x##j##u,0x##k##u,0x##l##u,0x##m##u,0x##n##u,0x##o##u,0x##p##u)\n"); 82 | - fprintf(fp, "#if ECMULT_TABLE_SIZE(ECMULT_WINDOW_SIZE) > %ld\n", ECMULT_TABLE_SIZE(ECMULT_WINDOW_SIZE)); 83 | + fprintf(fp, "#if ECMULT_WINDOW_SIZE > %d\n", window_g);
__
86 | fprintf(fp, "#ifdef EXHAUSTIVE_TEST_ORDER\n");
87 | fprintf(fp, "# error Cannot compile precomputed_ecmult.c in exhaustive test mode\n");
88 | fprintf(fp, "#endif /* EXHAUSTIVE_TEST_ORDER */\n");
89 | fprintf(fp, "#define WINDOW_G ECMULT_WINDOW_SIZE\n");
90 |
91 | - print_two_tables(fp, ECMULT_WINDOW_SIZE);
00
@
78 | @@ -77,15 +79,15 @@ int main(void) {
79 | fprintf(fp, "#include \"ecmult.h\"\n");
ecmult.h is no longer needed if you remove ECMULT_TABLE_SIZE.
I think it's correct to use ECMULT_TABLE_SIZE.
Right sorry. For some reason I thought all the users of ECMULT_TABLE_SIZE were eliminated, but that is not true.