ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: test/functional/test_framework/test_node.py (Python)

Code before:
import shlex
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,
    assert_equal,
    append_config,
    delete_cookie_file,
    get_auth_cookie,
    get_rpc_proxy,
    rpc_url,
    wait_until_helper_internal,
    p2p_port,
    tor_port,
)

BITCOIND_PROC_WAIT_TIMEOUT = 60


class FailedToStartError(Exception):
    """Raised when a node fails to start correctly."""


class ErrorMatch(Enum):
    FULL_TEXT = 1
    FULL_REGEX = 2
    PARTIAL_REGEX = 3


class TestNode():
    """A class for representing a bitcoind node under test.

    This class contains:

    - state about the node (whether it's running, etc)
    - a Python subprocess.Popen object representing the running process
    - an RPC connection to the node
    - one or more P2P connections to the node


    To make things easier for the test writer, any unrecognised messages will
    be dispatched to the RPC connection."""

    def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False):

Review comment (suggestion):
an 's' got lost on the move up
```suggestion
# from InitBlocksdirXorKey::xor_key.size()
```

Code after:
import shlex
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,
    assert_equal,
    append_config,
    delete_cookie_file,
    get_auth_cookie,
    get_rpc_proxy,
    rpc_url,
    wait_until_helper_internal,
    p2p_port,
    tor_port,
)

BITCOIND_PROC_WAIT_TIMEOUT = 60
# The size of the blocks xor key
# from InitBlocksdirXorKey::xor_key.size()
NUM_XOR_BYTES = 8
# The null blocks key (all 0s)
NULL_BLK_XOR_KEY = bytes([0] * NUM_XOR_BYTES)


class FailedToStartError(Exception):
    """Raised when a node fails to start correctly."""


class ErrorMatch(Enum):
    FULL_TEXT = 1
    FULL_REGEX = 2
    PARTIAL_REGEX = 3


class TestNode():
    """A class for representing a bitcoind node under test.

    This class contains:

    - state about the node (whether it's running, etc)
    - a Python subprocess.Popen object representing the running process
    - an RPC connection to the node
    - one or more P2P connections to the node
Repository: bitcoin/bitcoin
File: src/bench/bench.cpp (C++)

Code before:
// Copyright (c) 2015-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 <test/util/setup_common.h>
#include <util/fs.h>
#include <util/string.h>

#include <chrono>
#include <fstream>
#include <functional>
#include <iostream>
#include <map>
#include <regex>
#include <string>
#include <vector>

using namespace std::chrono_literals;
using util::Join;

const std::function<void(const std::string&)> G_TEST_LOG_FUN{};

const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};

const std::function<std::string()> G_TEST_GET_FULL_NAME{};

namespace {

void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& file, const char* tpl)
{

Review comment (bug):
Review note: this include (and thus the new pragma) is necessary to compile `setup_common.cpp`. Without it, fails with

```
Undefined symbols for architecture arm64:
  "_G_TEST_COMMAND_LINE_ARGUMENTS", referenced from:
      BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o)
  "_G_TEST_GET_FULL_NAME", referenced from:
      BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o)
  "_G_TEST_LOG_FUN", referenced from:
      BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o)
ld: symbol(s) not found for architecture arm64
```

Code after:
// Copyright (c) 2015-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 <test/util/setup_common.h> // IWYU pragma: keep
#include <tinyformat.h>
#include <util/fs.h>
#include <util/string.h>

#include <chrono>
#include <compare>
#include <fstream>
#include <functional>
#include <iostream>
#include <map>
#include <ratio>
#include <regex>
#include <set>
#include <stdexcept>
#include <string>
#include <vector>

using namespace std::chrono_literals;
using util::Join;

const std::function<void(const std::string&)> G_TEST_LOG_FUN{};

const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};

const std::function<std::string()> G_TEST_GET_FULL_NAME{};
Repository: bitcoin/bitcoin
File: src/node/blockstorage.h (C/C++)

Code before:

    //! Create or update a prune lock identified by its name
    void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

    /** Open a block file (blk?????.dat) */
    AutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;

    /** Translation to a filesystem path */
    fs::path GetBlockPosFilename(const FlatFilePos& pos) const;

    /**
     *  Actually unlink the specified files
     */
    void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const;

    /** Functions for disk access for blocks */
    bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) const;
    bool ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) const;
    bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos) const;

    bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& index) const;

    void CleanupBlockRevFiles() const;
};

void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> vImportFiles);
} // namespace node

#endif // BITCOIN_NODE_BLOCKSTORAGE_H


Review comment (suggestion):
nit: would be nice to modernize the name to e.g. `import_paths` since it's no longer a vector.
```suggestion
void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_paths);
```

Code after:

    //! Create or update a prune lock identified by its name
    void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

    /** Open a block file (blk?????.dat) */
    AutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;

    /** Translation to a filesystem path */
    fs::path GetBlockPosFilename(const FlatFilePos& pos) const;

    /**
     *  Actually unlink the specified files
     */
    void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const;

    /** Functions for disk access for blocks */
    bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) const;
    bool ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) const;
    bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos) const;

    bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& index) const;

    void CleanupBlockRevFiles() const;
};

void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_paths);
} // namespace node

#endif // BITCOIN_NODE_BLOCKSTORAGE_H
Repository: bitcoin/bitcoin
File: src/test/util/setup_common.cpp (C++)

Code before:
#include <util/thread.h>
#include <util/threadnames.h>
#include <util/time.h>
#include <util/translation.h>
#include <util/vector.h>
#include <validation.h>
#include <validationinterface.h>
#include <walletinitinterface.h>

#include <algorithm>
#include <functional>
#include <stdexcept>

using namespace util::hex_literals;
using kernel::BlockTreeDB;
using node::ApplyArgsManOptions;
using node::BlockAssembler;
using node::BlockManager;
using node::CalculateCacheSizes;
using node::KernelNotifications;
using node::LoadChainstate;
using node::RegenerateCommitments;
using node::VerifyLoadedChainstate;

const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;

/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
static FastRandomContext g_rng_temp_path;

std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
{
    os << num.ToString();
    return os;
}

std::ostream& operator<<(std::ostream& os, const uint160& num)
{
    os << num.ToString();
    return os;
}

std::ostream& operator<<(std::ostream& os, const uint256& num)
{
    os << num.ToString();
    return os;
}

struct NetworkSetup
{
    NetworkSetup()
    {

Review comment (security):
`constexpr inline` is used to abide by the One Definition Rule when putting variables in header files. This is a .CPP file.

Also would have added an extra newline to separate it from the commented code-section above.
```suggestion

constexpr auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
```

Code after:
#include <util/time.h>
#include <util/translation.h>
#include <util/vector.h>
#include <validation.h>
#include <validationinterface.h>
#include <walletinitinterface.h>

#include <algorithm>
#include <functional>
#include <stdexcept>

using kernel::BlockTreeDB;
using node::ApplyArgsManOptions;
using node::BlockAssembler;
using node::BlockManager;
using node::CalculateCacheSizes;
using node::KernelNotifications;
using node::LoadChainstate;
using node::RegenerateCommitments;
using node::VerifyLoadedChainstate;

const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;

constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
static FastRandomContext g_rng_temp_path;

std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
{
    os << num.ToString();
    return os;
}

std::ostream& operator<<(std::ostream& os, const uint160& num)
{
    os << num.ToString();
    return os;
}

std::ostream& operator<<(std::ostream& os, const uint256& num)
{
    os << num.ToString();
    return os;
}

struct NetworkSetup
{
    NetworkSetup()
    {
        Assert(SetupNetworking());
    }
Repository: bitcoin/bitcoin
File: test/lint/test_runner/src/main.rs (Rust)

Code before:
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

use std::env;
use std::fs;
use std::io::ErrorKind;
use std::path::{Path, PathBuf};
use std::process::{Command, ExitCode, Stdio};

type LintError = String;
type LintResult = Result<(), LintError>;
type LintFn = fn() -> LintResult;

struct Linter {
    pub description: &'static str,
    pub name: &'static str,
    pub lint_fn: LintFn,
}

fn get_linter_list() -> Vec<&'static Linter> {
    vec![
        &Linter {
            description: "Check that all command line arguments are documented.",
            name: "doc",
            lint_fn: lint_doc
        },
        &Linter {
            description: "Check that no symbol from bitcoin-config.h is used without the header being included",
            name: "includes_build_config",
            lint_fn: lint_includes_build_config
        },
        &Linter {
            description: "Check that markdown links resolve",
            name: "markdown",
            lint_fn: lint_markdown
        },

Review comment (bug):
a different linebreak would make this slightly more readable:
```suggestion
/// A possible error returned by any of the linters.
/// The error string should explain the failure type and list all violations.
```

Code after:
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

use std::env;
use std::fs;
use std::io::ErrorKind;
use std::path::PathBuf;
use std::process::{Command, ExitCode, Stdio};

/// A possible error returned by any of the linters.
///
/// The error string should explain the failure type and list all violations.
type LintError = String;
type LintResult = Result<(), LintError>;
type LintFn = fn() -> LintResult;

struct Linter {
    pub description: &'static str,
    pub name: &'static str,
    pub lint_fn: LintFn,
}

fn get_linter_list() -> Vec<&'static Linter> {
    vec![
        &Linter {
            description: "Check that all command line arguments are documented.",
            name: "doc",
            lint_fn: lint_doc
        },
        &Linter {
            description: "Check that no symbol from bitcoin-config.h is used without the header being included",
            name: "includes_build_config",
            lint_fn: lint_includes_build_config
        },
        &Linter {
            description: "Check that markdown links resolve",
Repository: bitcoin/bitcoin
File: src/util/fs_helpers.cpp (C++)

Code before:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2023 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 <util/fs_helpers.h>

#include <config/bitcoin-config.h> // IWYU pragma: keep

#include <logging.h>
#include <sync.h>
#include <util/fs.h>
#include <util/syserror.h>

#include <cerrno>
#include <fstream>
#include <map>
#include <memory>
#include <optional>
#include <string>
#include <system_error>
#include <utility>

#ifndef WIN32
// for posix_fallocate, in configure.ac we check if it is present after this
#ifdef __linux__

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#define _POSIX_C_SOURCE 200112L

#endif // __linux__

#include <fcntl.h>
#include <sys/resource.h>
#include <unistd.h>
#else
#include <io.h> /* For _get_osfhandle, _chsize */
#include <shlobj.h> /* For SHGetSpecialFolderPathW */
#endif // WIN32

/** Mutex to protect dir_locks. */
static GlobalMutex cs_dir_locks;
/** A map that contains all the currently held directory locks. After
 * successful locking, these will be held here until the global destructor
 * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks
 * is called.
 */

Review comment (suggestion):
```suggestion
// for posix_fallocate, in cmake/introspection.cmake we check if it is present after this
```

Code after:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2023 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 <util/fs_helpers.h>

#include <config/bitcoin-config.h> // IWYU pragma: keep

#include <logging.h>
#include <sync.h>
#include <util/fs.h>
#include <util/syserror.h>

#include <cerrno>
#include <fstream>
#include <map>
#include <memory>
#include <optional>
#include <string>
#include <system_error>
#include <utility>

#ifndef WIN32
// for posix_fallocate, in cmake/introspection.cmake we check if it is present after this
#ifdef __linux__

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#define _POSIX_C_SOURCE 200112L

#endif // __linux__

#include <fcntl.h>
#include <sys/resource.h>
#include <unistd.h>
#else
#include <io.h> /* For _get_osfhandle, _chsize */
#include <shlobj.h> /* For SHGetSpecialFolderPathW */
#endif // WIN32

/** Mutex to protect dir_locks. */
static GlobalMutex cs_dir_locks;
/** A map that contains all the currently held directory locks. After
 * successful locking, these will be held here until the global destructor
 * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks
 * is called.
 */
Repository: bitcoin/bitcoin
File: src/streams.cpp (C++)

Code before:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

#include <span.h>
#include <streams.h>

#include <array>

std::size_t AutoFile::detail_fread(Span<std::byte> dst)
{
    if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
    if (m_xor.empty()) {
        return std::fread(dst.data(), 1, dst.size(), m_file);
    } else {
        const auto init_pos{std::ftell(m_file)};
        if (init_pos < 0) throw std::ios_base::failure("AutoFile::read: ftell failed");
        std::size_t ret{std::fread(dst.data(), 1, dst.size(), m_file)};
        util::Xor(dst.subspan(0, ret), m_xor, init_pos);
        return ret;
    }
}

void AutoFile::seek(int64_t offset, int origin)
{
    if (IsNull()) {
        throw std::ios_base::failure("AutoFile::seek: file handle is nullptr");
    }
    if (std::fseek(m_file, offset, origin) != 0) {
        throw std::ios_base::failure(feof() ? "AutoFile::seek: end of file" : "AutoFile::seek: fseek failed");
    }
}

int64_t AutoFile::tell()
{
    if (IsNull()) {

Review comment (suggestion):
clang-tidy seems [upset](https://github.com/bitcoin/bitcoin/pull/30884/checks?check_run_id=30079004954) about setting `m_position{0}` here instead of using C++11 "Default member initialization" in the class declaration (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)

Code after:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

#include <span.h>
#include <streams.h>
#include <util/fs_helpers.h>

#include <array>

AutoFile::AutoFile(std::FILE* file, std::vector<std::byte> data_xor)
    : m_file{file}, m_xor{std::move(data_xor)}
{
    if (!IsNull()) {
        auto pos{std::ftell(m_file)};
        if (pos >= 0) m_position = pos;
    }
}

std::size_t AutoFile::detail_fread(Span<std::byte> dst)
{
    if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
    size_t ret = std::fread(dst.data(), 1, dst.size(), m_file);
    if (!m_xor.empty()) {
        if (!m_position.has_value()) throw std::ios_base::failure("AutoFile::read: position unknown");
        util::Xor(dst.subspan(0, ret), m_xor, *m_position);
    }
    if (m_position.has_value()) *m_position += ret;
    return ret;
}

void AutoFile::seek(int64_t offset, int origin)
{
    if (IsNull()) {
        throw std::ios_base::failure("AutoFile::seek: file handle is nullptr");
    }
Repository: bitcoin/bitcoin
File: src/streams.cpp (C++)

Code before:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

#include <span.h>
#include <streams.h>

#include <array>

std::size_t AutoFile::detail_fread(Span<std::byte> dst)
{
    if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
    if (m_xor.empty()) {
        return std::fread(dst.data(), 1, dst.size(), m_file);
    } else {
        const auto init_pos{std::ftell(m_file)};
        if (init_pos < 0) throw std::ios_base::failure("AutoFile::read: ftell failed");
        std::size_t ret{std::fread(dst.data(), 1, dst.size(), m_file)};
        util::Xor(dst.subspan(0, ret), m_xor, init_pos);
        return ret;
    }
}

void AutoFile::seek(int64_t offset, int origin)
{
    if (IsNull()) {
        throw std::ios_base::failure("AutoFile::seek: file handle is nullptr");
    }
    if (std::fseek(m_file, offset, origin) != 0) {
        throw std::ios_base::failure(feof() ? "AutoFile::seek: end of file" : "AutoFile::seek: fseek failed");
    }
}

int64_t AutoFile::tell()
{
    if (IsNull()) {
        throw std::ios_base::failure("AutoFile::tell: file handle is nullptr");
    }
    int64_t r{std::ftell(m_file)};
    if (r < 0) {
        throw std::ios_base::failure("AutoFile::tell: ftell failed");

Review comment (bug):
nit: Handle ftell error

```suggestion
        if (pos < 0) {
            throw std::ios_base::failure("AutoFile::seek: ftell failed");
        }
        m_position = pos;
```

Code after:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

#include <span.h>
#include <streams.h>
#include <util/fs_helpers.h>

#include <array>

AutoFile::AutoFile(std::FILE* file, std::vector<std::byte> data_xor)
    : m_file{file}, m_xor{std::move(data_xor)}
{
    if (!IsNull()) {
        auto pos{std::ftell(m_file)};
        if (pos >= 0) m_position = pos;
    }
}

std::size_t AutoFile::detail_fread(Span<std::byte> dst)
{
    if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
    size_t ret = std::fread(dst.data(), 1, dst.size(), m_file);
    if (!m_xor.empty()) {
        if (!m_position.has_value()) throw std::ios_base::failure("AutoFile::read: position unknown");
        util::Xor(dst.subspan(0, ret), m_xor, *m_position);
    }
    if (m_position.has_value()) *m_position += ret;
    return ret;
}

void AutoFile::seek(int64_t offset, int origin)
{
    if (IsNull()) {
        throw std::ios_base::failure("AutoFile::seek: file handle is nullptr");
    }
    if (std::fseek(m_file, offset, origin) != 0) {
        throw std::ios_base::failure(feof() ? "AutoFile::seek: end of file" : "AutoFile::seek: fseek failed");
    }
    if (origin == SEEK_SET) {
        m_position = offset;