<blockquote>
<p>Wouldn't the code below work just as well?</p>
</blockquote>
<p>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 <strong>more obviously correct</strong>.</p>
<p>The issue is that dereferencing a pointer through a type incompatible with the type of the actual object is undefined behavior, in many cases. <code>secp256k1_fe_storage</code> objects contain 4 64 bit integers, or 8 32 bit integers, while <code>secp256k1_pubkey</code> contains 64 bytes. These are not the same, and accessing one while pretending it's the other is not allowed.</p>
<p>On the other hand, <code>char</code>/<code>unsigned char</code> 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: <code>memcpy</code> treats both inputs as pointers to bytes (which is always allowed) and copies between those. Then a pointer to an actual <code>secp256k1_ge_storage</code> object (after being loaded with the bytes from the <code>secp256k1_pubkey</code>) is given to <code>secp256k1_ge_from_storage</code></p>
<blockquote>
<ol start="2">
<li>Is it possible to branch to else?</li>
</ol>
</blockquote>
<p>Maybe.</p>
<p>The <code>else</code> is just there to provide a fallback for systems where <code>uint32_t</code> isn't exactly 4 bytes, or <code>uint64_t</code> 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.</p>

CC BY-SA 4.0
2023-05-29T19:56:00.240Z
2023-05-29T19:56:00.240Z
Pieter Wuille
<p>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.</p>
<ol>
<li>When branching to an <code>if</code> statement, why is the buffer <code>s</code> used instead of copying directly from the <code>pubkey</code> to the parameter <code>ge</code> (and vice versa) in the <a href="https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L239" rel="nofollow noreferrer">secp256k1_pubkey_load</a>/<a href="https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L258" rel="nofollow noreferrer">save</a> function?</li>
</ol>
<p>Wouldn't the code below work just as well?</p>
<pre><code>// 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(&amp;s, &amp;pubkey-&gt;data[0], sizeof(s));
        secp256k1_ge_from_storage(ge, &amp;s);
#else
        secp256k1_ge_from_storage(ge, (secp256k1_ge_storage *)&amp;pubkey-&gt;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(&amp;x, pubkey-&gt;data);
        secp256k1_fe_set_b32(&amp;y, pubkey-&gt;data + 32);
        secp256k1_ge_set_xy(ge, &amp;x, &amp;y);
    }
    ARG_CHECK(!secp256k1_fe_is_zero(&amp;ge-&gt;x));
    return 1;
}
</code></pre>
<ol start="2">
<li>Is it possible to branch to <code>else</code>?</li>
</ol>
<p>Looking at the structure below, it seems impossible.</p>
<pre><code>// 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;
</code></pre>

<bitcoin-core-development><secp256k1><c>
Questions about secp256k1_pubkey_load/save() implementation
Gildong.Hong
# Questions about secp256k1_pubkey_load/save() implementation

(Asked by: username_0 on 2023-05-29)

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](https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L239)/[save](https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L258) 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;

```

**Tags:** bitcoin-core-development, secp256k1, c


---

## 1 answers

### Answer by username_1 on 2023-05-29. Score: 6 | Accepted answer

> 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.


---

