💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716582838)
Ah, that looks like a bad rebase - deleted the comment now, thanks
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716582838)
Ah, that looks like a bad rebase - deleted the comment now, thanks
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644034)
Right, I don't think it's problematic to not log tx invs during IBD since they are ignored anyway.
I've moved the logging change to its own commit, to keep the pure refactoring-ness of this commit :+1:
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644034)
Right, I don't think it's problematic to not log tx invs during IBD since they are ignored anyway.
I've moved the logging change to its own commit, to keep the pure refactoring-ness of this commit :+1:
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644076)
Ah, good observation. I've now gated the `AlreadyHaveTx` check inside of `AddTxAnnouncement` on `p2p_inv` so this is no longer happening.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644076)
Ah, good observation. I've now gated the `AlreadyHaveTx` check inside of `AddTxAnnouncement` on `p2p_inv` so this is no longer happening.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716591125)
done :+1:
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716591125)
done :+1:
👍 theStack approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237733485)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237733485)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2288387364)
> I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?
All changes to `depends/funcs.mk` from this PR must be ported to `hebasto/cmake-staging`.
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2288387364)
> I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?
All changes to `depends/funcs.mk` from this PR must be ported to `hebasto/cmake-staging`.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288403119)
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?
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288403119)
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?
👍 danielabrozzoni approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237782147)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237782147)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237724166)
Left some style nits / questions, feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237724166)
Left some style nits / questions, feel free to ignore.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716660247)
style nit in 29090eca423327353b474615d02ed7c3190e4a50: Can drop the trailing `\`, since this is a normal function and not a macro.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716660247)
style nit in 29090eca423327353b474615d02ed7c3190e4a50: Can drop the trailing `\`, since this is a normal function and not a macro.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716665464)
nit in 9dab917088e83f627786ed5caafec859a3481b78: If you change the behavior, it also needs to adjust the error string. Now it should say: `throw "Hex string must only contain lowercase hex chars";`.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716665464)
nit in 9dab917088e83f627786ed5caafec859a3481b78: If you change the behavior, it also needs to adjust the error string. Now it should say: `throw "Hex string must only contain lowercase hex chars";`.
💬 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.