💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288437679)
Is CMake meant to know about/be able to figure build-time dependencies?
For example, on master, if I `./configure`, and then call `make check`, `make` will compile and then run the tests; or, if I `./configure` for macOS, and call `make deploy`, `make` will build `bitcoin-qt`, and then pacakge it.
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:
```bash
cmake -B build
cmake --build build --target t
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288437679)
Is CMake meant to know about/be able to figure build-time dependencies?
For example, on master, if I `./configure`, and then call `make check`, `make` will compile and then run the tests; or, if I `./configure` for macOS, and call `make deploy`, `make` will build `bitcoin-qt`, and then pacakge it.
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:
```bash
cmake -B build
cmake --build build --target t
...
💬 0xawaz commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288449809)
Great, let me review the discussions again to clear my confusion and find answers to my questions. Lazy me, I was hoping for a highlight of major requirements that weren't met before, I guess I should do that highlight myself 😄
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288449809)
Great, let me review the discussions again to clear my confusion and find answers to my questions. Lazy me, I was hoping for a highlight of major requirements that weren't met before, I guess I should do that highlight myself 😄
💬 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
...