Questions about secp256k1_pubkey_load/save() implementation

Question

I'm a newbie who has just started reading Bitcoin Core code. I have a lot of questions about seemingly trivial code. Right now, I have two questions.

1. When branching to an if statement, why is the buffer s used instead of copying directly from the pubkey to the parameter ge (and vice versa) in the secp256k1_pubkey_load/save function?

Wouldn't the code below work just as well?

// secp256k1.c
static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge, const secp256k1_pubkey* pubkey) {
    if (sizeof(secp256k1_ge_storage) == 64) {
        /* When the secp256k1_ge_storage type is exactly 64 byte, use its
         * representation inside secp256k1_pubkey, as conversion is very fast.
         * Note that secp256k1_pubkey_save must use the same representation. */
#if (0)
        secp256k1_ge_storage s;
        memcpy(&s, &pubkey->data[0], sizeof(s));
        secp256k1_ge_from_storage(ge, &s);
#else
        secp256k1_ge_from_storage(ge, (secp256k1_ge_storage *)&pubkey->data[0]);
#endif /* (0) */
    } else {
        /* Otherwise, fall back to 32-byte big endian for X and Y. */
        secp256k1_fe x, y;
        secp256k1_fe_set_b32(&x, pubkey->data);
        secp256k1_fe_set_b32(&y, pubkey->data + 32);
        secp256k1_ge_set_xy(ge, &x, &y);
    }
    ARG_CHECK(!secp256k1_fe_is_zero(&ge->x));
    return 1;
}

2. Is it possible to branch to else?

Looking at the structure below, it seems impossible.

// group.h
typedef struct {
    secp256k1_fe_storage x;
    secp256k1_fe_storage y;
} secp256k1_ge_storage;

// field_5x52.h
typedef struct {
    uint64_t n[4];
} secp256k1_fe_storage;

// field_10x26.h
typedef struct {
    uint32_t n[8];
} secp256k1_fe_storage;

Answer 1

Wouldn't the code below work just as well?

Possibly, but I believe it would be undefined behavior (and if it isn't, it'd need careful language lawyering to make sure it isn't). The existing code is likely optimized by the compiler to act exactly like you'd hope your code would (it doesn't actually perform an intermediary copy if unnecessary), but is well-defined. So the short answer is just that the code here is more obviously correct.

The issue is that dereferencing a pointer through a type incompatible with the type of the actual object is undefined behavior, in many cases. secp256k1_fe_storage objects contain 4 64 bit integers, or 8 32 bit integers, while secp256k1_pubkey contains 64 bytes. These are not the same, and accessing one while pretending it's the other is not allowed.

On the other hand, char/unsigned char pointers are allowed to point to anything, including objects that don't actually consist of these (in which case you observe the byte representation of the object instead). That's what's going on here: memcpy treats both inputs as pointers to bytes (which is always allowed) and copies between those. Then a pointer to an actual secp256k1_ge_storage object (after being loaded with the bytes from the secp256k1_pubkey) is given to secp256k1_ge_from_storage

2. Is it possible to branch to else?

Maybe.

The else is just there to provide a fallback for systems where uint32_t isn't exactly 4 bytes, or uint64_t isn't exactly 8 bytes. I don't actually know of any systems for which that is the case, but I believe the C standard does permit it.