💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716737380)
> That makes sense, but I don't quite see the case for changing `GetNetworkForMagic` to accept a span in the future, so it feels a bit like premature optimization to me, but I may be missing context.
Yeah, it is hard to make predictions about the future. I was mostly thinking about a `DataStream` or `std::vector` (or similar) object, and someone wanting to just call `GetNetworkForMagic(std::span{obj}.first(4))`, without having to write code to create a copy of the first bytes.
It is mostly
...
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716737380)
> That makes sense, but I don't quite see the case for changing `GetNetworkForMagic` to accept a span in the future, so it feels a bit like premature optimization to me, but I may be missing context.
Yeah, it is hard to make predictions about the future. I was mostly thinking about a `DataStream` or `std::vector` (or similar) object, and someone wanting to just call `GetNetworkForMagic(std::span{obj}.first(4))`, without having to write code to create a copy of the first bytes.
It is mostly
...
💬 willcl-ark commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288487975)
FWIW I am maintaing some (very unofficial, but up-to-date!) docker images using this repo: https://github.com/willcl-ark/bitcoin-core-docker, just to add to those already mentioned in the discussion over at https://github.com/bitcoin-core/packaging/issues/55
It has currently-supported tags, as well as an automatic nightly master build.
IMO this issue can probably be closed in this repo. I don't think there is much appetite to maintain official docker images in this project and I think an i
...
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288487975)
FWIW I am maintaing some (very unofficial, but up-to-date!) docker images using this repo: https://github.com/willcl-ark/bitcoin-core-docker, just to add to those already mentioned in the discussion over at https://github.com/bitcoin-core/packaging/issues/55
It has currently-supported tags, as well as an automatic nightly master build.
IMO this issue can probably be closed in this repo. I don't think there is much appetite to maintain official docker images in this project and I think an i
...
🤔 paplorinc reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237589001)
I see this has been open for some time, if I missed an old comment that is still relevant, let me know.
I would prefer some simplifications, since the code becase slightly more complex and I think there are a few simple fixes for that.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237589001)
I see this has been open for some time, if I missed an old comment that is still relevant, let me know.
I would prefer some simplifications, since the code becase slightly more complex and I think there are a few simple fixes for that.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716574220)
<3
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716574220)
<3
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998)
nit: now that the code is available here, the the comment bacame redundant, since it doesn't provide any info that the code doesn't already explain
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998)
nit: now that the code is available here, the the comment bacame redundant, since it doesn't provide any info that the code doesn't already explain
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716616576)
internal whitespaces are weird enough - but do we still encourage mixed casing?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716616576)
internal whitespaces are weird enough - but do we still encourage mixed casing?
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716648031)
we've checked already that Size is always odd, it seems to me that we can safely truncate:
```suggestion
std::array<Byte, Size / 2> rv{};
```
and
```C++
std::array<Byte, Size / 2>
```
in the signature
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716648031)
we've checked already that Size is always odd, it seems to me that we can safely truncate:
```suggestion
std::array<Byte, Size / 2> rv{};
```
and
```C++
std::array<Byte, Size / 2>
```
in the signature
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716610850)
Nice!
Alternatively, since we claim to be testing whitespace support - and we've already tested that non-whitespace values are parsed correctly -, we might as well compare `ParseHex("12 34 56 78")` to `ParseHex("12345678")` instead - otherwise all tests would fail all the time when an error is introduced, this way only the appropriate tests would fail (+ testing is simpler).
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716610850)
Nice!
Alternatively, since we claim to be testing whitespace support - and we've already tested that non-whitespace values are parsed correctly -, we might as well compare `ParseHex("12 34 56 78")` to `ParseHex("12345678")` instead - otherwise all tests would fail all the time when an error is introduced, this way only the appropriate tests would fail (+ testing is simpler).
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716685019)
Could we simplify some of these declarations (at least in tests) to e.g.
```suggestion
auto expected{ArrayFromHex("971a55")};
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716685019)
Could we simplify some of these declarations (at least in tests) to e.g.
```suggestion
auto expected{ArrayFromHex("971a55")};
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716622610)
Can me modernized/simplified:
```suggestion
std::vector<unsigned char> expected(std::begin(ParseHex_expected), std::end(ParseHex_expected));
```
(nit: weird `ParseHex_expected` naming)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716622610)
Can me modernized/simplified:
```suggestion
std::vector<unsigned char> expected(std::begin(ParseHex_expected), std::end(ParseHex_expected));
```
(nit: weird `ParseHex_expected` naming)
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716705665)
Do we even need the vector conversion here?
```C++
return CScript(data.begin(), data.end());
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716705665)
Do we even need the vector conversion here?
```C++
return CScript(data.begin(), data.end());
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716709558)
Checked that `ScriptFromHex` is only used for non-static values now 👍
Though I'm not sure `ToScript(ArrayFromHex("0302ff03"))` is more readable (or performant) than `ScriptFromHex("0302ff03"`.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716709558)
Checked that `ScriptFromHex` is only used for non-static values now 👍
Though I'm not sure `ToScript(ArrayFromHex("0302ff03"))` is more readable (or performant) than `ScriptFromHex("0302ff03"`.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288502723)
> Are these just bugs with the implementation here, or an issue with CMake?
That's by CMake design.
> However, with CMake, it doesn't seem to figure out what order to do things in. i.e it just tries to run the tests, without compiling them:
>
> ```shell
> cmake -B build
> cmake --build build --target test
> Running tests...
> Test project /bitcoin/build
> Connected to MAKE jobserver
> Start 1: util_test_runner
> 1/133 Test #1: util_test_runner .....................*
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288502723)
> Are these just bugs with the implementation here, or an issue with CMake?
That's by CMake design.
> However, with CMake, it doesn't seem to figure out what order to do things in. i.e it just tries to run the tests, without compiling them:
>
> ```shell
> cmake -B build
> cmake --build build --target test
> Running tests...
> Test project /bitcoin/build
> Connected to MAKE jobserver
> Start 1: util_test_runner
> 1/133 Test #1: util_test_runner .....................*
...
💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288512730)
> Up to you, but there is a small benefit of going with the fuzz tests first (if possible)
It looks like the tests here mostly operate on the split out modules after the refactoring, as opposed to a implementation agnostic test through the p2p interface (which would be a giant pita to write). So splitting the fuzz test out would be fine as they don't directly assert that no behavior changes are made. It would of course be sad if we split them out and then only the refactoring lands (which has
...
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288512730)
> Up to you, but there is a small benefit of going with the fuzz tests first (if possible)
It looks like the tests here mostly operate on the split out modules after the refactoring, as opposed to a implementation agnostic test through the p2p interface (which would be a giant pita to write). So splitting the fuzz test out would be fine as they don't directly assert that no behavior changes are made. It would of course be sad if we split them out and then only the refactoring lands (which has
...
💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288526278)
> The test target is generated by CMake. Such targets [cannot](https://cmake.org/cmake/help/latest/command/add_dependencies.html) be subject to top-level target dependencies.
Ok. So what about `deploy`? If it's a custom target of ours, it should know to build `bitcoin-qt` first?
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288526278)
> The test target is generated by CMake. Such targets [cannot](https://cmake.org/cmake/help/latest/command/add_dependencies.html) be subject to top-level target dependencies.
Ok. So what about `deploy`? If it's a custom target of ours, it should know to build `bitcoin-qt` first?
🤔 marcofleon reviewed a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2237934699)
Tested ACK fa198d1a196482bf363ba57a3f4d6d2d860b8ce5
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2237934699)
Tested ACK fa198d1a196482bf363ba57a3f4d6d2d860b8ce5
🤔 stickies-v reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237840417)
Concept ACK for more compile time validation.
Strangely enough, 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 doesn't compile for me even though CI seems fine.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237840417)
Concept ACK for more compile time validation.
Strangely enough, 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 doesn't compile for me even though CI seems fine.
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716732991)
nit: while touching, might be worth updating to
```suggestion
assert(bytes.size() == m_keydata.size());
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716732991)
nit: while touching, might be worth updating to
```suggestion
assert(bytes.size() == m_keydata.size());
```
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716752262)
I'm unable to compile 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 using:
```
% clang --version
Homebrew clang version 17.0.6
Target: arm64-apple-darwin23.3.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
```
git clean -xdf && git checkout 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 && ./autogen.sh && ./configure --without-gui && make -j 7
./uint256.h:131:19: error: call to consteval function 'ConstevalHexDigit' is not a constant expression
auto lo = Const
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716752262)
I'm unable to compile 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 using:
```
% clang --version
Homebrew clang version 17.0.6
Target: arm64-apple-darwin23.3.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
```
git clean -xdf && git checkout 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 && ./autogen.sh && ./configure --without-gui && make -j 7
./uint256.h:131:19: error: call to consteval function 'ConstevalHexDigit' is not a constant expression
auto lo = Const
...
📝 hebasto opened a pull request: "guix: Drop unused module from manifest"
(https://github.com/bitcoin/bitcoin/pull/30653)
My Guix build:
```
x86_64
170df52c2238510bd166f3fb1c4c3c11d2c1480a2e468fd532cb4d0435ac11cf guix-build-c7fb80a08f98/output/aarch64-linux-gnu/SHA256SUMS.part
54e71ef5135464f58e3db4a3b893fa2f26a2c9cfb465699a363bb59a0d1bd94f guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu-debug.tar.gz
806d6042151e0af026748379b9bbbfea53d4c91555b2f0d05ed11faf83f429bb guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu.tar.gz
96f111f81
...
(https://github.com/bitcoin/bitcoin/pull/30653)
My Guix build:
```
x86_64
170df52c2238510bd166f3fb1c4c3c11d2c1480a2e468fd532cb4d0435ac11cf guix-build-c7fb80a08f98/output/aarch64-linux-gnu/SHA256SUMS.part
54e71ef5135464f58e3db4a3b893fa2f26a2c9cfb465699a363bb59a0d1bd94f guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu-debug.tar.gz
806d6042151e0af026748379b9bbbfea53d4c91555b2f0d05ed11faf83f429bb guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu.tar.gz
96f111f81
...