Do we want to protect against a malicious caller? signature.len() returns a value that could have previously been set with signature.set_len(). A malicious call could cause this function to panic. Not sure if that matters?

One solution might be:

    pub fn push_bitcoin_signature(&mut self, signature: &ecdsa::SerializedSignature, hash_type: EcdsaSighashType) {
        // Note that a maximal length ECDSA signature is 72 bytes, plus the sighash type makes 73
        let mut sig = [0; 73];
        let len = signature.len();

        if len > 72 {
            // This can happen if a malicious caller has previously called
            // signature.set_len() with a length intentionally too big.
            return;
        }

        sig[..len].copy_from_slice(&signature);
        sig[len] = hash_type as u8;
        self.push(&sig[..=len]);
    }

Perhaps we should just add a check to set_len instead?

I'm not sure what is the security model you are thinking of. If the attacker controls any code in the current process, aren't all bets off? they can just read/write random memory locations, including keys and such.

I don't see a set_len() anywhere in https://docs.rs/secp256k1/latest/secp256k1/ecdsa/struct.SerializedSignature.html? What am I missing?

It also looks to me like SerializedSignature cannot be coerced into having a longer length. If it were, I'd advocate for a comment describing the panic, but I'm ok with it existing.

I'm not sure what is the security model you are thinking of

Sorry, I misspoke I should have said 'defensive programming' instead of 'malicious caller', my bad.

I don't see a set_len() anywhere

Its pub (crate) so doesn't show up in those docs

It also looks to me like SerializedSignature cannot be coerced into having a longer length.

Thanks for taking a look.

don't we want this for Schnorr too?

or maybe a generic one:

pub fn push_with_hash_type<T: AsRef<[u8]>>(&mut self, new_element: T, hash_type: u8) {

I wasn't sure exactly the format for schnorr so didn't bother. I'd prefer to keep the SerializedSignature here as it makes the API more explicit, but would be happy to add one for schnorr as well. Note that making it generic would be a lot more involved as we wouldn't have a maximum length of the new_element hard-coded.

+1 to having a totally separate method for schnorr (doesn't need to be in this PR)