Overloading of serialization terms

[tcharding]

Currently we overload serialization terms and there is no obvious and easy way to distinguish them

• serde: Uses serialize and deserialize
• consensus: Encodable, consensus_encode, Decodable, consensus_decode
• arbitrary serialization to vectors of bytes and on stack array of bytes: We sometimes use serialize, sometimes to_bytes
• psbt: uses encode, consensus_encode, serialize, traits and inherent methods.

Possible solution

• to/as/from_bytes: Only when the type is byte-like (eg, wraps an array)
• serialize/deserialize: Leave these names for serde, includes any arbitrary serialization
• consensus::Encode::encode and consensus::Decode::decode for things that are governed by consensus rules
• Use marshal and unmarshal for things that have a specific custom serialization governed by a convention or BIP eg, PSBT

[apoelstra]

I think your proposal simply adds marshal and unmarshal, and therefore increases the amount of inconsistent terminology :).

In my view:

• It would be good to get rid of serialize on keys, signatures, etc., because this term causes constant confusion as to whether it allocates, whether it takes a nontrivial amount of work, etc. Unfortunately I don't have a good proposed replacement. What should a methad that turns a Signature into a SerializedSignature be called?
• For PSBT we need to more-or-less redo the API. But for byte encoding I think we should either use encode (because in my view PSBT serialization is morally the same kind of thing as "consensus encoding") or to_vec/to_bytes (to match std).

[tcharding]

I think your proposal simply adds marshal and unmarshal, and therefore increases the amount of inconsistent terminology :).

Fair point.

• It would be good to get rid of serialize on keys, signatures, etc., because this term causes constant confusion as to whether it allocates, whether it takes a nontrivial amount of work, etc. Unfortunately I don't have a good proposed replacement. What should a methad that turns a Signature into a SerializedSignature be called?

This serializing (adding the hash type on the end) is consensus-y stuff, right? Perhaps we move to using "encode" in identifiers that do consensus, Bitcoin specified, stuff and leave "serialize" for serde?

[apoelstra]

This serializing (adding the hash type on the end) is consensus-y stuff, right?

It matches the consensus encoding, yes, but it's also just the standard encoding of these objects (defined by SECG or ASN.1 or whatever) and there isn't really any other encoding. It's common to throw signatures, keys, hashes, etc., into configuration files or caches in ad-hoc ways and I worry that we shouldn't make users use the consensus Encodable traits for this. I feel like the consensus encoding traits are for building specific composite objects (blocks, transactions, and maaybe PSBTs).

[Kixunil]

I'm fine with Signature::serialize renaming to encode and the rest should probably stay? (Strictly speaking, I am annoyed but if this makes it easier to distinguish binary from serde sounds like a good thing.)

[dpc]

FWIW in Fediming we have our own consensus and consensus encoding that is modeled (copied?) from rust-bitcoin one, it's also called "encoding" (Encodabe/Decodable) and it grow on my that "encoding" is for consensus and "serialize" for other serde.

[apoelstra]

I'm fine with Signature::serialize renaming to encode and the rest should probably stay? (Strictly speaking, I am annoyed but if this makes it easier to distinguish binary from serde sounds like a good thing.)

I think this would make the serialization function harder to find, and that encode is misleading because it's not just encoding, it's actually creating a specific self-contained type that contains the encoding. Which is usually what "serialize" means.

The problem in my view is not about confusing serde with non-serde serializations, but confusing cheap serializers from allocating ones.

[Kixunil]

confusing cheap serializers from allocating ones.

I assume you mean heap allocations only, not stack allocations. Signature::serialize doesn't allocate on heap. The type created is effectively ArrayVec<const N = 65, u8>.

FTR I don't care too much. Let's pick a rule and apply it consistently.

[apoelstra]

Signature::serialize doesn't allocate on heap. The type created is effectively ArrayVec<const N = 65, u8>.

Right -- that's my point. We have a number of places where we call this method (or the same one on PublicKey) then immediately take a reference to the type to get a slice, which looks a lot like "allocate a vector and immediately throw it away". It would be better if the method were called as_bytes or something which made it clear that there are no allocations happening.

(And yes, by "allocation" I mean the kind of allocation that involves a syscall, potentially causes memory fragementation, and requires an allocator be available. Vs the kind where you increment a stack pointer register.)

[Kixunil]

to_bytes seems too straightforward? This is still not crazy cheap it goes through FFI and so. Our usual to_bytes functions could be trivially inlined and heavily optimized. Any other ideas?

[tcharding]

it's actually creating a specific self-contained type that contains the encoding.

Any other ideas?

Responding to both the above, what about stack_encode()? E.g.,

let serialized = TweakedPublicKey::stack_encode();
foo(&pk.stack_encode());

MessageSignature::serialize could be the same.

And for Signature to SerializedSignature, what about

And for this one
```rust
pub fn serialized_signature(&self) -> SerializedSignature {

Note that in other places we seem to use 'encode' to serialize to a writer and 'serialize' to serialize to a vector (or String) - this is in line with your comment, right @apoelstra?

[Kixunil]

Not really anything referring to the memory location - it's actually caller-decided whether it ends up on stack! We just say stack informally here because that's the intended use case.

I guess it should both refer to bytes and to possibly costly transformation of the value into bytes.

[tcharding]

Not really anything referring to the memory location - it's actually caller-decided whether it ends up on stack! We just say stack informally here because that's the intended use case.

huh? TweakedPublicKey::serialize() calls XOnlyPublicKey::serialize() which uses the stack

   #[inline]
    /// Serializes the key as a byte-encoded x coordinate value (32 bytes).
    pub fn serialize(&self) -> [u8; constants::SCHNORR_PUBLIC_KEY_SIZE] {
        let mut ret = [0u8; constants::SCHNORR_PUBLIC_KEY_SIZE];

        unsafe {
            let err = ffi::secp256k1_xonly_pubkey_serialize(
                ffi::secp256k1_context_no_precomp,
                ret.as_mut_c_ptr(),
                self.as_c_ptr(),
            );
            debug_assert_eq!(err, 1);
        }
        ret
    }

[Kixunil]

The type returned may end up on heap if you write Box::new(signature.stack_encode()).

[tcharding]

Yeah but that is still clear as to whats happening - allocate some memory and copy something into it.

[Kixunil]

That's just the more obvious version. If different pieces of code combined do that you may not see it directly and it ends up on heap after inlining. But the main idea is that while we do say "this method allocates" for allocating code, we should never say "this type is certainly on stack". And nowhere in Rust is _stack or _heap used, I believe for exactly that reason.

[apoelstra]

I agree we shouldn't say stack in the method signature. Aside from causing arguments like this one, it will also be confusing to new Rust programmers who don't know what it means (I've met a few; you can get surprisingly far with Rust without having any ideas about the underlying memory model).

@Kixunil yeah, I also agree that to_bytes is "too cheap" a name for something that crosses the FFI boundary and doesn't inline to a 32-byte memcpy or something. But OTOH I have a low-priority long-term plan to eventually Rust-ify all the parsing and serialization functions for libsecp types. Serialization shouldn't be too hard. Parsing might be, since it involves field arithmetic.

[Kixunil]

But OTOH I have a low-priority long-term plan to eventually Rust-ify all the parsing and serialization functions for libsecp types.

Is that even possible given upstream doesn't guarantee the layout? Or is it going to stabilize/already stable?

[apoelstra]

Is that even possible given upstream doesn't guarantee the layout? Or is it going to stabilize/already stable?

It's possible because we vendor a specific version of upstream.

[Kixunil]

Do we want to go that far given there was interest in dynamic linking? We could obviously not use the Rust code if linking is dynamic but maybe that's too much?

[apoelstra]

When we do dynamic linking we probably want to do some sanity checks anyway. One of those could be "does the dynamic library encode points and signatures the way we expect".

In practice I think this check would never trigger (upstream has never changed the format of these types without also changing their size, which is a breaking API change according to their own rules) but it would mean that if it did trigger, it'd be a visible failure not a "silently wrong crypto" one.

Alternately, we could define our own internal format for signatures and public keys and internally convert before doing "real" crypto. I believe we could set this up so that we never paid the FFI cost except when we were already paying a 10s-of-microsecond crypto cost. Note that for signatures and uncompressed keys, the libsecp parsing code is almost free since it just memcpys and does some range checks.

So I think we have some options, and we don't need to say "can't ever rustify the parsing code" on account of dynamic linking.

[Kixunil]

When we do dynamic linking we probably want to do some sanity checks anyway.

Do we really need to check anything else than version? We know that if major version matches it's OK unless whoever built the library made some non-standard changes in which case they get UB but they would get it regardless of our checks.

Alternately, we could define our own internal format for signatures and public keys and internally convert before doing "real" crypto. I believe we could set this up so that we never paid the FFI cost except when we were already paying a 10s-of-microsecond crypto cost.

I really like this. Especially given that maybe we can somehow detect that the format is actually the same and then just pass the pointer without conversion. This also goes hand in hand with making the sys crate optional.

[tcharding]

cc @LLFourn because we were talking about exactly this on the weekend.

[LLFourn]

I think it's fine to have to_bytes do ffi calls and other work. If you could do a memcpy you'd usually have as_bytes instead. So +1 for to_bytes/from_bytes for everything that has a fixed length binary encoding (and a FromStr/Display from hex to go with it).

It would be great if sys crate could be optional with some clever encoded newtypes used instead of types that need ffi calls to do encoding etc. I don't see why it wouldn't work but my knowledge is limited.

[tcharding]

Converted to a discussion because as an issue there was no action that could close it. Opened #2583 also.