👍 Sjors approved a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2240249200)
ACK cd913de6488d25057e3bf7dcad1508e9d689f87f
If you have to touch, can you make `contrib/devtools/headerssync-params.py` executable?
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2240249200)
ACK cd913de6488d25057e3bf7dcad1508e9d689f87f
If you have to touch, can you make `contrib/devtools/headerssync-params.py` executable?
💬 Sjors commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718320300)
985d1ec267f44b7897855fd2312da76b3fcd165b: this is already 13G for me.
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718320300)
985d1ec267f44b7897855fd2312da76b3fcd165b: this is already 13G for me.
💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291183910)
Can you explain this a bit more? I think optimizing the test framework to cleanly shut down on a failure is a bit premature, and may not be needed. However, if a failing test may leave behind a dangling bitcoind process, this seems like something to fix, because it will otherwise lead to test warnings and failures down the road.
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291183910)
Can you explain this a bit more? I think optimizing the test framework to cleanly shut down on a failure is a bit premature, and may not be needed. However, if a failing test may leave behind a dangling bitcoind process, this seems like something to fix, because it will otherwise lead to test warnings and failures down the road.
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2291189197)
> The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?
Good point! I'll address this, if I have to re-touch.
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2291189197)
> The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?
Good point! I'll address this, if I have to re-touch.
💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718344848)
Maybe a leftover previous `bitcoind` was used? Can you try `make clean`, just to double check?
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718344848)
Maybe a leftover previous `bitcoind` was used? Can you try `make clean`, just to double check?
🤔 ryanofsky reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239084398)
Code review 734ac5a9002493013c0f8afe763f751ac99f89c8
After experimenting with simplifying this. I don't think I like this approach anymore. I think adding the `ConstevalHexLiteral` class is basically going out of the way to make the code less efficient and more confusing, because instead of having a clear separation between runtime and compile time functions, we are duplicating code at runtime and compile time to provide a hybrid function that checks hex strings at compile time and then store
...
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239084398)
Code review 734ac5a9002493013c0f8afe763f751ac99f89c8
After experimenting with simplifying this. I don't think I like this approach anymore. I think adding the `ConstevalHexLiteral` class is basically going out of the way to make the code less efficient and more confusing, because instead of having a clear separation between runtime and compile time functions, we are duplicating code at runtime and compile time to provide a hybrid function that checks hex strings at compile time and then store
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717501165)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220
On second thought, while this warning message is more accurate, I don't think it is clear, because it is implying that return value will use stack space, when we should not expect that to be the case normally. Would suggest:
* It may be preferable to call VecFromHex instead of this function to save stack space, or to declare the returned constexpr to avoid using stack space, if returned array is large and being used t
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717501165)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220
On second thought, while this warning message is more accurate, I don't think it is clear, because it is implying that return value will use stack space, when we should not expect that to be the case normally. Would suggest:
* It may be preferable to call VecFromHex instead of this function to save stack space, or to declare the returned constexpr to avoid using stack space, if returned array is large and being used t
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717819227)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I can't see a benefit to defining a string_view constructor here and depending on string_view when this class is only supposed to accept null terminated string literals. This class could be simplified by dropping this constructor, dropping the string_view member and just declaring a const char* member.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717819227)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I can't see a benefit to defining a string_view constructor here and depending on string_view when this class is only supposed to accept null terminated string literals. This class could be simplified by dropping this constructor, dropping the string_view member and just declaring a const char* member.
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717821819)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
This will do a weird thing when passed a non-null terminated string and give an error about an out of bounds array access ("error: array subscript value ‘2’ is outside the bounds of array ‘hex’ of type ‘const char [2]’") for:
```c++
constexpr char hex[] = {'a', 'b'};
VecFromHex(hex);
```
This could be simplified and made less confusing by not requiring character arrays to be nul
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717821819)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
This will do a weird thing when passed a non-null terminated string and give an error about an out of bounds array access ("error: array subscript value ‘2’ is outside the bounds of array ‘hex’ of type ‘const char [2]’") for:
```c++
constexpr char hex[] = {'a', 'b'};
VecFromHex(hex);
```
This could be simplified and made less confusing by not requiring character arrays to be nul
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717827853)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex"
(b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I don't think it makes sense to have a class that check a hex string at compile time, store the literal hex string in the binary, and then parse the hex string again. It makes the implementation unnecessarily complicated because code loops over the hex string ConstevalHexLiteral 3 different places instead of 1, duplicates checks like `throw "2 hex digits required per byte"` twice, and
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717827853)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex"
(b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I don't think it makes sense to have a class that check a hex string at compile time, store the literal hex string in the binary, and then parse the hex string again. It makes the implementation unnecessarily complicated because code loops over the hex string ConstevalHexLiteral 3 different places instead of 1, duplicates checks like `throw "2 hex digits required per byte"` twice, and
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717828979)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex"
(b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I think it would be safer and more explicit to use `static_cast<Byte>(...)` instead of functional cast `Byte(...)`. Not actually sure about this particular situation, but in general there are unsafe conversions that C casts allow which static_cast does not allow.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717828979)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex"
(b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)
I think it would be safer and more explicit to use `static_cast<Byte>(...)` instead of functional cast `Byte(...)`. Not actually sure about this particular situation, but in general there are unsafe conversions that C casts allow which static_cast does not allow.
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717490226)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717352300
> In the PR summary you mean?
I do actually mean the commit message. The current commit message ddd06a0ec0a3b2c1d128fe5986a1363e7cf8e365 makes it sounds like it only switching to BOOST_CHECK_EQUAL_COLLECTIONS, but in reality is doing that and also making a seemingly unrelated test cleanup. Would suggest mentioning the other cleanup in the commit message, so for example, if someone is debugging something and wants to s
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717490226)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717352300
> In the PR summary you mean?
I do actually mean the commit message. The current commit message ddd06a0ec0a3b2c1d128fe5986a1363e7cf8e365 makes it sounds like it only switching to BOOST_CHECK_EQUAL_COLLECTIONS, but in reality is doing that and also making a seemingly unrelated test cleanup. Would suggest mentioning the other cleanup in the commit message, so for example, if someone is debugging something and wants to s
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717492122)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717426365
> Do we really need to support both?
It seems easier to support both than to add an error message explaining that the last byte has to be null, when there is no particular reason it needs to be null. But mainly I didn't think the last byte should be silently ignored, so at least that problem is fixed now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717492122)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717426365
> Do we really need to support both?
It seems easier to support both than to add an error message explaining that the last byte has to be null, when there is no particular reason it needs to be null. But mainly I didn't think the last byte should be silently ignored, so at least that problem is fixed now.
🤔 ismaelsadeeq reviewed a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240311191)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240311191)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e
💬 ismaelsadeeq commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718356981)
nit: should this change be in its own commit? then you can use scripted diff for the rename.
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718356981)
nit: should this change be in its own commit? then you can use scripted diff for the rename.
🤔 TheCharlatan reviewed a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240344135)
I still see a doubling in performance of the valid test if the validation cache is initialized with minimal values. Do you want to add it back?
current HEAD:
```
#10000 DONE cov: 2794 ft: 3029 corp: 66/642b lim: 33 exec/s: 161 rss: 406Mb
Done 10000 runs in 62 second(s)
```
+ min validation cache:
```
#10000 DONE cov: 2788 ft: 3011 corp: 54/431b lim: 68 exec/s: 333 rss: 350Mb
Done 10000 runs in 30 second(s)
```
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240344135)
I still see a doubling in performance of the valid test if the validation cache is initialized with minimal values. Do you want to add it back?
current HEAD:
```
#10000 DONE cov: 2794 ft: 3029 corp: 66/642b lim: 33 exec/s: 161 rss: 406Mb
Done 10000 runs in 62 second(s)
```
+ min validation cache:
```
#10000 DONE cov: 2788 ft: 3011 corp: 54/431b lim: 68 exec/s: 333 rss: 350Mb
Done 10000 runs in 30 second(s)
```
👍 ismaelsadeeq approved a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2240361497)
Tested ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2240361497)
Tested ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2291245181)
Closing until the migration to CMake is complete.
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2291245181)
Closing until the migration to CMake is complete.
✅ hebasto closed a pull request: "test: Do not write Python bytecode to source directory"
(https://github.com/bitcoin/bitcoin/pull/30533)
(https://github.com/bitcoin/bitcoin/pull/30533)
👍 ryanofsky approved a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240380401)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e. This is a much cleaner bugfix than I suggested in #29073. Moving the unload notification makes the shutdown sequence easier to understand, more efficient, and safe.
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240380401)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e. This is a much cleaner bugfix than I suggested in #29073. Moving the unload notification makes the shutdown sequence easier to understand, more efficient, and safe.