ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/test/util/random.cpp (C++)

Code before:
FastRandomContext g_insecure_rand_ctx;

extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;

void SeedRandomForTest(SeedRand seedtype)
{
    static const std::string RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};

    // Do this once, on the first call, regardless of seedtype, because once
    // MakeRandDeterministicDANGEROUS is called, the output of GetRandHash is
    // no longer truly random. It should be enough to get the seed once for the
    // process.
    static const uint256 ctx_seed = []() {
        // If RANDOM_CTX_SEED is set, use that as seed.
        const char* num = std::getenv(RANDOM_CTX_SEED.c_str());
        if (num) return uint256S(num);
        // Otherwise use a (truly) random value.
        return GetRandHash();
    }();

    const uint256& seed{seedtype == SeedRand::SEED ? ctx_seed : uint256::ZERO};
    LogPrintf("%s: Setting random seed for current tests to %s=%s\n", __func__, RANDOM_CTX_SEED, seed.GetHex());
    MakeRandDeterministicDANGEROUS(seed);
    g_insecure_rand_ctx.Reseed(GetRandHash());
}


Review comment (refactor):
If you re-touch this commit, it could make sense to remove the `__func__` printing in the log call below. It doesn't add any value and may be duplicate. I understand it is unrelated, but when touching a test-only function, may as well fix all trivial issues in one go. (See the commit I referred to in https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199, which also has steps to reproduce and test in the description)

Code after:

FastRandomContext g_insecure_rand_ctx;

extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;

void SeedRandomForTest(SeedRand seedtype)
{
    constexpr auto RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};

    // Do this once, on the first call, regardless of seedtype, because once
    // MakeRandDeterministicDANGEROUS is called, the output of GetRandHash is
    // no longer truly random. It should be enough to get the seed once for the
    // process.
    static const uint256 ctx_seed = []() {
        // If RANDOM_CTX_SEED is set, use that as seed.
        if (const char* num{std::getenv(RANDOM_CTX_SEED)}) {
            if (auto num_parsed{uint256::FromUserHex(num)}) {
                return *num_parsed;
            } else {
                std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";
                std::abort();
            }
        }
        // Otherwise use a (truly) random value.
        return GetRandHash();
    }();

    const uint256& seed{seedtype == SeedRand::SEED ? ctx_seed : uint256::ZERO};
    LogInfo("Setting random seed for current tests to %s=%s\n", RANDOM_CTX_SEED, seed.GetHex());
    MakeRandDeterministicDANGEROUS(seed);
    g_insecure_rand_ctx.Reseed(GetRandHash());
}
Repository: bitcoin/bitcoin
File: src/test/fuzz/bech32.cpp (C++)

Code before:
#include <util/strencodings.h>

#include <cassert>
#include <cstdint>
#include <string>
#include <utility>
#include <vector>

FUZZ_TARGET(bech32)
{
    const std::string random_string(buffer.begin(), buffer.end());
    const auto r1 = bech32::Decode(random_string);
    if (r1.hrp.empty()) {
        assert(r1.encoding == bech32::Encoding::INVALID);
        assert(r1.data.empty());
    } else {
        assert(r1.encoding != bech32::Encoding::INVALID);
        const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
        assert(CaseInsensitiveEqual(random_string, reencoded));
    }

    std::vector<unsigned char> input;
    ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());

    if (input.size() + 3 + 6 <= 90) {
        // If it's possible to encode input in Bech32(m) without exceeding the 90-character limit:
        for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
            const std::string encoded = bech32::Encode(encoding, "bc", input);
            assert(!encoded.empty());
            const auto r2 = bech32::Decode(encoded);
            assert(r2.encoding == encoding);
            assert(r2.hrp == "bc");
            assert(r2.data == input);
        }
    }
}


Review comment (suggestion):
nit: According to https://en.bitcoin.it/wiki/BIP_0173 `bc` + `1` is `human-readable part` + `separator`, if we're splitting these up, maybe this would be a better documentation:
```suggestion
    const auto hrp_size = 2, separator_size = 1;
    if (input.size() + hrp_size + separator_size + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
```

Code after:
#include <util/strencodings.h>

#include <cassert>
#include <cstdint>
#include <string>
#include <utility>
#include <vector>

FUZZ_TARGET(bech32)
{
    const std::string random_string(buffer.begin(), buffer.end());
    const auto r1 = bech32::Decode(random_string);
    if (r1.hrp.empty()) {
        assert(r1.encoding == bech32::Encoding::INVALID);
        assert(r1.data.empty());
    } else {
        assert(r1.encoding != bech32::Encoding::INVALID);
        const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
        assert(CaseInsensitiveEqual(random_string, reencoded));
    }

    std::vector<unsigned char> input;
    ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());

    // Input data part + 3 characters for the HRP and separator (bc1) + the checksum characters
    if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
        // If it's possible to encode input in Bech32(m) without exceeding the bech32-character limit:
        for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
            const std::string encoded = bech32::Encode(encoding, "bc", input);
            assert(!encoded.empty());
            const auto r2 = bech32::Decode(encoded);
            assert(r2.encoding == encoding);
            assert(r2.hrp == "bc");
            assert(r2.data == input);
        }
    }
}
Repository: bitcoin/bitcoin
File: src/test/fuzz/bech32.cpp (C++)

Code before:
#include <cstdint>
#include <string>
#include <utility>
#include <vector>

FUZZ_TARGET(bech32)
{
    const std::string random_string(buffer.begin(), buffer.end());
    const auto r1 = bech32::Decode(random_string);
    if (r1.hrp.empty()) {
        assert(r1.encoding == bech32::Encoding::INVALID);
        assert(r1.data.empty());
    } else {
        assert(r1.encoding != bech32::Encoding::INVALID);
        const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
        assert(CaseInsensitiveEqual(random_string, reencoded));
    }

    std::vector<unsigned char> input;
    ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());

    if (input.size() + 3 + 6 <= 90) {
        // If it's possible to encode input in Bech32(m) without exceeding the 90-character limit:
        for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
            const std::string encoded = bech32::Encode(encoding, "bc", input);
            assert(!encoded.empty());
            const auto r2 = bech32::Decode(encoded);
            assert(r2.encoding == encoding);
            assert(r2.hrp == "bc");
            assert(r2.data == input);
        }
    }
}


Review comment (suggestion):
according to the bip the hrp `MUST contain 1 to 83 US-ASCII characters` - maybe we could extend the fuzz testing from the hard-coded "bc"

Code after:
#include <cstdint>
#include <string>
#include <utility>
#include <vector>

FUZZ_TARGET(bech32)
{
    const std::string random_string(buffer.begin(), buffer.end());
    const auto r1 = bech32::Decode(random_string);
    if (r1.hrp.empty()) {
        assert(r1.encoding == bech32::Encoding::INVALID);
        assert(r1.data.empty());
    } else {
        assert(r1.encoding != bech32::Encoding::INVALID);
        const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
        assert(CaseInsensitiveEqual(random_string, reencoded));
    }

    std::vector<unsigned char> input;
    ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());

    // Input data part + 3 characters for the HRP and separator (bc1) + the checksum characters
    if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
        // If it's possible to encode input in Bech32(m) without exceeding the bech32-character limit:
        for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
            const std::string encoded = bech32::Encode(encoding, "bc", input);
            assert(!encoded.empty());
            const auto r2 = bech32::Decode(encoded);
            assert(r2.encoding == encoding);
            assert(r2.hrp == "bc");
            assert(r2.data == input);
        }
    }
}
Repository: bitcoin/bitcoin
File: src/test/util/setup_common.h (C/C++)

Code before:
    CKey coinbaseKey; // private/public key needed to spend coinbase transactions
};

/**
 * Make a test setup that has disk access to the debug.log file disabled. Can
 * be used in "hot loops", for example fuzzing or benchmarking.
 */
template <class T = const BasicTestingSetup>
std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
{
    opts.extra_args = Cat(
        {
            "-nodebuglogfile",
            "-nodebug",
        },
        opts.extra_args);

    return std::make_unique<T>(chain_type, opts);
}

CBlock getBlock13b8a();

// Make types usable in BOOST_CHECK_*
std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
std::ostream& operator<<(std::ostream& os, const uint160& num);
std::ostream& operator<<(std::ostream& os, const uint256& num);

/**
 * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.
 * Use as
 * BOOST_CHECK_EXCEPTION(code that throws, exception type, HasReason("foo"));
 */
class HasReason
{
public:
    explicit HasReason(const std::string& reason) : m_reason(reason) {}
    bool operator()(const std::exception& e) const
    {
        return std::string(e.what()).find(m_reason) != std::string::npos;
    };

private:
    const std::string m_reason;
};

#endif // BITCOIN_TEST_UTIL_SETUP_COMMON_H


Review comment (suggestion):
Good to group the output stream operators, but better to put them down here.

They are arguably less important (enabling Boost tests to work rather than defining new types like `ChainTestingSetup`) and puts them next to `BOOST_CHECK_EXCEPTION`-related code below.

Code after:
 * be used in "hot loops", for example fuzzing or benchmarking.
 */
template <class T = const BasicTestingSetup>
std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
{
    opts.extra_args = Cat(
        {
            "-nodebuglogfile",
            "-nodebug",
        },
        opts.extra_args);

    return std::make_unique<T>(chain_type, opts);
}

CBlock getBlock13b8a();

// Make types usable in BOOST_CHECK_* @{
namespace std {
template <typename T> requires std::is_enum_v<T>
inline std::ostream& operator<<(std::ostream& os, const T& e)
{
    return os << static_cast<std::underlying_type_t<T>>(e);
}

template <typename T>
inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
{
    return v ? os << *v
             : os << "std::nullopt";
}
} // namespace std

std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
std::ostream& operator<<(std::ostream& os, const uint160& num);
std::ostream& operator<<(std::ostream& os, const uint256& num);
// @}

/**
 * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.
 * Use as
 * BOOST_CHECK_EXCEPTION(code that throws, exception type, HasReason("foo"));
 */
class HasReason
{
public:
    explicit HasReason(const std::string& reason) : m_reason(reason) {}
    bool operator()(const std::exception& e) const
    {
        return std::string(e.what()).find(m_reason) != std::string::npos;
    };
Repository: bitcoin/bitcoin
File: src/test/fuzz/hex.cpp (C++)

Code before:
#include <univalue.h>
#include <util/strencodings.h>
#include <util/transaction_identifier.h>

#include <algorithm>
#include <cassert>
#include <cstdint>
#include <string>
#include <vector>

FUZZ_TARGET(hex)
{
    const std::string random_hex_string(buffer.begin(), buffer.end());
    const std::vector<unsigned char> data = ParseHex(random_hex_string);
    const std::vector<std::byte> bytes{ParseHex<std::byte>(random_hex_string)};
    assert(std::ranges::equal(AsBytes(Span{data}), bytes));
    const std::string hex_data = HexStr(data);
    if (IsHex(random_hex_string)) {
        assert(ToLower(random_hex_string) == hex_data);
    }
    if (uint256::FromHex(random_hex_string)) {
        assert(random_hex_string.length() == 64);
        assert(Txid::FromHex(random_hex_string));
        assert(Wtxid::FromHex(random_hex_string));
        assert(uint256::FromUserHex(random_hex_string));
    }
    if (const auto result{uint256::FromUserHex(random_hex_string)}) {
        assert(uint256::FromHex(result->ToString()));
    }
    try {
        (void)HexToPubKey(random_hex_string);
    } catch (const UniValue&) {
    }
    CBlockHeader block_header;
    (void)DecodeHexBlockHeader(block_header, random_hex_string);
    CBlock block;
    (void)DecodeHexBlk(block, random_hex_string);
}


Review comment (suggestion):
nit
```suggestion
        const auto result_string{result->ToString()};
```

Code after:
#include <univalue.h>
#include <util/strencodings.h>
#include <util/transaction_identifier.h>

#include <algorithm>
#include <cassert>
#include <cstdint>
#include <string>
#include <vector>

FUZZ_TARGET(hex)
{
    const std::string random_hex_string(buffer.begin(), buffer.end());
    const std::vector<unsigned char> data = ParseHex(random_hex_string);
    const std::vector<std::byte> bytes{ParseHex<std::byte>(random_hex_string)};
    assert(std::ranges::equal(AsBytes(Span{data}), bytes));
    const std::string hex_data = HexStr(data);
    if (IsHex(random_hex_string)) {
        assert(ToLower(random_hex_string) == hex_data);
    }
    if (uint256::FromHex(random_hex_string)) {
        assert(random_hex_string.length() == 64);
        assert(Txid::FromHex(random_hex_string));
        assert(Wtxid::FromHex(random_hex_string));
        assert(uint256::FromUserHex(random_hex_string));
    }
    if (const auto result{uint256::FromUserHex(random_hex_string)}) {
        const auto result_string{result->ToString()}; // ToString() returns a fixed-length string without "0x" prefix
        assert(result_string.length() == 64);
        assert(IsHex(result_string));
        assert(TryParseHex(result_string));
        assert(Txid::FromHex(result_string));
        assert(Wtxid::FromHex(result_string));
        assert(uint256::FromHex(result_string));
    }
    (void)uint256S(random_hex_string);
    try {
        (void)HexToPubKey(random_hex_string);
    } catch (const UniValue&) {
    }
    CBlockHeader block_header;
    (void)DecodeHexBlockHeader(block_header, random_hex_string);
    CBlock block;
    (void)DecodeHexBlk(block, random_hex_string);
}
Repository: bitcoin/bitcoin
File: src/wallet/wallet.cpp (C++)

Code before:
    return IsMine(txout.scriptPubKey);
}

isminetype CWallet::IsMine(const CTxDestination& dest) const
{
    AssertLockHeld(cs_wallet);
    return IsMine(GetScriptForDestination(dest));
}

isminetype CWallet::IsMine(const CScript& script) const
{
    AssertLockHeld(cs_wallet);

    // Search the cache so that IsMine is called only on the relevant SPKMs instead of on everything in m_spk_managers
    const auto& it = m_cached_spks.find(script);
    if (it != m_cached_spks.end()) {
        isminetype res = ISMINE_NO;
        for (const auto& spkm : it->second) {
            res = std::max(res, spkm->IsMine(script));
        }
        Assume(res == ISMINE_SPENDABLE);
        return res;
    }

    // Legacy wallet
    if (IsLegacy()) { // Blank legacy wallets have a null spkm
        if (LegacyScriptPubKeyMan* spkm = GetLegacyScriptPubKeyMan()) {
            return spkm->IsMine(script);
        }
    }

    return ISMINE_NO;
}

bool CWallet::IsMine(const CTransaction& tx) const
{
    AssertLockHeld(cs_wallet);
    for (const CTxOut& txout : tx.vout)
        if (IsMine(txout))
            return true;
    return false;
}

isminetype CWallet::IsMine(const COutPoint& outpoint) const
{
    AssertLockHeld(cs_wallet);
    auto wtx = GetWalletTx(outpoint.hash);
    if (!wtx) {
        return ISMINE_NO;
    }
    if (outpoint.n >= wtx->tx->vout.size()) {

Review comment (suggestion):
In 30f701317b4a674fea422766ac3ee179c9c6f554 "wallet: fix, detect blank legacy wallets in IsLegacy"

`IsLegacy()` is unnecessary here now since `GetLegacyScriptPubKeyMan()` also checks for `WALLET_FLAG_DESCRIPTORS`.

Code after:
    return IsMine(txout.scriptPubKey);
}

isminetype CWallet::IsMine(const CTxDestination& dest) const
{
    AssertLockHeld(cs_wallet);
    return IsMine(GetScriptForDestination(dest));
}

isminetype CWallet::IsMine(const CScript& script) const
{
    AssertLockHeld(cs_wallet);

    // Search the cache so that IsMine is called only on the relevant SPKMs instead of on everything in m_spk_managers
    const auto& it = m_cached_spks.find(script);
    if (it != m_cached_spks.end()) {
        isminetype res = ISMINE_NO;
        for (const auto& spkm : it->second) {
            res = std::max(res, spkm->IsMine(script));
        }
        Assume(res == ISMINE_SPENDABLE);
        return res;
    }

    // Legacy wallet
    if (LegacyScriptPubKeyMan* spkm = GetLegacyScriptPubKeyMan()) {
        return spkm->IsMine(script);
    }

    return ISMINE_NO;
}

bool CWallet::IsMine(const CTransaction& tx) const
{
    AssertLockHeld(cs_wallet);
    for (const CTxOut& txout : tx.vout)
        if (IsMine(txout))
            return true;
    return false;
}

isminetype CWallet::IsMine(const COutPoint& outpoint) const
{
    AssertLockHeld(cs_wallet);
    auto wtx = GetWalletTx(outpoint.hash);
    if (!wtx) {
        return ISMINE_NO;
    }
    if (outpoint.n >= wtx->tx->vout.size()) {
        return ISMINE_NO;
    }
Repository: bitcoin/bitcoin
File: test/functional/test_framework/test_node.py (Python)

Code before:
#!/usr/bin/env python3
# Copyright (c) 2017-2022 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Class for bitcoind node under test"""

import contextlib
import decimal
import errno
from enum import Enum
import http.client
import json
import logging
import os
import platform
import re
import subprocess
import tempfile
import time
import urllib.parse
import collections
import shlex
import sys
from pathlib import Path

from .authproxy import (
    JSONRPCException,
    serialization_fallback,
)
from .descriptors import descsum_create
from .messages import NODE_P2P_V2
from .p2p import P2P_SERVICES, P2P_SUBVERSION
from .util import (

Review comment (suggestion):
(Could possibly use `collections.defaultdict(int)` instead of `Counter` iff I really need to retouch).

Code after:
#!/usr/bin/env python3
# Copyright (c) 2017-present The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Class for bitcoind node under test"""

import contextlib
import decimal
import errno
from enum import Enum
import json
import logging
import os
import platform
import re
import subprocess
import tempfile
import time
import urllib.parse
import collections
import shlex
import sys
from pathlib import Path

from .authproxy import (
    JSONRPCException,
    serialization_fallback,
)
from .descriptors import descsum_create
from .messages import NODE_P2P_V2
from .p2p import P2P_SERVICES, P2P_SUBVERSION
from .util import (
    MAX_NODES,
Repository: bitcoin/bitcoin
File: test/functional/feature_blocksxor.py (Python)

Code before:
        assert_greater_than(len(block_files), 1)  # we want at least one full block file

        self.log.info("Shut down node and un-XOR block/undo files manually")
        self.stop_node(0)
        xor_key = read_xor_key(node=node)
        for data_file in sorted(block_files + undo_files):
            self.log.debug(f"Rewriting file {data_file}...")
            with open(data_file, 'rb+') as f:
                xored_data = f.read()
                f.seek(0)
                f.write(util_xor(xored_data, xor_key, offset=0))

        self.log.info("Check that restarting with 'blocksxor=0' fails if XOR key is present")
        node.assert_start_raises_init_error(['-blocksxor=0'],
            'The blocksdir XOR-key can not be disabled when a random key was already stored!',
            match=ErrorMatch.PARTIAL_REGEX)

        self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity")
        os.remove(node.blocks_path / 'xor.dat')
        self.start_node(0, extra_args=['-blocksxor=0'])
        # checklevel=2 -> verify block validity + undo data
        # nblocks=0    -> verify all blocks
        node.verifychain(checklevel=2, nblocks=0)


if __name__ == '__main__':
    BlocksXORTest(__file__).main()


Review comment (suggestion):
```suggestion
        assert_equal(read_xor_key(node=node), NULL_BLK_XOR_KEY)
```

Code after:
        assert_equal(len(block_files), len(undo_files))
        assert_greater_than(len(block_files), 1)  # we want at least one full block file

        self.log.info("Shut down node and un-XOR block/undo files manually")
        self.stop_node(0)
        xor_key = node.read_xor_key()
        for data_file in sorted(block_files + undo_files):
            self.log.debug(f"Rewriting file {data_file}...")
            with open(data_file, 'rb+') as f:
                xored_data = f.read()
                f.seek(0)
                f.write(util_xor(xored_data, xor_key, offset=0))

        self.log.info("Check that restarting with 'blocksxor=0' fails if XOR key is present")
        node.assert_start_raises_init_error(['-blocksxor=0'],
            'The blocksdir XOR-key can not be disabled when a random key was already stored!',
            match=ErrorMatch.PARTIAL_REGEX)

        self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity")
        node.blocks_key_path.unlink()
        self.start_node(0, extra_args=['-blocksxor=0'])
        # checklevel=2 -> verify block validity + undo data
        # nblocks=0    -> verify all blocks
        node.verifychain(checklevel=2, nblocks=0)
        self.log.info("Check that blocks XOR key is recreated")
        assert_equal(node.read_xor_key(), NULL_BLK_XOR_KEY)


if __name__ == '__main__':
    BlocksXORTest(__file__).main()