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

build: Replace hardcoded "auto" value with default one #1535

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 27, 2024

"auto" implies that a value is being chosen based on build system introspection or host system capabilities. However, for the --with-ecmult-window and --with-ecmult-gen-kb options, the values "auto" are hardcoded, which might lead to confusion.

This PR replaces "auto" with more appropriate default values.

If Concept ACKed, I'll add equivalent commits for CMake.

"auto" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`--with-ecmult-window` option, the value "auto" is hardcoded, which
might lead to confusion.

This change replaces "auto" with a more appropriate default value.
"auto" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`--with-ecmult-gen-kb` option, the value "auto" is hardcoded, which
might lead to confusion.

This change replaces "auto" with a more appropriate default value.
@real-or-random
Copy link
Contributor

Concept ACK from my side

I think having a default is good enough, we don't need two layers of indirection (default -> auto -> some number).

"AUTO" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`SECP256K1_ECMULT_WINDOW_SIZE` option, the value "AUTO" is hardcoded,
which might lead to confusion.

This change replaces "AUTO" with a more appropriate default value.
"AUTO" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`SECP256K1_ECMULT_GEN_KB` option, the value "AUTO" is hardcoded, which
might lead to confusion.

This change replaces "AUTO" with a more appropriate default value.
@hebasto
Copy link
Member Author

hebasto commented May 27, 2024

If Concept ACKed, I'll add equivalent commits for CMake.

The equivalent commits for CMake have been added.

@real-or-random real-or-random changed the title autotools: Replace hardcoded "auto" value with default one build: Replace hardcoded "auto" value with default one May 27, 2024
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4d9645b good from my side, but let's see if we can get more (Concept) ACKs

I think we'd still want to have something like profiles (e.g., "desktop", "embedded", "minimal") eventually, but this change makes sense independently of this.

@bitcoin-core bitcoin-core deleted a comment from Nichebiche Jun 13, 2024
@fanquake
Copy link
Member

Concept ACK

@sipa
Copy link
Contributor

sipa commented Jun 25, 2024

utACK 4d9645b

@real-or-random real-or-random merged commit 4af241b into bitcoin-core:master Jun 25, 2024
108 checks passed
@hebasto hebasto deleted the 240527-auto branch June 25, 2024 14:02
@real-or-random
Copy link
Contributor

real-or-random commented Jun 25, 2024

Does this need a changelog entry? Strictly speaking yes because this removes a valid config option. But pragmatically, I tend to say no: the breakage occurs only for people who passed =auto (the default) explicitly, so this should hardly affect anyone.

edit: Let me just add the label, we can revisit this when we do the release.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Jun 25, 2024
4af241b320 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one
f473c959f0 Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project
d403eea484 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach
d7ae25ce6f Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c
0e2fadb20c fix: typos in secp256k1.c
69b2192ad4 Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
5dd637f3cf Merge bitcoin-core/secp256k1#1548: README: mention ellswift module
7454a53736 README: mention ellswift module
4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach
c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`
f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
158f9e5eae cmake: Do not modify build types when integrating by downstream project
35c0fdc86b Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project
4392f0f717 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (bitcoin#1491)
bedffd53d8 Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job
4b8d5eeacf Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests
f55703ba49 autotools: Delete unneeded compiler test
396e885886 autotools: Align MSan checking code with CMake's implementation
abde59f52d cmake: Report more compiler details in summary
7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
4d9645bee0 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option
a06805ee74 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option
1791f6fce4 Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests
26b94ee92a autotools: Remove "auto" value of `--with-ecmult-gen-kb` option
122dbaeb37 autotools: Remove "auto" value of `--with-ecmult-window` option
e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions
0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions
0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h
0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude`
59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...`
ebfb82ee2f ci: Add job with -fsanitize-memory-param-retval
e1bef0961c configure: Move "experimental" warning to bottom
55e5d975db autotools: Disable eager MSan in ctime_tests
ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
218f0cc93b ci: Add native macOS arm64 job

git-subtree-dir: src/secp256k1
git-subtree-split: 4af241b32099067464e015fa66daac5096206dea
achow101 added a commit to bitcoin/bitcoin that referenced this pull request Jun 26, 2024
1408944 Squashed 'src/secp256k1/' changes from 06bff6dec8..4af241b320 (fanquake)

Pull request description:

  Updates the libsecp256k1 subtree to bitcoin-core/secp256k1@f473c95. This includes a number of CMake related changes, including one that prevents CMake from segfaulting when we were configuring the subtree. A number of these changes have come from the review/discussion in hebasto#192:

  * bitcoin-core/secp256k1#1529
  * bitcoin-core/secp256k1#1532
  * bitcoin-core/secp256k1#1535
  * bitcoin-core/secp256k1#1543
  * bitcoin-core/secp256k1#1545
  * bitcoin-core/secp256k1#1546

  Also includes:

  * bitcoin-core/secp256k1#1488
  * bitcoin-core/secp256k1#1517
  * bitcoin-core/secp256k1#1533
  * bitcoin-core/secp256k1#1548
  * bitcoin-core/secp256k1#1550

ACKs for top commit:
  achow101:
    ACK cc58e95
  TheCharlatan:
    ACK cc58e95
  hebasto:
    re-ACK cc58e95.
  real-or-random:
    utACK cc58e95

Tree-SHA512: 41409bc7f65bd17a9feb5c0455e2de2d291a25e4ce14e4a01fe25fcf9d45c64ddf55f274c17d1c86a63ab6b4870997ab79c65ec2795e5b3b49502823770c500f
josibake added a commit to josibake/bitcoin that referenced this pull request Jun 29, 2024
ac59105026 ci: enable silentpayments module
fc6c7f0abb tests: add BIP-352 test vectors
0a3e59b5d2 silentpayments: add benchmark for `scan_outputs`
6812e3f292 silentpayments: add examples/silentpayments.c
c1eef5a0db silentpayments: add recipient light client support
2e8791ae2f silentpayments: add recipient scanning routine
d7dff22516 silentpayments: add opaque data type `public_data`
e29a4f3b74 silentpayments: add recipient label support
b90c219944 silentpayments: add sender routine
81e6b8dcf6 silentpayments: implement output pubkey creation
7f7fb99f33 silentpayments: implement shared secret creation
b0866a2912 silentpayments: add sortable recipient struct
01d6e461a5 doc: add module description for silentpayments
b52dcb0bc5 build: add skeleton for new silentpayments (BIP352) module
REVERT: 4af241b320 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one
REVERT: f473c959f0 Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project
REVERT: d403eea484 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach
REVERT: d7ae25ce6f Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c
REVERT: 0e2fadb20c fix: typos in secp256k1.c
REVERT: 69b2192ad4 Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
REVERT: 5dd637f3cf Merge bitcoin-core/secp256k1#1548: README: mention ellswift module
REVERT: 7454a53736 README: mention ellswift module
REVERT: 4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach
REVERT: c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`
REVERT: f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
REVERT: 158f9e5eae cmake: Do not modify build types when integrating by downstream project
REVERT: 35c0fdc86b Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project
REVERT: 4392f0f717 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (bitcoin#1491)
REVERT: bedffd53d8 Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job
REVERT: 4b8d5eeacf Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests
REVERT: f55703ba49 autotools: Delete unneeded compiler test
REVERT: 396e885886 autotools: Align MSan checking code with CMake's implementation
REVERT: abde59f52d cmake: Report more compiler details in summary
REVERT: 7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
REVERT: 4d9645bee0 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option
REVERT: a06805ee74 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option
REVERT: 26b94ee92a autotools: Remove "auto" value of `--with-ecmult-gen-kb` option
REVERT: 122dbaeb37 autotools: Remove "auto" value of `--with-ecmult-window` option
REVERT: e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions
REVERT: 0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions
REVERT: 0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h
REVERT: 0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude`
REVERT: 59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...`
REVERT: ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
REVERT: cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
REVERT: 218f0cc93b ci: Add native macOS arm64 job

git-subtree-dir: src/secp256k1
git-subtree-split: ac591050262d9d00b629943d598f62b47e1ca7ae
vmta added a commit to umkoin/umkoin that referenced this pull request Jul 3, 2024
a5269373f Merge bitcoin-core/secp256k1#1555: Fixed O3 replacement
b8fe33332 cmake: Fixed O3 replacement
4af241b32 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one
f473c959f Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project
d403eea48 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach
d7ae25ce6 Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c
0e2fadb20 fix: typos in secp256k1.c
69b2192ad Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
5dd637f3c Merge bitcoin-core/secp256k1#1548: README: mention ellswift module
7454a5373 README: mention ellswift module
4706be2cd cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach
c2764dbb9 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`
f87a3589f cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
158f9e5ea cmake: Do not modify build types when integrating by downstream project
35c0fdc86 Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project
4392f0f71 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (#1491)
bedffd53d Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job
4b8d5eeac Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests
f55703ba4 autotools: Delete unneeded compiler test
396e88588 autotools: Align MSan checking code with CMake's implementation
abde59f52 cmake: Report more compiler details in summary
7abf979a4 cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
4d9645bee cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option
a06805ee7 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option
1791f6fce Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests
26b94ee92 autotools: Remove "auto" value of `--with-ecmult-gen-kb` option
122dbaeb3 autotools: Remove "auto" value of `--with-ecmult-window` option
e73f6f8fd tests: refactor: drop `secp256k1_` prefix from testrand.h functions
0ee7453a9 tests: refactor: add `testutil_` prefix to testutil.h functions
0c6bc76dc tests: refactor: move `random_` helpers from tests.c to testutil.h
0fef8479b tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude`
59db007f0 tests: refactor: rename `random_group_element_...` -> `random_ge_...`
ebfb82ee2 ci: Add job with -fsanitize-memory-param-retval
e1bef0961 configure: Move "experimental" warning to bottom
55e5d975d autotools: Disable eager MSan in ctime_tests
ec4c002fa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
cae9a7ad1 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
218f0cc93 ci: Add native macOS arm64 job

git-subtree-dir: src/secp256k1
git-subtree-split: a5269373fa13ff845f654d81b90629dd78495641
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jul 26, 2024
4af241b320 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one
f473c959f0 Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project
d403eea484 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach
d7ae25ce6f Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c
0e2fadb20c fix: typos in secp256k1.c
69b2192ad4 Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
5dd637f3cf Merge bitcoin-core/secp256k1#1548: README: mention ellswift module
7454a53736 README: mention ellswift module
4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach
c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`
f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
158f9e5eae cmake: Do not modify build types when integrating by downstream project
35c0fdc86b Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project
4392f0f717 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (#1491)
bedffd53d8 Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job
4b8d5eeacf Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests
f55703ba49 autotools: Delete unneeded compiler test
396e885886 autotools: Align MSan checking code with CMake's implementation
abde59f52d cmake: Report more compiler details in summary
7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
4d9645bee0 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option
a06805ee74 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option
1791f6fce4 Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests
26b94ee92a autotools: Remove "auto" value of `--with-ecmult-gen-kb` option
122dbaeb37 autotools: Remove "auto" value of `--with-ecmult-window` option
e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions
0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions
0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h
0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude`
59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...`
ebfb82ee2f ci: Add job with -fsanitize-memory-param-retval
e1bef0961c configure: Move "experimental" warning to bottom
55e5d975db autotools: Disable eager MSan in ctime_tests
ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
218f0cc93b ci: Add native macOS arm64 job

git-subtree-dir: src/secp256k1
git-subtree-split: 4af241b32099067464e015fa66daac5096206dea
vmta added a commit to umkoin/umkoin that referenced this pull request Sep 6, 2024
a5269373f Merge bitcoin-core/secp256k1#1555: Fixed O3 replacement
b8fe33332 cmake: Fixed O3 replacement
4af241b32 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one
f473c959f Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project
d403eea48 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach
d7ae25ce6 Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c
0e2fadb20 fix: typos in secp256k1.c
69b2192ad Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
5dd637f3c Merge bitcoin-core/secp256k1#1548: README: mention ellswift module
7454a5373 README: mention ellswift module
4706be2cd cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach
c2764dbb9 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS`
f87a3589f cmake: Do not set `CTEST_TEST_TARGET_ALIAS`
158f9e5ea cmake: Do not modify build types when integrating by downstream project
35c0fdc86 Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project
4392f0f71 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (#1491)
bedffd53d Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job
4b8d5eeac Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests
f55703ba4 autotools: Delete unneeded compiler test
396e88588 autotools: Align MSan checking code with CMake's implementation
abde59f52 cmake: Report more compiler details in summary
7abf979a4 cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
4d9645bee cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option
a06805ee7 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option
1791f6fce Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests
26b94ee92 autotools: Remove "auto" value of `--with-ecmult-gen-kb` option
122dbaeb3 autotools: Remove "auto" value of `--with-ecmult-window` option
e73f6f8fd tests: refactor: drop `secp256k1_` prefix from testrand.h functions
0ee7453a9 tests: refactor: add `testutil_` prefix to testutil.h functions
0c6bc76dc tests: refactor: move `random_` helpers from tests.c to testutil.h
0fef8479b tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude`
59db007f0 tests: refactor: rename `random_group_element_...` -> `random_ge_...`
ebfb82ee2 ci: Add job with -fsanitize-memory-param-retval
e1bef0961 configure: Move "experimental" warning to bottom
55e5d975d autotools: Disable eager MSan in ctime_tests
ec4c002fa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation
cae9a7ad1 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable
218f0cc93 ci: Add native macOS arm64 job

git-subtree-dir: src/secp256k1
git-subtree-split: a5269373fa13ff845f654d81b90629dd78495641
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants