💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718893373)
Nit: The includes need to be corrected again.
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718893373)
Nit: The includes need to be corrected again.
💬 mzumsande commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718921534)
I see similar size to @Sjors, as of height=2873716:
```
du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
13G /home/martin/.bitcoin/testnet3/chainstate/
82G /home/martin/.bitcoin/testnet3/blocks/
```
(I wasn't continuously online during the fork shenanigans, so having fewer blocks makes sense).
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718921534)
I see similar size to @Sjors, as of height=2873716:
```
du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
13G /home/martin/.bitcoin/testnet3/chainstate/
82G /home/martin/.bitcoin/testnet3/blocks/
```
(I wasn't continuously online during the fork shenanigans, so having fewer blocks makes sense).
📝 hebasto opened a pull request: "CMake replaces Autotools"
(https://github.com/bitcoin/bitcoin/pull/30664)
This PR is based on https://github.com/bitcoin/bitcoin/pull/30454 with additional commits that delete both the Autotools-based build system and the legacy MSVC build system.
It aims reveal conflicts with other PRs.
For more details, please refer to today's IRC meeting discussion.
(https://github.com/bitcoin/bitcoin/pull/30664)
This PR is based on https://github.com/bitcoin/bitcoin/pull/30454 with additional commits that delete both the Autotools-based build system and the legacy MSVC build system.
It aims reveal conflicts with other PRs.
For more details, please refer to today's IRC meeting discussion.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993291)
fixed
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993291)
fixed
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993817)
fixed (as part of one of the larger changes)
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993817)
fixed (as part of one of the larger changes)
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718995086)
Adopted the type approach you suggested so this is resolved as well with that.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718995086)
Adopted the type approach you suggested so this is resolved as well with that.
💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718994222)
> nit in [bb67ca0](https://github.com/bitcoin/bitcoin/commit/bb67ca09c88a46b99bf83e937fa761f0e25cba37): Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
Makes sense. I hadn't really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:
```c++
SeedRandomForTest(SeedRand seed = SeedRand:
...
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718994222)
> nit in [bb67ca0](https://github.com/bitcoin/bitcoin/commit/bb67ca09c88a46b99bf83e937fa761f0e25cba37): Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.
Makes sense. I hadn't really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:
```c++
SeedRandomForTest(SeedRand seed = SeedRand:
...
👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241332588)
Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241332588)
Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718998488)
Thanks, I agree that the default wasn't very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it's also easier to hand because you don't have to deal with a hash or height in the most common use cases.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718998488)
Thanks, I agree that the default wasn't very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it's also easier to hand because you don't have to deal with a hash or height in the most common use cases.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719001528)
I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it's easier to review this way. I added some minor documentation changes since `CreateUTXOSnaphot()` is now only used in tests.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719001528)
I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it's easier to review this way. I added some minor documentation changes since `CreateUTXOSnaphot()` is now only used in tests.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155)
As mentioned in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847, who would do that?
Someone could theoretically do
```C++
constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
VecFromHex(hex);
```
and have your version fail to compile.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155)
As mentioned in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847, who would do that?
Someone could theoretically do
```C++
constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
VecFromHex(hex);
```
and have your version fail to compile.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718998801)
I see what you mean and tried it out, but ended up preferring input validation at the top of the function.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718998801)
I see what you mean and tried it out, but ended up preferring input validation at the top of the function.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718978767)
Thank you!
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718978767)
Thank you!
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718976845)
I got lost around `SerializeMany` :), but agree, **serialize.h** does the `std::array` -> `Span` conversion internally.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718976845)
I got lost around `SerializeMany` :), but agree, **serialize.h** does the `std::array` -> `Span` conversion internally.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718996633)
Thanks for finding this! Remains from battling the compiler.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718996633)
Thanks for finding this! Remains from battling the compiler.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718966018)
Yes, this change isn't to improve benchmark performance.
IMO the changed version is cleaner - the number of bytes to be converted is explicitly enforced in the type and can be connected to the calculation below.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718966018)
Yes, this change isn't to improve benchmark performance.
IMO the changed version is cleaner - the number of bytes to be converted is explicitly enforced in the type and can be connected to the calculation below.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718993637)
The intention was that the `string_view` length which is used at runtime would have a higher chance of being calculated at compile time.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718993637)
The intention was that the `string_view` length which is used at runtime would have a higher chance of being calculated at compile time.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719000295)
See https://github.com/bitcoin/bitcoin/pull/30377/files#r1716903532
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719000295)
See https://github.com/bitcoin/bitcoin/pull/30377/files#r1716903532
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719001947)
Good suggestion, seems to work out well.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719001947)
Good suggestion, seems to work out well.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718972546)
It's because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.
If we go the `Vec(HexLiteral` route at least it becomes shorter.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718972546)
It's because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.
If we go the `Vec(HexLiteral` route at least it becomes shorter.