💬 hebasto commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2288250542)
cc @furszy
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2288250542)
cc @furszy
💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716596528)
Oh sorry, missed that. It's fine.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716596528)
Oh sorry, missed that. It's fine.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288318410)
Force pushed for :rocket: lazy re-init, which gives:
* 100x iterations/s in the lazy case
* 1.35x iterations/s in the expensive case
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288318410)
Force pushed for :rocket: lazy re-init, which gives:
* 100x iterations/s in the lazy case
* 1.35x iterations/s in the expensive case
🤔 glozow reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2237602476)
Thanks @theStack!
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2237602476)
Thanks @theStack!
💬 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
...