ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/crypto/siphash.h (C/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.

#ifndef BITCOIN_CRYPTO_SIPHASH_H
#define BITCOIN_CRYPTO_SIPHASH_H

#include <cstdint>
#include <span>

class uint256;

/** SipHash-2-4 */
class CSipHasher
{
private:
    uint64_t v[4];
    uint64_t tmp;
    uint8_t count; // Only the low 8 bits of the input size matter.

public:
    /** Construct a SipHash calculator initialized with 128-bit key (k0, k1) */
    CSipHasher(uint64_t k0, uint64_t k1);
    /** Hash a 64-bit integer worth of data
     *  It is treated as if this was the little-endian interpretation of 8 bytes.
     *  This function can only be used when a multiple of 8 bytes have been written so far.
     */
    CSipHasher& Write(uint64_t data);
    /** Hash arbitrary bytes. */
    CSipHasher& Write(std::span<const unsigned char> data);
    /** Compute the 64-bit SipHash-2-4 of the data written so far. The object remains untouched. */
    uint64_t Finalize() const;
};

/** Optimized SipHash-2-4 implementation for uint256.
 *
 *  It is identical to:
 *    SipHasher(k0, k1)
 *      .Write(val.GetUint64(0))
 *      .Write(val.GetUint64(1))

Review comment (refactor):
In commit "refactor: extract shared SipHash state into SipSalt"

Given that instances of this class are apprently used for the internal state in `CSipHasher` and `PresaltedSipHasher`, maybe it would be more appropriate to call it `SipHashState` or so?

Code after:
// 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.

#ifndef BITCOIN_CRYPTO_SIPHASH_H
#define BITCOIN_CRYPTO_SIPHASH_H

#include <array>
#include <cstdint>
#include <span>

class uint256;

/** Shared SipHash internal state v[0..3], initialized from (k0, k1). */
class SipHashState
{
    static constexpr uint64_t C0{0x736f6d6570736575ULL}, C1{0x646f72616e646f6dULL}, C2{0x6c7967656e657261ULL}, C3{0x7465646279746573ULL};

public:
    explicit SipHashState(uint64_t k0, uint64_t k1) noexcept : v{C0 ^ k0, C1 ^ k1, C2 ^ k0, C3 ^ k1} {}

    std::array<uint64_t, 4> v{};
};

/** General SipHash-2-4 implementation. */
class CSipHasher
{
    SipHashState m_state;
    uint64_t m_tmp{0};
    uint8_t m_count{0}; //!< Only the low 8 bits of the input size matter.

public:
    /** Construct a SipHash calculator initialized with 128-bit key (k0, k1). */
    CSipHasher(uint64_t k0, uint64_t k1);
    /** Hash a 64-bit integer worth of data.
     *  It is treated as if this was the little-endian interpretation of 8 bytes.
     *  This function can only be used when a multiple of 8 bytes have been written so far.
     */
    CSipHasher& Write(uint64_t data);
    /** Hash arbitrary bytes. */
Repository: bitcoin/bitcoin
File: src/uint256.cpp (C++)

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

#include <util/strencodings.h>

template <unsigned int BITS>
std::string base_blob<BITS>::GetHex() const
{
    uint8_t m_data_rev[WIDTH];
    for (int i = 0; i < WIDTH; ++i) {
        m_data_rev[i] = m_data[WIDTH - 1 - i];
    }
    return HexStr(m_data_rev);
}

template <unsigned int BITS>
void base_blob<BITS>::SetHex(const std::string_view str)
{
    std::fill(m_data.begin(), m_data.end(), 0);

    const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");

    // Note: if we are passed a greater number of digits than would fit as bytes
    // in m_data, we will be discarding the leftmost ones.
    // str="12bc" in a WIDTH=1 m_data => m_data[] == "\0xbc", not "0x12".
    size_t digits = 0;
    for (const char c : trimmed) {
        if (::HexDigit(c) == -1) break;
        ++digits;
    }
    unsigned char* p1 = m_data.data();
    unsigned char* pend = p1 + WIDTH;
    while (digits > 0 && p1 < pend) {
        *p1 = ::HexDigit(trimmed[--digits]);
        if (digits > 0) {
            *p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
            p1++;
        }
    }
}

template <unsigned int BITS>
std::string base_blob<BITS>::ToString() const
{

Review comment (suggestion):
We shouldn't need zeroing, if we're processing the values exhaustively - but if we do, we could use
```suggestion
    SetNull();
```

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

#include <util/strencodings.h>

template <unsigned int BITS>
std::string base_blob<BITS>::GetHex() const
{
    uint8_t m_data_rev[WIDTH];
    for (int i = 0; i < WIDTH; ++i) {
        m_data_rev[i] = m_data[WIDTH - 1 - i];
    }
    return HexStr(m_data_rev);
}

template <unsigned int BITS>
void base_blob<BITS>::SetHexDeprecated(const std::string_view str)
{
    std::fill(m_data.begin(), m_data.end(), 0);

    const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");

    // Note: if we are passed a greater number of digits than would fit as bytes
    // in m_data, we will be discarding the leftmost ones.
    // str="12bc" in a WIDTH=1 m_data => m_data[] == "\0xbc", not "0x12".
    size_t digits = 0;
    for (const char c : trimmed) {
        if (::HexDigit(c) == -1) break;
        ++digits;
    }
    unsigned char* p1 = m_data.data();
    unsigned char* pend = p1 + WIDTH;
    while (digits > 0 && p1 < pend) {
        *p1 = ::HexDigit(trimmed[--digits]);
        if (digits > 0) {
            *p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
            p1++;
        }
    }
}

template <unsigned int BITS>
std::string base_blob<BITS>::ToString() const
{
Repository: bitcoin/bitcoin
File: src/kernel/context.cpp (C++)

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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <string>

namespace kernel {
Context::Context()
{
    static bool initialized{false};
    if (!initialized) {
        std::string sha256_algo = SHA256AutoDetect();
        LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    }
    initialized = true;
}


} // namespace kernel


Review comment (bug):
While this code is being touched, do we want to use more specific logging instead of `LogPrintf`?  (e.g. `LogDebug`, `LogInfo`, etc.)

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging

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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <mutex>
#include <string>

namespace kernel {
Context::Context()
{
    static std::once_flag globals_initialized{};
    std::call_once(globals_initialized, []() {
        std::string sha256_algo = SHA256AutoDetect();
        LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    });
}


} // namespace kernel
Repository: bitcoin/bitcoin
File: src/kernel/context.cpp (C++)

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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <string>

namespace kernel {
Context::Context()
{
    static bool initialized{false};
    if (!initialized) {
        std::string sha256_algo = SHA256AutoDetect();
        LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    }
    initialized = true;
}


} // namespace kernel


Review comment (suggestion):
nit: `std::call_once` would be appropriate here I think, but the current approach is clear too so not important

<details>
<summary>git diff on 50c14e5a17</summary>

```diff
diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp
index a7c3457c8c..c34dc84704 100644
--- a/src/kernel/context.cpp
+++ b/src/kernel/context.cpp
@@ -8,19 +8,18 @@
 #include <logging.h>
 #include <random.h>
 
+#include <mutex>
 #include <string>
 
 namespace kernel {
 Context::Context()
 {
-    static bool initialized{false};
-    if (!initialized) {
+    static std::once_flag is_initialized;
+    std::call_once(is_initialized, [](){
         std::string sha256_algo = SHA256AutoDetect();
         LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
         RandomInit();
-    }
-    initialized = true;
+    });
 }
 
-
 } // namespace kernel

```
</details>


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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <mutex>
#include <string>

namespace kernel {
Context::Context()
{
    static std::once_flag globals_initialized{};
    std::call_once(globals_initialized, []() {
        std::string sha256_algo = SHA256AutoDetect();
        LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    });
}


} // namespace kernel
Repository: bitcoin/bitcoin
File: src/kernel/context.cpp (C++)

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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <string>

namespace kernel {
Context::Context()
{
    static bool initialized{false};
    if (!initialized) {
        std::string sha256_algo = SHA256AutoDetect();
        LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    }
    initialized = true;
}


} // namespace kernel


Review comment (suggestion):
nit: since the flag is about the globals, and not the context, being initialized - would `globals_initialized` be a better name? It doesn't make a difference now, but it will when the constructor does more than just setting up globals.

```suggestion
    static bool globals_initialized{false};
```

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

#include <crypto/sha256.h>
#include <logging.h>
#include <random.h>

#include <mutex>
#include <string>

namespace kernel {
Context::Context()
{
    static std::once_flag globals_initialized{};
    std::call_once(globals_initialized, []() {
        std::string sha256_algo = SHA256AutoDetect();
        LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
        RandomInit();
    });
}


} // namespace kernel
Repository: bitcoin/bitcoin
File: test/lint/test_runner/src/main.rs (Rust)

Code before:
    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_std_filesystem() -> LintResult {
    let found = git()
        .args([
            "grep",
            "std::filesystem",
            "--",
            "./src/",
            ":(exclude)src/util/fs.h",
        ])
        .status()
        .expect("command error")
        .success();
    if found {
        Err(r#"
^^^
Direct use of std::filesystem may be dangerous and buggy. Please include <util/fs.h> and use the
fs:: namespace, which has unsafe filesystem functions marked as deleted.
            "#
        .to_string())
    } else {
        Ok(())
    }
}

/// Return the pathspecs for whitespace related excludes
fn get_pathspecs_exclude_whitespace() -> Vec<String> {
    let mut list = get_pathspecs_exclude_subtrees();
    list.extend(
        [
            // Permanent excludes
            "*.patch",
            "src/qt/locale",
            "contrib/windeploy/win-codesign.cert",
            "doc/README_windows.txt",
            // Temporary excludes, or existing violations
            "doc/release-notes/release-notes-0.*",
            "contrib/init/bitcoind.openrc",

Review comment (suggestion):
```suggestion
    let files = check_output(
        git()
            .args(["ls-files", "--", "*.py"])
            .args(get_pathspecs_exclude_subtrees()),
    )?;
```
nit: exclude subtrees from this check

Code after:

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_py_mut_arg_default() -> LintResult {
    let bin_name = "ruff";
    let checks = ["B006", "B008"]
        .iter()
        .map(|c| format!("--select={}", c))
        .collect::<Vec<_>>();
    let files = check_output(
        git()
            .args(["ls-files", "--", "*.py"])
            .args(get_pathspecs_exclude_subtrees()),
    )?;

    let mut cmd = Command::new(bin_name);
    cmd.arg("check").args(checks).args(files.lines());

    match cmd.status() {
        Ok(status) if status.success() => Ok(()),
        Ok(_) => Err(format!("`{}` found errors!", bin_name)),
        Err(e) if e.kind() == ErrorKind::NotFound => {
            println!(
                "`{}` was not found in $PATH, skipping those checks.",
                bin_name
            );
            Ok(())
        }
        Err(e) => Err(format!("Error running `{}`: {}", bin_name, e)),
    }
}

fn lint_std_filesystem() -> LintResult {
    let found = git()
        .args([
Repository: bitcoin/bitcoin
File: test/lint/test_runner/src/main.rs (Rust)

Code before:
        Err("".to_string())
    }
}

fn lint_std_filesystem() -> LintResult {
    let found = git()
        .args([
            "grep",
            "std::filesystem",
            "--",
            "./src/",
            ":(exclude)src/util/fs.h",
        ])
        .status()
        .expect("command error")
        .success();
    if found {
        Err(r#"
^^^
Direct use of std::filesystem may be dangerous and buggy. Please include <util/fs.h> and use the
fs:: namespace, which has unsafe filesystem functions marked as deleted.
            "#
        .to_string())
    } else {
        Ok(())
    }
}

/// Return the pathspecs for whitespace related excludes
fn get_pathspecs_exclude_whitespace() -> Vec<String> {
    let mut list = get_pathspecs_exclude_subtrees();
    list.extend(
        [
            // Permanent excludes
            "*.patch",
            "src/qt/locale",
            "contrib/windeploy/win-codesign.cert",
            "doc/README_windows.txt",
            // Temporary excludes, or existing violations
            "doc/release-notes/release-notes-0.*",
            "contrib/init/bitcoind.openrc",
            "contrib/macdeploy/macdeployqtplus",
            "src/crypto/sha256_sse4.cpp",
            "src/qt/res/src/*.svg",
            "test/functional/test_framework/crypto/ellswift_decode_test_vectors.csv",
            "test/functional/test_framework/crypto/xswiftec_inv_test_vectors.csv",
            "contrib/qos/tc.sh",
            "contrib/verify-commits/gpg.sh",
            "src/univalue/include/univalue_escapes.h",
            "src/univalue/test/object.cpp",
            "test/lint/git-subtree-check.sh",

Review comment (suggestion):
nit:
```suggestion
                "`{}` was not found in $PATH, skipping this check.",
```

Code after:
            .success();
    }
    if good {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_py_mut_arg_default() -> LintResult {
    let bin_name = "ruff";
    let checks = ["B006", "B008"]
        .iter()
        .map(|c| format!("--select={}", c))
        .collect::<Vec<_>>();
    let files = check_output(
        git()
            .args(["ls-files", "--", "*.py"])
            .args(get_pathspecs_exclude_subtrees()),
    )?;

    let mut cmd = Command::new(bin_name);
    cmd.arg("check").args(checks).args(files.lines());

    match cmd.status() {
        Ok(status) if status.success() => Ok(()),
        Ok(_) => Err(format!("`{}` found errors!", bin_name)),
        Err(e) if e.kind() == ErrorKind::NotFound => {
            println!(
                "`{}` was not found in $PATH, skipping those checks.",
                bin_name
            );
            Ok(())
        }
        Err(e) => Err(format!("Error running `{}`: {}", bin_name, e)),
    }
}

fn lint_std_filesystem() -> LintResult {
    let found = git()
        .args([
            "grep",
            "std::filesystem",
            "--",
            "./src/",
            ":(exclude)src/util/fs.h",
        ])
        .status()
        .expect("command error")
        .success();
    if found {
Repository: bitcoin/bitcoin
File: test/lint/test_runner/src/main.rs (Rust)

Code before:
    if good {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_std_filesystem() -> LintResult {
    let found = git()
        .args([
            "grep",
            "std::filesystem",
            "--",
            "./src/",
            ":(exclude)src/util/fs.h",
        ])
        .status()
        .expect("command error")
        .success();
    if found {
        Err(r#"
^^^
Direct use of std::filesystem may be dangerous and buggy. Please include <util/fs.h> and use the
fs:: namespace, which has unsafe filesystem functions marked as deleted.
            "#
        .to_string())
    } else {
        Ok(())
    }
}

/// Return the pathspecs for whitespace related excludes
fn get_pathspecs_exclude_whitespace() -> Vec<String> {
    let mut list = get_pathspecs_exclude_subtrees();
    list.extend(
        [
            // Permanent excludes
            "*.patch",
            "src/qt/locale",
            "contrib/windeploy/win-codesign.cert",
            "doc/README_windows.txt",
            // Temporary excludes, or existing violations
            "doc/release-notes/release-notes-0.*",
            "contrib/init/bitcoind.openrc",
            "contrib/macdeploy/macdeployqtplus",
            "src/crypto/sha256_sse4.cpp",
            "src/qt/res/src/*.svg",
            "test/functional/test_framework/crypto/ellswift_decode_test_vectors.csv",
            "test/functional/test_framework/crypto/xswiftec_inv_test_vectors.csv",
            "contrib/qos/tc.sh",
            "contrib/verify-commits/gpg.sh",

Review comment (suggestion):
nit:

```suggestion
    cmd.arg("check").args(checks).args(files.lines()).arg("--quiet");
```

This would silence unhelpful messages about fixes not being available.


Code after:
            .arg(subtree)
            .status()
            .expect("command_error")
            .success();
    }
    if good {
        Ok(())
    } else {
        Err("".to_string())
    }
}

fn lint_py_mut_arg_default() -> LintResult {
    let bin_name = "ruff";
    let checks = ["B006", "B008"]
        .iter()
        .map(|c| format!("--select={}", c))
        .collect::<Vec<_>>();
    let files = check_output(
        git()
            .args(["ls-files", "--", "*.py"])
            .args(get_pathspecs_exclude_subtrees()),
    )?;

    let mut cmd = Command::new(bin_name);
    cmd.arg("check").args(checks).args(files.lines());

    match cmd.status() {
        Ok(status) if status.success() => Ok(()),
        Ok(_) => Err(format!("`{}` found errors!", bin_name)),
        Err(e) if e.kind() == ErrorKind::NotFound => {
            println!(
                "`{}` was not found in $PATH, skipping those checks.",
                bin_name
            );
            Ok(())
        }
        Err(e) => Err(format!("Error running `{}`: {}", bin_name, e)),
    }
}

fn lint_std_filesystem() -> LintResult {
    let found = git()
        .args([
            "grep",
            "std::filesystem",
            "--",
            "./src/",
            ":(exclude)src/util/fs.h",
        ])
        .status()
Repository: bitcoin/bitcoin
File: src/test/util/random.cpp (C++)

Code before:
// Copyright (c) 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 <test/util/random.h>

#include <logging.h>
#include <random.h>
#include <uint256.h>

#include <cstdlib>
#include <string>

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 (suggestion):
`constexpr` implies `inline` for functions, doesn't it also for variables? Never used `inline` on variables myself, but seems it is useful in **.h** files to avoid ODR-issues. This is a **.cpp** file though.

Code after:
// Copyright (c) 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 <test/util/random.h>

#include <logging.h>
#include <random.h>
#include <uint256.h>
#include <util/check.h>

#include <cstdlib>
#include <iostream>

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};