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

Code before:
{
    LOCK(g_maplocalhost_mutex);
    LogPrintf("RemoveLocal(%s)\n", addr.ToStringAddrPort());
    mapLocalHost.erase(addr);
}

/** vote for a local address */
bool SeenLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    const auto it = mapLocalHost.find(addr);
    if (it == mapLocalHost.end()) return false;
    ++it->second.nScore;
    return true;
}


/** check whether a given address is potentially local */
bool IsLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    return mapLocalHost.count(addr) > 0;
}

CNode* CConnman::FindNode(const CNetAddr& ip)
{
    LOCK(m_nodes_mutex);
    for (CNode* pnode : m_nodes) {
      if (static_cast<CNetAddr>(pnode->addr) == ip) {
            return pnode;
        }
    }
    return nullptr;
}

CNode* CConnman::FindNode(const std::string& addrName)
{
    LOCK(m_nodes_mutex);
    for (CNode* pnode : m_nodes) {
        if (pnode->m_addr_name == addrName) {
            return pnode;
        }
    }
    return nullptr;
}

CNode* CConnman::FindNode(const CService& addr)
{
    LOCK(m_nodes_mutex);
    for (CNode* pnode : m_nodes) {
        if (static_cast<CService>(pnode->addr) == addr) {

Review comment (bug):
In commit "net: change FindNode() to not return a node and rename it"

I think this name change is confusing, because `CNode::m_addr_name` is not necessarily an address/port string; in the case of manually-connected nodes (`-addnode` and friends), it can also be a hostname.

Checking the codebase to verify this made me realize that the RPC help for `getpeerinfo` is also wrong in this regard.

Code after:
{
    LOCK(g_maplocalhost_mutex);
    LogPrintf("RemoveLocal(%s)\n", addr.ToStringAddrPort());
    mapLocalHost.erase(addr);
}

/** vote for a local address */
bool SeenLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    const auto it = mapLocalHost.find(addr);
    if (it == mapLocalHost.end()) return false;
    ++it->second.nScore;
    return true;
}


/** check whether a given address is potentially local */
bool IsLocal(const CService& addr)
{
    LOCK(g_maplocalhost_mutex);
    return mapLocalHost.count(addr) > 0;
}

bool CConnman::AlreadyConnectedToHost(const std::string& host) const
{
    LOCK(m_nodes_mutex);
    return std::ranges::any_of(m_nodes, [&host](CNode* node) { return node->m_addr_name == host; });
}

bool CConnman::AlreadyConnectedToAddressPort(const CService& addr_port) const
{
    LOCK(m_nodes_mutex);
    return std::ranges::any_of(m_nodes, [&addr_port](CNode* node) { return node->addr == addr_port; });
}

bool CConnman::AlreadyConnectedToAddress(const CNetAddr& addr) const
{
    LOCK(m_nodes_mutex);
    return std::ranges::any_of(m_nodes, [&addr](CNode* node) { return node->addr == addr; });
}

bool CConnman::CheckIncomingNonce(uint64_t nonce)
{
    LOCK(m_nodes_mutex);
    for (const CNode* pnode : m_nodes) {
        if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce)
            return false;
    }
    return true;
}
Repository: bitcoin/bitcoin
File: contrib/guix/security-check.py (Python)

Code before:
        lief.ARCHITECTURES.PPC: BASE_ELF + [('FORTIFY', check_ELF_FORTIFY)],
        lief.ARCHITECTURES.RISCV: BASE_ELF, # Skip FORTIFY. See https://github.com/lief-project/LIEF/issues/1082.
    },
    lief.EXE_FORMATS.PE: {
        lief.ARCHITECTURES.X86: BASE_PE,
    },
    lief.EXE_FORMATS.MACHO: {
        lief.ARCHITECTURES.X86: BASE_MACHO + [('PIE', check_PIE),
                                              ('NX', check_NX),
                                              ('CONTROL_FLOW', check_MACHO_CONTROL_FLOW)],
        lief.ARCHITECTURES.ARM64: BASE_MACHO + [('BRANCH_PROTECTION', check_MACHO_BRANCH_PROTECTION)],
    }
}

if __name__ == '__main__':
    retval: int = 0
    for filename in sys.argv[1:]:
        binary = lief.parse(filename)
        etype = binary.format
        arch = binary.abstract.header.architecture
        binary.concrete

        failed: list[str] = []
        for (name, func) in CHECKS[etype][arch]:
            if not func(binary):
                failed.append(name)
        if failed:
            print(f'{filename}: failed {" ".join(failed)}')
            retval = 1
    sys.exit(retval)


Review comment (suggestion):
Can you clarify why the casting was added; with it removed, I don't see any issues in our CI.

Code after:
]

CHECKS = {
    lief.Binary.FORMATS.ELF: {
        lief.Header.ARCHITECTURES.X86_64: BASE_ELF + [('CONTROL_FLOW', check_ELF_CONTROL_FLOW)],
        lief.Header.ARCHITECTURES.ARM: BASE_ELF,
        lief.Header.ARCHITECTURES.ARM64: BASE_ELF,
        lief.Header.ARCHITECTURES.PPC64: BASE_ELF,
        lief.Header.ARCHITECTURES.RISCV: BASE_ELF,
    },
    lief.Binary.FORMATS.PE: {
        lief.Header.ARCHITECTURES.X86_64: BASE_PE,
    },
    lief.Binary.FORMATS.MACHO: {
        lief.Header.ARCHITECTURES.X86_64: BASE_MACHO + [('PIE', check_PIE),
                                              ('NX', check_NX),
                                              ('CONTROL_FLOW', check_MACHO_CONTROL_FLOW)],
        lief.Header.ARCHITECTURES.ARM64: BASE_MACHO + [('BRANCH_PROTECTION', check_MACHO_BRANCH_PROTECTION)],
    }
}

if __name__ == '__main__':
    retval: int = 0
    for filename in sys.argv[1:]:
        binary = lief.parse(filename)

        etype = binary.format
        arch = binary.abstract.header.architecture

        failed: list[str] = []
        for (name, func) in CHECKS[etype][arch]:
            if not func(binary):
                failed.append(name)
        if failed:
            print(f'{filename}: failed {" ".join(failed)}')
            retval = 1
    sys.exit(retval)
Repository: bitcoin/bitcoin
File: src/checkqueue.h (C/C++)

Code before:
        m_worker_cv.notify_all();
        for (std::thread& t : m_worker_threads) {
            t.join();
        }
    }

    bool HasThreads() const { return !m_worker_threads.empty(); }
};

/**
 * RAII-style controller object for a CCheckQueue that guarantees the passed
 * queue is finished before continuing.
 */
template <typename T, typename R = std::remove_cvref_t<decltype(std::declval<T>()().value())>>
class CCheckQueueControl
{
private:
    CCheckQueue<T, R> * const pqueue;
    bool fDone;

public:
    CCheckQueueControl() = delete;
    CCheckQueueControl(const CCheckQueueControl&) = delete;
    CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
    explicit CCheckQueueControl(CCheckQueue<T> * const pqueueIn) : pqueue(pqueueIn), fDone(false)
    {
        // passed queue is supposed to be unused, or nullptr
        if (pqueue != nullptr) {
            ENTER_CRITICAL_SECTION(pqueue->m_control_mutex);
        }
    }

    std::optional<R> Complete()
    {
        if (pqueue == nullptr) return std::nullopt;
        auto ret = pqueue->Complete();
        fDone = true;
        return ret;
    }

    void Add(std::vector<T>&& vChecks)
    {
        if (pqueue != nullptr) {
            pqueue->Add(std::move(vChecks));
        }
    }

    ~CCheckQueueControl()
    {
        if (!fDone)
            Complete();

Review comment (bug):
I think it would be nice if we could get some kind of locking warning here if a second `CCheckQueueControl` is created from the same `CCheckQueue`, instead of just deadlocking. Tried coming up with a combination of LOCK and UNLOCK_FUNCTION annotation, but could not find something that actually worked.

Code after:
        m_worker_cv.notify_all();
        for (std::thread& t : m_worker_threads) {
            t.join();
        }
    }

    bool HasThreads() const { return !m_worker_threads.empty(); }
};

/**
 * RAII-style controller object for a CCheckQueue that guarantees the passed
 * queue is finished before continuing.
 */
template <typename T, typename R = std::remove_cvref_t<decltype(std::declval<T>()().value())>>
class SCOPED_LOCKABLE CCheckQueueControl
{
private:
    CCheckQueue<T, R>& m_queue;
    UniqueLock<Mutex> m_lock;
    bool fDone;

public:
    CCheckQueueControl() = delete;
    CCheckQueueControl(const CCheckQueueControl&) = delete;
    CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
    explicit CCheckQueueControl(CCheckQueue<T>& queueIn) EXCLUSIVE_LOCK_FUNCTION(queueIn.m_control_mutex) : m_queue(queueIn), m_lock(LOCK_ARGS(queueIn.m_control_mutex)), fDone(false) {}

    std::optional<R> Complete()
    {
        auto ret = m_queue.Complete();
        fDone = true;
        return ret;
    }

    void Add(std::vector<T>&& vChecks)
    {
        m_queue.Add(std::move(vChecks));
    }

    ~CCheckQueueControl() UNLOCK_FUNCTION()
    {
        if (!fDone)
            Complete();
    }
};

#endif // BITCOIN_CHECKQUEUE_H
Repository: bitcoin/bitcoin
File: src/wallet/test/util.h (C/C++)

Code before:
/** A WalletDatabase whose contents and return values can be modified as needed for testing
 **/
class MockableDatabase : public WalletDatabase
{
public:
    MockableData m_records;
    bool m_pass{true};

    MockableDatabase(MockableData records = {}) : WalletDatabase(), m_records(records) {}
    ~MockableDatabase() = default;

    void Open() override {}

    bool Rewrite(const char* pszSkip=nullptr) override { return m_pass; }
    bool Backup(const std::string& strDest) const override { return m_pass; }
    void Close() override {}

    std::string Filename() override { return "mockable"; }
    std::string Format() override { return "mock"; }
    std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableBatch>(m_records, m_pass); }
};

std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
MockableDatabase& GetMockableDatabase(CWallet& wallet);

ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
} // namespace wallet

#endif // BITCOIN_WALLET_TEST_UTIL_H


Review comment (suggestion):
In commit "wallet: Use util::Error throughout AddWalletDescriptor" (218db39973736ac50e912bec440b1e512586d3b0)

I think I understand the need to use std::reference_wrapper with util::Result. But it would seem good to replace `std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>>` here and other places with just `DescriptorScriptPubKeyMan*` for simplicity and avoid reference_wrapper except in the cases where it's really needed. It seems like doing this would make a lot of the test changes here unnecessary and the tests themselves more readable. 

I think use of reference_wrapper could to go away later with #25665 and normal references could be used.

Code after:
/** A WalletDatabase whose contents and return values can be modified as needed for testing
 **/
class MockableDatabase : public WalletDatabase
{
public:
    MockableData m_records;
    bool m_pass{true};

    MockableDatabase(MockableData records = {}) : WalletDatabase(), m_records(records) {}
    ~MockableDatabase() = default;

    void Open() override {}

    bool Rewrite(const char* pszSkip=nullptr) override { return m_pass; }
    bool Backup(const std::string& strDest) const override { return m_pass; }
    void Close() override {}

    std::string Filename() override { return "mockable"; }
    std::string Format() override { return "mock"; }
    std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableBatch>(m_records, m_pass); }
};

std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
MockableDatabase& GetMockableDatabase(CWallet& wallet);

DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
} // namespace wallet

#endif // BITCOIN_WALLET_TEST_UTIL_H
Repository: bitcoin/bitcoin
File: test/lint/test_runner/src/main.rs (Rust)

Code before:
/// Return all subtree paths
fn get_subtrees() -> Vec<&'static str> {
    vec![
        "src/crc32c",
        "src/crypto/ctaes",
        "src/leveldb",
        "src/minisketch",
        "src/secp256k1",
    ]
}

/// Return the pathspecs to exclude all subtrees
fn get_pathspecs_exclude_subtrees() -> Vec<String> {
    get_subtrees()
        .iter()
        .map(|s| format!(":(exclude){}", s))
        .collect()
}

fn lint_subtree() -> LintResult {
    // This only checks that the trees are pure subtrees, it is not doing a full
    // check with -r to not have to fetch all the remotes.
    let mut good = true;
    for subtree in get_subtrees() {
        good &= Command::new("test/lint/git-subtree-check.sh")
            .arg(subtree)
            .status()
            .expect("command_error")
            .success();
    }
    if good {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_scripted_diff() -> LintResult {
    if Command::new("test/lint/commit-script-check.sh")
        .arg(commit_range())
        .status()
        .expect("command error")
        .success()
    {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_commit_msg() -> LintResult {

Review comment (suggestion):
haven't tried it, but was wondering it this would also work with [chaining](https://doc.rust-lang.org/std/iter/struct.Chain.html#examples) the iterators?
```suggestion
    get_subtrees()
        .into_iter()
        .chain(["doc/release-notes/release-notes-*"]) // archived notes
```

Code after:
                .expect("check_output failed")
        )
    })
}

/// Return all subtree paths
fn get_subtrees() -> Vec<&'static str> {
    vec![
        "src/crc32c",
        "src/crypto/ctaes",
        "src/leveldb",
        "src/minisketch",
        "src/secp256k1",
    ]
}

/// Return the pathspecs to exclude by default
fn get_pathspecs_default_excludes() -> Vec<String> {
    get_subtrees()
        .iter()
        .chain(&[
            "doc/release-notes/release-notes-*", // archived notes
        ])
        .map(|s| format!(":(exclude){}", s))
        .collect()
}

fn lint_subtree() -> LintResult {
    // This only checks that the trees are pure subtrees, it is not doing a full
    // check with -r to not have to fetch all the remotes.
    let mut good = true;
    for subtree in get_subtrees() {
        good &= Command::new("test/lint/git-subtree-check.sh")
            .arg(subtree)
            .status()
            .expect("command_error")
            .success();
    }
    if good {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_scripted_diff() -> LintResult {
    if Command::new("test/lint/commit-script-check.sh")
        .arg(commit_range())
        .status()
        .expect("command error")
        .success()
Repository: bitcoin/bitcoin
File: src/bench/merkle_root.cpp (C++)

Code before:
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <bench/bench.h>
#include <consensus/merkle.h>
#include <random.h>
#include <uint256.h>

#include <vector>

static void MerkleRoot(benchmark::Bench& bench)
{
    FastRandomContext rng(true);
    std::vector<uint256> leaves;
    leaves.resize(9001);
    for (auto& item : leaves) {
        item = rng.rand256();
    }
    bench.batch(leaves.size()).unit("leaf").run([&] {
        bool mutation = false;
        uint256 hash = ComputeMerkleRoot(std::vector<uint256>(leaves), &mutation);
        leaves[mutation] = hash;
    });
}

BENCHMARK(MerkleRoot, benchmark::PriorityLevel::HIGH);


Review comment (suggestion):
nit: consistent coding style
```diff
-        for (size_t s = 0; s < hashes.size(); s++) {
+        for (auto s{0}; s < hashes.size(); ++s) {
```

Code after:
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <bench/bench.h>
#include <consensus/merkle.h>
#include <random.h>
#include <uint256.h>

#include <cassert>
#include <vector>

static void MerkleRoot(benchmark::Bench& bench)
{
    FastRandomContext rng{/*fDeterministic=*/true};

    std::vector<uint256> hashes{};
    hashes.resize(9001);
    for (auto& item : hashes) {
        item = rng.rand256();
    }

    constexpr uint256 expected_root{"d8d4dfd014a533bc3941b8663fa6e7f3a8707af124f713164d75b0c3179ecb08"};
    for (bool mutate : {false, true}) {
        bench.name(mutate ? "MerkleRootWithMutation" : "MerkleRoot").batch(hashes.size()).unit("leaf").run([&] {
            std::vector<uint256> leaves;
            leaves.reserve((hashes.size() + 1) & ~1ULL); // capacity rounded up to even
            for (size_t s = 0; s < hashes.size(); s++) {
                leaves.push_back(hashes[s]);
            }

            bool mutated{false};
            const uint256 root{ComputeMerkleRoot(std::move(leaves), mutate ? &mutated : nullptr)};
            assert(root == expected_root);
        });
    }
}

BENCHMARK(MerkleRoot, benchmark::PriorityLevel::HIGH);
Repository: bitcoin/bitcoin
File: src/bench/merkle_root.cpp (C++)

Code before:
// Copyright (c) 2016-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.

#include <bench/bench.h>
#include <consensus/merkle.h>
#include <random.h>
#include <uint256.h>

#include <vector>

static void MerkleRoot(benchmark::Bench& bench)
{
    FastRandomContext rng(true);
    std::vector<uint256> leaves;
    leaves.resize(9001);
    for (auto& item : leaves) {
        item = rng.rand256();
    }
    bench.batch(leaves.size()).unit("leaf").run([&] {
        bool mutation = false;
        uint256 hash = ComputeMerkleRoot(std::vector<uint256>(leaves), &mutation);
        leaves[mutation] = hash;
    });
}

BENCHMARK(MerkleRoot, benchmark::PriorityLevel::HIGH);


Review comment (refactor):
in 45dc62f0edf71d52ae6b024dec6b11746e3c4076 "refactor: make `MerkleRoot` benchmark more representative": Sending in the `&mutation` parameter makes the function perform more work (somewhat quadratically?). Are we sure we want to drop that?

Code after:
// Copyright (c) 2016-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.

#include <bench/bench.h>
#include <consensus/merkle.h>
#include <random.h>
#include <uint256.h>

#include <cassert>
#include <vector>

static void MerkleRoot(benchmark::Bench& bench)
{
    FastRandomContext rng{/*fDeterministic=*/true};

    std::vector<uint256> hashes{};
    hashes.resize(9001);
    for (auto& item : hashes) {
        item = rng.rand256();
    }

    constexpr uint256 expected_root{"d8d4dfd014a533bc3941b8663fa6e7f3a8707af124f713164d75b0c3179ecb08"};
    for (bool mutate : {false, true}) {
        bench.name(mutate ? "MerkleRootWithMutation" : "MerkleRoot").batch(hashes.size()).unit("leaf").run([&] {
            std::vector<uint256> leaves;
            leaves.reserve((hashes.size() + 1) & ~1ULL); // capacity rounded up to even
            for (size_t s = 0; s < hashes.size(); s++) {
                leaves.push_back(hashes[s]);
            }

            bool mutated{false};
            const uint256 root{ComputeMerkleRoot(std::move(leaves), mutate ? &mutated : nullptr)};
            assert(root == expected_root);
        });
    }
}

BENCHMARK(MerkleRoot, benchmark::PriorityLevel::HIGH);
Repository: bitcoin/bitcoin
File: src/test/merkle_tests.cpp (C++)

Code before:
    uint256 rootOfRightSubtree = BlockMerkleRoot(rightSubtreeBlock);
    std::vector<uint256> leftRight;
    leftRight.push_back(rootOfLeftSubtree);
    leftRight.push_back(rootOfRightSubtree);
    uint256 rootOfLR = ComputeMerkleRoot(leftRight);

    BOOST_CHECK_EQUAL(root, rootOfLR);
}

BOOST_AUTO_TEST_CASE(merkle_test_BlockWitness)
{
    CBlock block;

    constexpr size_t vtx_count{3};
    block.vtx.resize(vtx_count);
    for (std::size_t pos = 0; pos < vtx_count; pos++) {
        CMutableTransaction mtx;
        mtx.nLockTime = pos;
        block.vtx[pos] = MakeTransactionRef(std::move(mtx));
    }

    uint256 blockWitness = BlockWitnessMerkleRoot(block);

    std::vector<uint256> hashes;
    hashes.resize(vtx_count); // Note: leaving odd count to exercise old behavior
    for(size_t pos{1}; pos < vtx_count; pos++) {
        hashes[pos] = block.vtx[pos]->GetWitnessHash().ToUint256();
    }

    uint256 merkleRootofHashes = ComputeMerkleRoot(hashes);
    BOOST_CHECK_EQUAL(merkleRootofHashes, blockWitness);
}
BOOST_AUTO_TEST_SUITE_END()


Review comment (suggestion):
style: space after `for` and `++i` is preferred over `i++` (from `doc/developer-notes.md`).
```suggestion
    for (size_t pos{1}; pos < vtx_count; ++pos) {
```
(the loop a few lines above also uses `pos++`)

Code after:
    for (pos = block.vtx.size() / 2; pos < block.vtx.size(); pos++)
        rightSubtreeBlock.vtx.push_back(block.vtx[pos]);

    uint256 root = BlockMerkleRoot(block);
    uint256 rootOfLeftSubtree = BlockMerkleRoot(leftSubtreeBlock);
    uint256 rootOfRightSubtree = BlockMerkleRoot(rightSubtreeBlock);
    std::vector<uint256> leftRight;
    leftRight.push_back(rootOfLeftSubtree);
    leftRight.push_back(rootOfRightSubtree);
    uint256 rootOfLR = ComputeMerkleRoot(leftRight);

    BOOST_CHECK_EQUAL(root, rootOfLR);
}

BOOST_AUTO_TEST_CASE(merkle_test_BlockWitness)
{
    CBlock block;

    constexpr size_t vtx_count{3};
    block.vtx.resize(vtx_count);
    for (std::size_t pos = 0; pos < vtx_count; pos++) {
        CMutableTransaction mtx;
        mtx.nLockTime = pos;
        block.vtx[pos] = MakeTransactionRef(std::move(mtx));
    }

    uint256 blockWitness = BlockWitnessMerkleRoot(block);

    std::vector<uint256> hashes;
    hashes.resize(vtx_count); // Note: leaving odd count to exercise old behavior
    for (size_t pos{1}; pos < vtx_count; ++pos) {
        hashes[pos] = block.vtx[pos]->GetWitnessHash().ToUint256();
    }

    uint256 merkleRootofHashes = ComputeMerkleRoot(hashes);
    BOOST_CHECK_EQUAL(merkleRootofHashes, blockWitness);
}
BOOST_AUTO_TEST_SUITE_END()