ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/common/system.h (C/C++)

Code before:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-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_COMMON_SYSTEM_H
#define BITCOIN_COMMON_SYSTEM_H

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

#include <cstdint>
#include <optional>
#include <string>

// Monotonic uptime in seconds (not affected by system time changes).
int64_t GetUptime();

void SetupEnvironment();
[[nodiscard]] bool SetupNetworking();
#ifndef WIN32
std::string ShellEscape(const std::string& arg);
#endif
#if HAVE_SYSTEM
void runCommand(const std::string& strCommand);
#endif

/**
 * Return the number of cores available on the current system.
 * @note This does count virtual cores, such as those provided by HyperThreading.
 */
int GetNumCores();

/**
 * Return the total RAM available on the current system, if detectable.
 */
std::optional<size_t> GetTotalRAM();

#endif // BITCOIN_COMMON_SYSTEM_H


Review comment (suggestion):
nit: You are changing all call-sites anyway, so my preference would be to return the full SteadyClock resolution here, and only cast at call-sites, when needed.

Code after:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-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_COMMON_SYSTEM_H
#define BITCOIN_COMMON_SYSTEM_H

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

#include <chrono>
#include <cstdint>
#include <optional>
#include <string>

/// Monotonic uptime (not affected by system time changes).
SteadyClock::duration GetUptime();

void SetupEnvironment();
[[nodiscard]] bool SetupNetworking();
#ifndef WIN32
std::string ShellEscape(const std::string& arg);
#endif
#if HAVE_SYSTEM
void runCommand(const std::string& strCommand);
#endif

/**
 * Return the number of cores available on the current system.
 * @note This does count virtual cores, such as those provided by HyperThreading.
 */
int GetNumCores();

/**
 * Return the total RAM available on the current system, if detectable.
 */
std::optional<size_t> GetTotalRAM();

#endif // BITCOIN_COMMON_SYSTEM_H
Repository: bitcoin/bitcoin
File: test/functional/rpc_uptime.py (Python)

Code before:
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        before = time.monotonic()

        uptime_before = self.nodes[0].uptime()
        self.nodes[0].setmocktime(int(time.time()) + 20_000_000)  # Simulate an NTP correction.
        uptime_after = self.nodes[0].uptime()
        self.nodes[0].setmocktime(0)

        after = time.monotonic()
        assert uptime_after - uptime_before <= int(after - before), "uptime should not jump with wall clock"


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


Review comment (suggestion):
```suggestion
        assert uptime_after - uptime_before <= wait_time, "uptime should not jump with wall clock"
```

Why not just keep the previous variable and logic? Possibly with wait_time=20_000

Code after:
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        wait_time = 20_000
        uptime_before = self.nodes[0].uptime()
        self.nodes[0].setmocktime(int(time.time()) + wait_time)
        uptime_after = self.nodes[0].uptime()
        self.nodes[0].setmocktime(0)
        assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"


if __name__ == '__main__':
    UptimeTest(__file__).main()
Repository: bitcoin/bitcoin
File: src/common/system.h (C/C++)

Code before:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-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_COMMON_SYSTEM_H
#define BITCOIN_COMMON_SYSTEM_H

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

#include <chrono>
#include <cstdint>
#include <optional>
#include <string>

// Monotonic uptime (not affected by system time changes).
SteadyClock::duration GetUptime();

void SetupEnvironment();
[[nodiscard]] bool SetupNetworking();
#ifndef WIN32
std::string ShellEscape(const std::string& arg);
#endif
#if HAVE_SYSTEM
void runCommand(const std::string& strCommand);
#endif

/**
 * Return the number of cores available on the current system.
 * @note This does count virtual cores, such as those provided by HyperThreading.
 */
int GetNumCores();

/**
 * Return the total RAM available on the current system, if detectable.
 */
std::optional<size_t> GetTotalRAM();

#endif // BITCOIN_COMMON_SYSTEM_H


Review comment (suggestion):
```suggestion
/// Monotonic uptime (not affected by system time changes).
```

nit: I think doxygen comments require three `///`.

Code after:
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-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_COMMON_SYSTEM_H
#define BITCOIN_COMMON_SYSTEM_H

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

#include <chrono>
#include <cstdint>
#include <optional>
#include <string>

/// Monotonic uptime (not affected by system time changes).
SteadyClock::duration GetUptime();

void SetupEnvironment();
[[nodiscard]] bool SetupNetworking();
#ifndef WIN32
std::string ShellEscape(const std::string& arg);
#endif
#if HAVE_SYSTEM
void runCommand(const std::string& strCommand);
#endif

/**
 * Return the number of cores available on the current system.
 * @note This does count virtual cores, such as those provided by HyperThreading.
 */
int GetNumCores();

/**
 * Return the total RAM available on the current system, if detectable.
 */
std::optional<size_t> GetTotalRAM();

#endif // BITCOIN_COMMON_SYSTEM_H
Repository: bitcoin/bitcoin
File: test/functional/rpc_uptime.py (Python)

Code before:
"""

import time

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        uptime_before = self.nodes[0].uptime()
        self.nodes[0].setmocktime(int(time.time()) + 20_000_000)
        uptime_after = self.nodes[0].uptime()
        self.nodes[0].setmocktime(0)
        assert uptime_after - uptime_before < 60, "uptime should not jump with wall clock"


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


Review comment (suggestion):
```suggestion
    assert uptime_after - uptime_before < wait_time  # uptime should not jump with wall clock
```

nit: Could use wait_time here, to avoid something hard-coded? (Here and in setmocktime)

Code after:
"""

import time

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        wait_time = 20_000
        uptime_before = self.nodes[0].uptime()
        self.nodes[0].setmocktime(int(time.time()) + wait_time)
        uptime_after = self.nodes[0].uptime()
        self.nodes[0].setmocktime(0)
        assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"


if __name__ == '__main__':
    UptimeTest(__file__).main()
Repository: bitcoin/bitcoin
File: test/functional/rpc_uptime.py (Python)

Code before:

import time

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        wait_time = 10
        self.nodes[0].setmocktime(int(time.time() + wait_time))
        assert self.nodes[0].uptime() >= wait_time


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


Review comment (refactor):
llm-nit: Possible places where comparison-specific test macros should replace generic comparisons:

* test/functional/rpc_uptime.py "assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"" -> recommendation: use the comparison helper by reversing operands, e.g. assert_greater_than(wait_time, uptime_after - uptime_before), to avoid a bare assert.


Code after:

import time

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_raises_rpc_error


class UptimeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1
        self.setup_clean_chain = True

    def run_test(self):
        self._test_negative_time()
        self._test_uptime()

    def _test_negative_time(self):
        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)

    def _test_uptime(self):
        wait_time = 20_000
        uptime_before = self.nodes[0].uptime()
        self.nodes[0].setmocktime(int(time.time()) + wait_time)
        uptime_after = self.nodes[0].uptime()
        self.nodes[0].setmocktime(0)
        assert uptime_after - uptime_before < wait_time, "uptime should not jump with wall clock"


if __name__ == '__main__':
    UptimeTest(__file__).main()
Repository: bitcoin/bitcoin
File: test/functional/test_framework/netutil.py (Python)

Code before:
    except socket.error:
        have_ipv6 = False
    return have_ipv6

def test_unix_socket():
    '''Return True if UNIX sockets are available on this platform.'''
    try:
        socket.AF_UNIX
    except AttributeError:
        return False
    else:
        return True

def format_addr_port(addr, port):
    '''Return either "addr:port" or "[addr]:port" based on whether addr looks like an IPv6 address.'''
    if ":" in addr:
        return f"[{addr}]:{port}"
    else:
        return f"{addr}:{port}"


def set_freebsd_high_port_range(sock):
    '''On FreeBSD, set socket to use the high ephemeral port range (49152-65535).

    FreeBSD's default ephemeral port range (10000-65535) overlaps with the test
    framework's static port range (11000-26000). Using IP_PORTRANGE_HIGH avoids
    this overlap when binding to port 0 for dynamic port allocation.
    '''
    if sys.platform.startswith('freebsd'):
        # Constants from FreeBSD's netinet/in.h and netinet6/in6.h
        IP_PORTRANGE = 19
        IPV6_PORTRANGE = 14
        IP_PORTRANGE_HIGH = 1  # Same value for both IPv4 and IPv6
        if sock.family == socket.AF_INET6:
            sock.setsockopt(socket.IPPROTO_IPV6, IPV6_PORTRANGE, IP_PORTRANGE_HIGH)
        else:
            sock.setsockopt(socket.IPPROTO_IP, IP_PORTRANGE, IP_PORTRANGE_HIGH)


Review comment (suggestion):
```suggestion
    framework's static port range starting at TEST_RUNNER_PORT_MIN (default=11000). Using IP_PORTRANGE_HIGH avoids
```

nit: Could avoid hard-coding the upper range, which may change in the future.

Code after:
    except socket.error:
        have_ipv6 = False
    return have_ipv6

def test_unix_socket():
    '''Return True if UNIX sockets are available on this platform.'''
    try:
        socket.AF_UNIX
    except AttributeError:
        return False
    else:
        return True

def format_addr_port(addr, port):
    '''Return either "addr:port" or "[addr]:port" based on whether addr looks like an IPv6 address.'''
    if ":" in addr:
        return f"[{addr}]:{port}"
    else:
        return f"{addr}:{port}"


def set_ephemeral_port_range(sock):
    '''On FreeBSD, set socket to use the high ephemeral port range (49152-65535).

    FreeBSD's default ephemeral port range (10000-65535) overlaps with the test
    framework's static port range starting at TEST_RUNNER_PORT_MIN (default=11000).
    Using IP_PORTRANGE_HIGH avoids this overlap when binding to port 0 for dynamic
    port allocation.
    '''
    if sys.platform.startswith('freebsd'):
        # Constants from FreeBSD's netinet/in.h and netinet6/in6.h
        IP_PORTRANGE = 19
        IPV6_PORTRANGE = 14
        IP_PORTRANGE_HIGH = 1  # Same value for both IPv4 and IPv6
        if sock.family == socket.AF_INET6:
            sock.setsockopt(socket.IPPROTO_IPV6, IPV6_PORTRANGE, IP_PORTRANGE_HIGH)
        else:
            sock.setsockopt(socket.IPPROTO_IP, IP_PORTRANGE, IP_PORTRANGE_HIGH)
Repository: bitcoin/bitcoin
File: ci/lint.py (Python)

Code before:
        *shlex.split(os.environ.get("DOCKER_BUILD_CACHE_ARG", "")),
        f"--file={repo_root}/ci/lint_imagefile",
        str(repo_root),
    ]
    if run(build_cmd, check=False).returncode != 0:
        if is_ci:
            print("Retry building image after failure")
            time.sleep(3)
        run(build_cmd)

    extra_env = []
    if is_ci:
        if os.environ.get("GITHUB_EVENT_NAME") == "pull_request":
            extra_env = ["--env", "LINT_CI_IS_PR=1"]
        elif os.environ.get("GITHUB_REPOSITORY") == "bitcoin/bitcoin":
            extra_env = ["--env", "LINT_CI_SANITY_CHECK_COMMIT_SIG=1"]

    run(
        [
            "docker",
            "run",
            "--rm",
            *extra_env,
            f"--volume={repo_root}:/bitcoin",
            *get_worktree_mounts(repo_root),
            *([] if is_ci else ["-it"]),
            container,
            *sys.argv[1:],
        ]
    )


if __name__ == "__main__":
    main()


Review comment (suggestion):
Would be nice to add a note documenting this in the README

e.g. just mentioning that you can do:

```bash
./ci/lint.py ./test/lint/lint-python.py
```

not blocking at all, feel free to disregard

Code after:
        str(repo_root),
    ]
    if run(build_cmd, check=False).returncode != 0:
        if is_ci:
            print("Retry building image after failure")
            time.sleep(3)
        run(build_cmd)

    extra_env = []
    if is_ci:
        if os.environ.get("GITHUB_EVENT_NAME") == "pull_request":
            extra_env = ["--env", "LINT_CI_IS_PR=1"]
        elif os.environ.get("GITHUB_REPOSITORY") == "bitcoin/bitcoin":
            extra_env = ["--env", "LINT_CI_SANITY_CHECK_COMMIT_SIG=1"]

    run(
        [
            "docker",
            "run",
            "--rm",
            *extra_env,
            f"--volume={repo_root}:/bitcoin",
            *get_worktree_mounts(repo_root),
            *([] if is_ci else ["-it"]),
            container,
            *sys.argv[1:],
        ]
    )


if __name__ == "__main__":
    main()
Repository: bitcoin/bitcoin
File: ci/lint.py (Python)

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

import os
import shlex
import subprocess
import sys
import time
from pathlib import Path

CONTAINER_NAME = "bitcoin-linter"


def run(cmd, **kwargs):
    print("+ " + shlex.join(cmd), flush=True)
    kwargs.setdefault("check", True)
    try:
        return subprocess.run(cmd, **kwargs)
    except Exception as e:
        sys.exit(str(e))


def get_worktree_mounts(repo_root):
    git_path = repo_root / ".git"
    if not git_path.is_file():
        return []
    content = git_path.read_text().strip()
    if not content.startswith("gitdir: "):
        return []
    gitdir = (repo_root / content.removeprefix("gitdir: ")).resolve()
    main_gitdir = gitdir.parent.parent
    return [
        f"--volume={gitdir}:{gitdir}",
        f"--volume={main_gitdir}:{main_gitdir}:ro",
    ]


def main():
    repo_root = Path(__file__).resolve().parent.parent
    is_ci = os.environ.get("GITHUB_ACTIONS") == "true"
    container = os.environ.get("CONTAINER_NAME", CONTAINER_NAME)

    build_cmd = [

Review comment (bug):
any reason to change this? seems better to leave this move-only? Probably an LLM bug, because they can't move code?

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

import os
import shlex
import subprocess
import sys
import time
from pathlib import Path


def run(cmd, **kwargs):
    print("+ " + shlex.join(cmd), flush=True)
    kwargs.setdefault("check", True)
    try:
        return subprocess.run(cmd, **kwargs)
    except Exception as e:
        sys.exit(str(e))


def get_worktree_mounts(repo_root):
    git_path = repo_root / ".git"
    if not git_path.is_file():
        return []
    content = git_path.read_text().strip()
    if not content.startswith("gitdir: "):
        return []
    gitdir = (repo_root / content.removeprefix("gitdir: ")).resolve()
    main_gitdir = gitdir.parent.parent
    return [
        f"--volume={gitdir}:{gitdir}",
        f"--volume={main_gitdir}:{main_gitdir}:ro",
    ]


def main():
    repo_root = Path(__file__).resolve().parent.parent
    is_ci = os.environ.get("GITHUB_ACTIONS") == "true"
    container = "bitcoin-linter"

    build_cmd = [
        "docker",
        "buildx",
Repository: bitcoin/bitcoin
File: ci/test/00_setup_env_native_chimera_lto.sh (Shell)

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

export LC_ALL=C.UTF-8

export CONTAINER_NAME=ci_native_chimera_musl
export CI_IMAGE_NAME_TAG="mirror.gcr.io/chimeralinux/chimera"
export CI_BASE_PACKAGES="ccache chimerautils chimerautils-extra clang cmake curl e2fsprogs git gmake gtar linux-headers ninja pkgconf procps python-devel python-pip rsync util-linux util-linux-lscpu xz"
export PIP_PACKAGES="--break-system-packages pyzmq pycapnp"
export DEP_OPTS="build_CC=clang build_CXX=clang++ build_TAR=gtar AR=llvm-ar CC=clang CXX=clang++ NM=llvm-nm RANLIB=llvm-ranlib STRIP=llvm-strip NO_QT=1"
export TEST_RUNNER_EXTRA="--exclude wallet_backwards_compatibility"
export GOAL="install"
export BITCOIN_CONFIG="\
 --preset=dev-mode \
 -DBUILD_GUI=OFF \
 -DREDUCE_EXPORTS=ON \
 -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON \
"


Review comment (bug):
it is a rolling release, which makes it less reproducible. Any commit that passes CI at one point may fail to pass in the at another point in time (and vice-versa).

Maybe it is annoying, but fine if older commits where the CI passed once no longer passes. Though, i worry about cases where a pull request fails to compile on one day, but not on the next day (or when it is merged). It is a bit like a constant rolling silent merge conflict, because the whole deps tree here is constantly rolling.

I think this task config is useful. However, the ideal place for nightly/rolling CI (like the nightly GCC or nightly clang CI) may not be in the main repo.

The easiest approach could be to run it along the other nightly CI. An alternative could be to split up another monotree repo for nightly CI.

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

# This config is experimental, and may not be reproducible, given
# the use of a rolling distro.

export LC_ALL=C.UTF-8

export CONTAINER_NAME=ci_native_chimera_musl
export CI_IMAGE_NAME_TAG="mirror.gcr.io/chimeralinux/chimera"
export CI_BASE_PACKAGES="ccache chimerautils chimerautils-extra clang cmake curl e2fsprogs git gmake gtar linux-headers ninja pkgconf procps python-devel python-pip rsync util-linux util-linux-lscpu xz"
export PIP_PACKAGES="--break-system-packages pyzmq pycapnp"
export DEP_OPTS="build_CC=clang build_CXX=clang++ build_TAR=gtar AR=llvm-ar CC=clang CXX=clang++ NM=llvm-nm RANLIB=llvm-ranlib STRIP=llvm-strip NO_QT=1"
export GOAL="install"
export BITCOIN_CONFIG="\
 --preset=dev-mode \
 -DBUILD_GUI=OFF \
 -DREDUCE_EXPORTS=ON \
 -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON \
"