💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774806)
Tried using `update()`, but it resulted in things like:
```
AssertionError: [node 0] Unable to connect to bitcoind after 10s (ignored errors: {'m': 40, 'i': 120, 's': 120, 'n': 80, 'g': 40, '_': 40, 'c': 40, 'r': 40, 'e': 80, 'd': 40, 't': 40, 'a': 40, 'l': 40}, latest error: ValueError('No RPC credentials'))
```
i.e. it was iterating over the string and incrementing an entry for each unique character, which seems to match the [docs](https://docs.python.org/3/library/collections.html#collect
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774806)
Tried using `update()`, but it resulted in things like:
```
AssertionError: [node 0] Unable to connect to bitcoind after 10s (ignored errors: {'m': 40, 'i': 120, 's': 120, 'n': 80, 'g': 40, '_': 40, 'c': 40, 'r': 40, 'e': 80, 'd': 40, 't': 40, 'a': 40, 'l': 40}, latest error: ValueError('No RPC credentials'))
```
i.e. it was iterating over the string and incrementing an entry for each unique character, which seems to match the [docs](https://docs.python.org/3/library/collections.html#collect
...
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826772195)
(Same issue with `update()`).
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826772195)
(Same issue with `update()`).
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774077)
Had to add a commit (2a0c0e410e9f32dbf7af7229457889b09553c438) that refurbishes `assert_raises_message()`, but test is much cleaner now!
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826774077)
Had to add a commit (2a0c0e410e9f32dbf7af7229457889b09553c438) that refurbishes `assert_raises_message()`, but test is much cleaner now!
👋 l0rinc's pull request is ready for review: "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144)
(https://github.com/bitcoin/bitcoin/pull/31144)
💬 hodlinator commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2453170834)
> The [toolchain file provided by `vcpkg`](https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/CMakePresets.json#L32) sets the [`CMAKE_FIND_ROOT_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_ROOT_PATH.html) variable, directing the `find_path()` and `find_library()` commands to the appropriate paths.
Okay, the toolchain file seems to be *C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg\scripts\buildsystems\vcpkg.cmake* in my case. D
...
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2453170834)
> The [toolchain file provided by `vcpkg`](https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/CMakePresets.json#L32) sets the [`CMAKE_FIND_ROOT_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_ROOT_PATH.html) variable, directing the `find_path()` and `find_library()` commands to the appropriate paths.
Okay, the toolchain file seems to be *C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg\scripts\buildsystems\vcpkg.cmake* in my case. D
...
👍 tdb3 approved a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2411750030)
Code Review re ACK 1148c4a53295e7635a813aadfde53ba5d973395c
Nice work cleaning up the existing code around the core changes. Increases readability.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2411750030)
Code Review re ACK 1148c4a53295e7635a813aadfde53ba5d973395c
Nice work cleaning up the existing code around the core changes. Increases readability.
💬 tdb3 commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826867112)
non-blocking pico-nit:
If this file gets touched again, could use `f` string instead of `format`.
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826867112)
non-blocking pico-nit:
If this file gets touched again, could use `f` string instead of `format`.
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826869399)
Good point! Resolved in latest push.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1826869399)
Good point! Resolved in latest push.
👍 tdb3 approved a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2411752016)
re ACK 042cee7073b338d79b5176d4157cf08bbd079b9f
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2411752016)
re ACK 042cee7073b338d79b5176d4157cf08bbd079b9f
👍 jasonribble approved a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2411753208)
ACK
I went through each of the commits. It is an improvement of the branch and bound selection tests.
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2411753208)
ACK
I went through each of the commits. It is an improvement of the branch and bound selection tests.
🤔 JoeyVee8666 reviewed a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2411758402)
Looks kinda sus...
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2411758402)
Looks kinda sus...
💬 JoeyVee8666 commented on pull request "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction":
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2453235424)
> <!--e57a25ab6845829454e8d69fc972939a-->
>
> The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
>
> <!--006a51241073e994b41acfe9ec718e94-->
> ### Code Coverage & Benchmarks
> For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31203.
> <!--021abf342d371248e50ceaed478a90ca-->
> ### Reviews
> See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process.
> | T
...
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2453235424)
> <!--e57a25ab6845829454e8d69fc972939a-->
>
> The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
>
> <!--006a51241073e994b41acfe9ec718e94-->
> ### Code Coverage & Benchmarks
> For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31203.
> <!--021abf342d371248e50ceaed478a90ca-->
> ### Reviews
> See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process.
> | T
...
💬 hebasto commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2453345360)
> > The [toolchain file provided by `vcpkg`](https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/CMakePresets.json#L32) sets the [`CMAKE_FIND_ROOT_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_ROOT_PATH.html) variable, directing the `find_path()` and `find_library()` commands to the appropriate paths.
>
> Okay, the toolchain file seems to be _C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg\scripts\buildsystems\vcpkg.cmake_ in my c
...
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2453345360)
> > The [toolchain file provided by `vcpkg`](https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/CMakePresets.json#L32) sets the [`CMAKE_FIND_ROOT_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_ROOT_PATH.html) variable, directing the `find_path()` and `find_library()` commands to the appropriate paths.
>
> Okay, the toolchain file seems to be _C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg\scripts\buildsystems\vcpkg.cmake_ in my c
...
📝 Abdulkbk opened a pull request: "Improve lockunspent validation for vout"
(https://github.com/bitcoin/bitcoin/pull/31205)
This PR adds a check for the vout value passed during the `lockunspent` RPC call. The code verifies if a floating-point number is provided and returns a descriptive error message.
(https://github.com/bitcoin/bitcoin/pull/31205)
This PR adds a check for the vout value passed during the `lockunspent` RPC call. The code verifies if a floating-point number is provided and returns a descriptive error message.
📝 JeremyRand opened a pull request: "doc: Use relative hyperlinks in release-process.md"
(https://github.com/bitcoin/bitcoin/pull/31206)
Improves usability with offline clones of the documentation.
Refs
https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127
(https://github.com/bitcoin/bitcoin/pull/31206)
Improves usability with offline clones of the documentation.
Refs
https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127
💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1826962618)
Thanks a lot @maflcko, that reproducer was super useful!
Added a `& 0xFF` (or do we prefer `0xff`? Found about the same number of occurrences in the code) and bumped the uint8_t to get rid of the overflow warning, see: https://github.com/bitcoin/bitcoin/compare/100cded580cfe9d69cc30866c29697d9658b4ce3..42066f45ff5d48e78a317eda63c035809bd657c6
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1826962618)
Thanks a lot @maflcko, that reproducer was super useful!
Added a `& 0xFF` (or do we prefer `0xff`? Found about the same number of occurrences in the code) and bumped the uint8_t to get rid of the overflow warning, see: https://github.com/bitcoin/bitcoin/compare/100cded580cfe9d69cc30866c29697d9658b4ce3..42066f45ff5d48e78a317eda63c035809bd657c6
💬 Veri-max commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2453411805)
> developer-notes.md says:
>
> > 1. Configure source file mapping.
> >
> > For `gdb` create or append to `.gdbinit` file:
> > ```
> > set substitute-path ./src /path/to/project/root/src
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > For `lldb` create or append to `.lldbinit` file:
> > ```
> > settings set target.source-map ./src /path/to/project/root/src
> > ```
>
> But I found I needed to create a `.lldbinit` file with
...
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2453411805)
> developer-notes.md says:
>
> > 1. Configure source file mapping.
> >
> > For `gdb` create or append to `.gdbinit` file:
> > ```
> > set substitute-path ./src /path/to/project/root/src
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > For `lldb` create or append to `.lldbinit` file:
> > ```
> > settings set target.source-map ./src /path/to/project/root/src
> > ```
>
> But I found I needed to create a `.lldbinit` file with
...
⚠️ JeremyRand opened an issue: "guix: Linux and macOS builds are not cross-arch reproducible with powerpc64le build arch"
(https://github.com/bitcoin/bitcoin/issues/31207)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The Guix output hashes for Windows and the source tarball match between my powerpc64le build machine and the official binaries, but the hashes for macOS and Linux do not match.
This seems to be a partial regression from 27.1, where Windows, Linux, and the source tarball all matched for me. macOS did not even build without errors for 27.1 on powerpc64le (that is at least fixed in 28.0),
...
(https://github.com/bitcoin/bitcoin/issues/31207)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The Guix output hashes for Windows and the source tarball match between my powerpc64le build machine and the official binaries, but the hashes for macOS and Linux do not match.
This seems to be a partial regression from 27.1, where Windows, Linux, and the source tarball all matched for me. macOS did not even build without errors for 27.1 on powerpc64le (that is at least fixed in 28.0),
...
📝 Abdulkbk converted_to_draft a pull request: "Improve lockunspent validation for vout"
(https://github.com/bitcoin/bitcoin/pull/31205)
This PR adds a check for the vout value passed during the `lockunspent` RPC call. The code verifies if a floating-point number is provided and returns a descriptive error message.
(https://github.com/bitcoin/bitcoin/pull/31205)
This PR adds a check for the vout value passed during the `lockunspent` RPC call. The code verifies if a floating-point number is provided and returns a descriptive error message.
💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827125042)
in commit 6fa10b4537b03b1746fd899de20b6f10dd6e15f0: seems like the ephemeral invariants check could return too early, potentially skipping lots of mempool txs?
```suggestion
if (dust_indexes.empty()) continue;
```
(same for the no-children condition below)
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827125042)
in commit 6fa10b4537b03b1746fd899de20b6f10dd6e15f0: seems like the ephemeral invariants check could return too early, potentially skipping lots of mempool txs?
```suggestion
if (dust_indexes.empty()) continue;
```
(same for the no-children condition below)