💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866)
style nit in 6a256b96af8c94804538eb9e78964557c032b74f: I think in tests it is more important that they are short, easy to read, and write. Compile time enforcements are neat at best, but shouldn't be a goal. Generally, the unit tests are single threaded and fully deterministic, so if the test ran once (and passed), it should always pass.
So my preference would be to just call `data = *Assert(TryParseHex(str))` inside `ScriptFromHex`.
This makes the diff smaller and also ensures that the sa
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866)
style nit in 6a256b96af8c94804538eb9e78964557c032b74f: I think in tests it is more important that they are short, easy to read, and write. Compile time enforcements are neat at best, but shouldn't be a goal. Generally, the unit tests are single threaded and fully deterministic, so if the test ran once (and passed), it should always pass.
So my preference would be to just call `data = *Assert(TryParseHex(str))` inside `ScriptFromHex`.
This makes the diff smaller and also ensures that the sa
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716703571)
in 29090eca423327353b474615d02ed7c3190e4a50: When touching all of those lines, I wonder if it makes sense to switch them to `std::byte`, but that will probably be more involved.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716703571)
in 29090eca423327353b474615d02ed7c3190e4a50: When touching all of those lines, I wonder if it makes sense to switch them to `std::byte`, but that will probably be more involved.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716693711)
ToByteVector seems to be called in a few places, though mostly in tests.
I wonder if it makes sense to just go with https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716693711)
ToByteVector seems to be called in a few places, though mostly in tests.
I wonder if it makes sense to just go with https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110
💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716712062)
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.
It's a nit, and the commit is orthogonal to the PR anyway, so this is the last I'll comment on it - up to you what you prefer, but I feel like this would probably be the most elegant implementation:
```cpp
std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
{
sta
...
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716712062)
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.
It's a nit, and the commit is orthogonal to the PR anyway, so this is the last I'll comment on it - up to you what you prefer, but I feel like this would probably be the most elegant implementation:
```cpp
std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
{
sta
...
💬 maflcko commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288433763)
> Would it help if I put the test/fuzz changes in a separate PR from the refactoring changes? It would make the first PR less exciting to review, but perhaps easier to review?
Up to you, but there is a small benefit of going with the fuzz tests first (if possible), in that they first run on master (and pass), and then on the refactor changes (and pass as well). For unit tests this can be achieved by just putting them in an early commit and then running the tests for all commits. For fuzz test
...
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288433763)
> Would it help if I put the test/fuzz changes in a separate PR from the refactoring changes? It would make the first PR less exciting to review, but perhaps easier to review?
Up to you, but there is a small benefit of going with the fuzz tests first (if possible), in that they first run on master (and pass), and then on the refactor changes (and pass as well). For unit tests this can be achieved by just putting them in an early commit and then running the tests for all commits. For fuzz test
...
💬 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 .....................*
...