Needs a test where the pubkey doesn't decode.

Added

Thanks but please also add one that could actually have come in over the wire for BIP151, -- the first byte isn't sent there. What I'm hoping we check for is that it correctly rejects points that aren't on the curve here, rather than performing operations on the twist.

The new pubkeydata[9] = 0xFF; line checks that, right? Maybe add a comment to clarify?

Is it better to pass a SECP256K1_CONTEXT_NONE context, given the context is not currently used by these functions?

That would require creating a dummy context just for this purpose, so I don't think that would be best.

Maybe rename secp256k1_context_sign to secp256k1_context?

Extract ComputeECDHSecret from CKey to make it a freestanding function?

bool ComputeECDHSecret(const CKey& key, const CPubKey& pubkey, CPrivKey& secret_out);

Although it is convenient to add new BIP responsibilities directly to CKey, I would recommend against adding more responsibilities to CKey than it already has.

I think this makes sense to be a member function (in align with Derive and other member functions of CKey).

"A function that can be simply, elegantly, and efficiently implemented as a freestanding function (that is, as a nonmember function) should be implemented outside the class." (Stroustrup) is a very commonly used and well documented best practice in C++ development.

See C.4. Make a function a member only if it needs direct access to the representation of a class for more info and guidelines.

bool ComputeECDHSecret(const CKey& key, const CPubKey& pubkey, CPrivKey& secret_out)
{
    secp256k1_pubkey pubkey_internal;
    if (!secp256k1_ec_pubkey_parse(secp256k1_context_sign, &pubkey_internal, pubkey.data(), pubkey.size())) {
        return false;
    }

    secret_out.resize(32);
    return secp256k1_ecdh(secp256k1_context_sign, &secret_out[0], &pubkey_internal, key.begin()) == 1;
}

Is this --enable-experimental "permanent"? Perhaps this change should be behind a ./configure --enable-experimental-ecdh, or is overly pedantic?

It's an upstream question. Should be asked at https://github.com/bitcoin-core/secp256k1

It's declared experimental upstream. Asking upstream to no longer declare it experimental is an upstream question. My point is about using it here; as long as it's experimental upstream, shouldn't it be considered experimental here?

I think it's best to not hide it behind a configuration option on our side. I think we should always compile BIP324 features but only activate them when passing a startup argument (can be switched to default later)