Introduce secp256k1 module with field and group classes to test framework

[sipa]

This PR rewrites a portion of test_framework/key.py, in a compatible way, by introducing classes that encapsulate field element and group element logic, in an attempt to be more readable and reusable.

To maximize readability, the group element logic does not use Jacobian coordinates. Instead, group elements just store (affine) X and Y coordinates directly. To compensate for the performance loss this causes, field elements are represented as fractions. This undoes most, but not all, of the performance loss, and there is a few % slowdown (as measured in feature_taproot.py, which heavily uses this).

The upside is that the implementation for group laws (point doubling, addition, subtraction, ...) is very close to the mathematical description of elliptic curves, and this extends to potential future extensions (e.g. ElligatorSwift as needed by #27479).

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, stratospher, achow101
Concept ACK	hebasto, pinheadmz, real-or-random

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27653 (test: add unit test coverage for Python ECDSA implementation by theStack)
• #24005 (test: add python implementation of Elligator swift by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[amovfx]

Reviewed to learn.

[sipa]

Rebased.

[theStack]

Concept ACK, +1 on increased readability and reusability 💯 (found my way here through #24748)

The slow-down is unfortunately pretty significant on the two x86_64 machines I tested, feature_taproot.py taking almost twice as long compared to master here. Results measured via time:

Machine / OS	master	pr26222 (rebased)
AMD EPYC 7702P (6) / Ubuntu 22.04.1 LTS	40.496s	1m16.587s
Intel (Broadwell) (2) / OpenBSD 7.2	2m57.43s	5m45.90s

For the fun of it, I looked a bit into other ways how to compensate the performance loss. Considering that scalar multiplication with G is a frequent operation, precomputing G's doubled-up values (G, 2G, 4G, ..., (2^254)G, (2^255)G) in a lookup table seemed promising, with the goal of needing only ~128 point additions on average (one for each bit set in the scalar), without any extra double operations. Using that, the execution time is still not matching, but at least pretty close to the master branch (45.334s for first, 3m23.48s for second machine), without too much additional code (~25 LOC):

diff for commit "test: EC: optimize scalar multiplication of G by using lookup table"

index 7434ba49e..1ad1bc3e4 100644
--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -233,6 +233,8 @@ class GE:
 
     def __mul__(self, a):
         """Multiply a point with an integer (scalar multiplication)."""
+        if self == SECP256K1_G:  # optimize generator multiplication using precomputed data
+            return fast_g.mul(a)
         r = None
         for i in range(a.bit_length() - 1, -1, -1):
             if r is not None:
@@ -596,6 +598,31 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False):
     e = int.from_bytes(TaggedHash("BIP0340/challenge", R.to_bytes_xonly() + P.to_bytes_xonly() + msg), 'big') % GE.ORDER
     return R.to_bytes_xonly() + ((k + e * sec) % GE.ORDER).to_bytes(32, 'big')
 
+
+class FastG:
+    """Speed up scalar multiplication with the generator point G
+       by using a precomputed lookup table with its powers of 2:
+       g_table = [G, G*2, G*4, G*(2^3), G*(2^4), ..., G*(2^255)]
+       The points corresponding to each bit set in the scalar are
+       added up, i.e. on average ~128 point additions take place.
+    """
+    def __init__(self):
+        self.g_table = []  # g_table[i] = G * (2^i)
+        g_i = SECP256K1_G
+        for bit in range(256):
+            self.g_table.append(g_i)
+            g_i = g_i.double()
+
+    def mul(self, a):
+        result = None
+        for bit in range(a.bit_length()):
+            if (a & (1 << bit)):
+                result += self.g_table[bit]
+        return result
+
+fast_g = FastG()
+
+
 class TestFrameworkKey(unittest.TestCase):
     def test_schnorr(self):
         """Test the Python Schnorr implementation."""

(see branch https://github.com/theStack/bitcoin/tree/pr26222_followup_precompute_g_doubles)

Not 100% sure if we even want optimizations like this in test framwork (also, immediately generating data at module import seems kind of hacky), but it may be worth it in this case? I'm pretty sure there are more efficient ways to pursue this idea (e.g. extending the lookup table for larger chunks than only 1-bit pieces to need even less additions), but probably the extra implementation / code clutter / maintenance effort is not worth it. Any other ideas? Happy to hear shadowy secp256k1 super-magicians' thoughts here :)

[sipa]

@theStack I've included your commit to add the precomputed G table. The speedup is significant enough that it's worth it, I think.

You've indeed discovered one of the techniques that are used for speeding up EC multiplications with precomputed tables. Libsecp256k1 today uses a more advanced version of that idea, where all multiples of the form i*16^j*G for all i=0..15, and j=0..63 are precomputed, leaving us with ~63 point additions. An even more advanced approach is discussed in bitcoin-core/secp256k1#1058, if you're interested. All of that is IMO out of scope for the test framework, though.

[fanquake]

cc @real-or-random

[sipa]

Addressed review comments. I've also renamed FE.num and FE.den to FE._num and FE._den to mark them as private (to the extent that Python allows that). All interactions with these objects should be done through their methods.

[theStack]

ACK ab30e81

[theStack]

re-ACK d4fb58a

(as per $ git range-diff ab30e81b...d4fb58ae, verified that the only changes since the last ACK were a rebase on master and #26222 (comment) tackled)

[stratospher]

tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!

[achow101]

ACK d4fb58a

Much simpler to understand, and the more complicated stuff matches up with the descriptions in WIkipedia.