💬 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.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719045686)
I made some edits here, making sure to clearly spell out what you suggested to include.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719045686)
I made some edits here, making sure to clearly spell out what you suggested to include.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2292338592)
Addressed feedback and rebased.
There is a bit of a hack in the tests now that is made necessary by an unrelated bug: #26245 I marked this with an TODO accordingly.
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2292338592)
Addressed feedback and rebased.
There is a bit of a hack in the tests now that is made necessary by an unrelated bug: #26245 I marked this with an TODO accordingly.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131)
Thanks for taking the time @ryanofsky!
Your troubles with runtime vs compile time memory is indeed part of what fed into prior solutions. Cool that `VectorFromHex<"1234">()` works, but I'm happy you're not a fan either.
While I did like the symmetry of `ArrayFromHex` with the recently added `Txid::FromHex` and `uint256::FromHex`, the latter ones like to reverse the byte order though, and `HexLiteral` is 2 chars less. `HexLiteral` feels more like prime real estate, so hiding behind the fig-
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131)
Thanks for taking the time @ryanofsky!
Your troubles with runtime vs compile time memory is indeed part of what fed into prior solutions. Cool that `VectorFromHex<"1234">()` works, but I'm happy you're not a fan either.
While I did like the symmetry of `ArrayFromHex` with the recently added `Txid::FromHex` and `uint256::FromHex`, the latter ones like to reverse the byte order though, and `HexLiteral` is 2 chars less. `HexLiteral` feels more like prime real estate, so hiding behind the fig-
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719074063)
Thank you for being persistent! Must have had other local changes interacting badly with it when I tried it. Got it working with only your diff, so if we end up resurrecting `ConstevalHexLiteral` I'll go with your version.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719074063)
Thank you for being persistent! Must have had other local changes interacting badly with it when I tried it. Got it working with only your diff, so if we end up resurrecting `ConstevalHexLiteral` I'll go with your version.