Repository: bitcoin/bitcoin
File: src/key.cpp (C++)

Code before:
    uint256 hash{Hash(str, rnd)};
    std::vector<unsigned char> vchSig;
    Sign(hash, vchSig);
    return pubkey.Verify(hash, vchSig);
}

bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig) const {
    if (!keydata)
        return false;
    vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE);
    int rec = -1;
    secp256k1_ecdsa_recoverable_signature rsig;
    int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &rsig, hash.begin(), UCharCast(begin()), secp256k1_nonce_function_rfc6979, nullptr);
    assert(ret);
    ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &rsig);
    assert(ret);
    assert(rec != -1);
    vchSig[0] = 27 + rec + (fCompressed ? 4 : 0);
    // Additional verification step to prevent using a potentially corrupted signature
    secp256k1_pubkey epk, rpk;
    ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &epk, UCharCast(begin()));
    assert(ret);
    ret = secp256k1_ecdsa_recover(secp256k1_context_static, &rpk, &rsig, hash.begin());
    assert(ret);
    ret = secp256k1_ec_pubkey_cmp(secp256k1_context_static, &epk, &rpk);
    assert(ret == 0);
    return true;
}

bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
{
    return ComputeKeyPair(merkle_root).SignSchnorr(hash, sig, aux);
}

bool CKey::Load(const CPrivKey &seckey, const CPubKey &vchPubKey, bool fSkipCheck=false) {
    MakeKeyData();
    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size())) {
        ClearKeyData();
        return false;
    }
    fCompressed = vchPubKey.IsCompressed();

    if (fSkipCheck)
        return true;

    return VerifyPubKey(vchPubKey);
}

bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const {
    assert(IsValid());
    assert(IsCompressed());

Review comment (security):
Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn't really useful – having separate red and green parts, not really a diff.

Additionally, the commit doesn't explain each change separately. For example:
* The exact reason for memory management changes (via `make_secure_unique`) is not explained, nor are the security implications or potential performance impacts discussed.
* It's unclear why `GetKey` is only used in tests. Why not delete it and introduce it in another PR where it's actually needed by production code?
* The fate of `memory_cleanse(&keypair, sizeof(keypair))` is not explained, nor are the implications of its removal or replacement addressed.

It would help a lot if a commit did a single thing, such as a rename or cast change, changing a constant, moving implementation over to a new method, introducing a new class, or splitting a method into two methods, etc - where the commit message explains why the change was necessarry.
This would separate trivial changes from high-risk ones that need extra attention, so we don't have unrelated changes competing for our attention.

Please see https://github.com/bitcoin/bitcoin/pull/28280/commits for an example of focused, single-purpose commits that separate low and high-risk changes (e.g., preparatory refactorings from the focus of the PR), making the review a lot easier.


Code after:
bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig) const {
    if (!keydata)
        return false;
    vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE);
    int rec = -1;
    secp256k1_ecdsa_recoverable_signature rsig;
    int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &rsig, hash.begin(), UCharCast(begin()), secp256k1_nonce_function_rfc6979, nullptr);
    assert(ret);
    ret = secp256k1_ecdsa_recoverable_signature_serialize_compact(secp256k1_context_sign, &vchSig[1], &rec, &rsig);
    assert(ret);
    assert(rec != -1);
    vchSig[0] = 27 + rec + (fCompressed ? 4 : 0);
    // Additional verification step to prevent using a potentially corrupted signature
    secp256k1_pubkey epk, rpk;
    ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &epk, UCharCast(begin()));
    assert(ret);
    ret = secp256k1_ecdsa_recover(secp256k1_context_static, &rpk, &rsig, hash.begin());
    assert(ret);
    ret = secp256k1_ec_pubkey_cmp(secp256k1_context_static, &epk, &rpk);
    assert(ret == 0);
    return true;
}

bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
{
    KeyPair kp = ComputeKeyPair(merkle_root);
    return kp.SignSchnorr(hash, sig, aux);
}

bool CKey::Load(const CPrivKey &seckey, const CPubKey &vchPubKey, bool fSkipCheck=false) {
    MakeKeyData();
    if (!ec_seckey_import_der(secp256k1_context_sign, (unsigned char*)begin(), seckey.data(), seckey.size())) {
        ClearKeyData();
        return false;
    }
    fCompressed = vchPubKey.IsCompressed();

    if (fSkipCheck)
        return true;

    return VerifyPubKey(vchPubKey);
}

bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const {
    assert(IsValid());
    assert(IsCompressed());
    std::vector<unsigned char, secure_allocator<unsigned char>> vout(64);
    if ((nChild >> 31) == 0) {
        CPubKey pubkey = GetPubKey();
        assert(pubkey.size() == CPubKey::COMPRESSED_SIZE);
        BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, vout.data());