This bug was introduced in 7506e064d791e529d2e57bb52c156deb33b897ef by adding an allocation but not updating the constant.
ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS #1004
pull jonasnick wants to merge 1 commits into bitcoin-core:master from jonasnick:fix-strauss changing 1 files +9 −3-
jonasnick commented at 8:45 PM on November 6, 2021: contributor
- jonasnick cross-referenced this on Nov 6, 2021 from issue Function to compute optimal ecmult_multi scratch size for a number of points by jonasnick
-
real-or-random commented at 10:24 AM on November 8, 2021: contributor
Good to move this to a separate PR.
Unfortunately the constants are highly coupled with the allocation code. Can you add a comment to the allocation code that says that the constant must be updated when this is changed (also for pippenger)? Otherwise I bet we'll get this wrong again in the future.
-
robot-dreams commented at 5:19 PM on November 28, 2021: contributor
Thanks for catching this. Looks good to me—I confirmed that:
secp256k1_ecmult_strauss_batchnow has 7 calls tosecp256k1_scratch_allocinstead of 6- What matters for
STRAUSS_SCRATCH_OBJECTSis the number of allocations (not the total size being allocated), since it's used to account for per-allocation alignment padding
I agree with @real-or-random 's suggestion. In addition, do the functions
secp256k1_strauss_scratch_sizeandsecp256k1_pippenger_scratch_sizealso need to be kept in sync when allocations are updated? - jonasnick force-pushed on Nov 30, 2021
-
60bf8890df
ecmult: fix definition of STRAUSS_SCRATCH_OBJECTS
This bug was introduced in 7506e064d791e529d2e57bb52c156deb33b897ef by adding an allocation but not updating the constant.
-
jonasnick commented at 7:25 PM on November 30, 2021: contributor
Good idea. Added comments.
- real-or-random approved
-
real-or-random commented at 7:39 PM on November 30, 2021: contributor
ACK 60bf8890df5360148df921f26d8dc4d667dd5926
-
real-or-random commented at 3:05 PM on December 2, 2021: contributor
@robot-dreams Want give a final review here?
-
robot-dreams commented at 3:30 PM on December 2, 2021: contributor
ACK 60bf8890df5360148df921f26d8dc4d667dd5926
- real-or-random merged this on Dec 2, 2021
- real-or-random closed this on Dec 2, 2021
- fanquake referenced this in commit c4a1e09a8c on Dec 3, 2021
- sipa referenced this in commit 86dbc4d075 on Dec 15, 2021
- sipa cross-referenced this on Dec 15, 2021 from issue Update libsecp256k1 subtree to current master by sipa
- jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick
- real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022
- gwillen referenced this in commit 35d6112a72 on May 25, 2022
- janus referenced this in commit 879a9a27b9 on Jul 10, 2022
- patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
- patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
- backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023
- str4d referenced this in commit 6de4698bf9 on Apr 21, 2023
- vmta referenced this in commit e1120c94a1 on Jun 4, 2023
- vmta referenced this in commit 8f03457eed on Jul 1, 2023
