💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824444165)
Don't see much point in adding the assert here in 23fc898514bf9696facbaff65251b62c362d214e were we still only have a fixed-size `std::array` with the asserted size of 8. Seems sufficient with the assert in the `BlockManager`-ctor.

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824574701)
In 4437d4379c42a7b87bd01ad5ea6c450a732f4f95:
Less verbose:
`{std::byte{0x00}, std::byte{0x00}, std::byte{0x00}, std::byte{0x00}, std::byte{0x00}, std::byte{0x00}, std::byte{0x00}, std::byte{0x00}}`
=>
`{8, std::byte{0x00}}`

💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#discussion_r1824619412)
Yeah, the two are exclusive. It is not possible to add `constexpr` here and use `std::is_constant_evaluated()` correctly. The meaning of `std::is_constant_evaluated()` in this context denotes whether the `Assume()` was called in a constexpr context, not whether the if condition was evaluated constant.

💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#discussion_r1824620444)
(resolving for now)

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824625015)
This pattern has been [used before](https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L64), I meant to narrow the scope of the var here.
If you feel strongly about it, I'll separate them.

💬 sipa commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450100260)
@ryanofsky Yeah, the context here is exactly `Assume()` calls where the conditions is easily shown to be side-effect free by the compiler. I have been using those fairly liberally (inside production code), because it means free extra checks in fuzzing tests that have no runtime cost, but are simultaneously definitely not going to result in different behavior between fuzz tests and production code (because it's the compiler deciding the condition is side-effect free).

I think these are very us
...

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824627942)
I've added the fills to make sure we're not using them after conversion anymore.
What would be the advantage of the static asserts?
I don't mind removing these failsafes if you think they're redundant or noisy.

💬 polespinasa commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2450106148)
I'm not able to run fuzzing tests also because of this:
```
$ cmake --build build_fuzz
....
/bitcoin/src/uint256.h:133:19: error: call to consteval function 'util::ConstevalHexDigit' is not a constant expression
auto lo = util::ConstevalHexDigit(*(str_it++));
```
Which clang version are we suposed to use?
```
$ clang --version
Ubuntu clang version 14.0.0-1ubuntu1.1
```

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824630954)
yes

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824631355)
it's removed in the next commit

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2450109402)
> Which clang version are we suposed to use?

https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824645305)
> This pattern has been [used before](https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L64)

In the example you give, the variable is used in a more "positive" sense. Here we test the negated value, which is part of what makes it jarring.

One could maybe improve readability by moving the negation and renaming:
```suggestion
if (bool unknown_key = !Read(OBFUSCATE_KEY_KEY, obfuscate_key_vector); unknown_key && params.obfuscate && IsEmpty()) {
```

> I meant to narrow the
...

💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450130161)
> so unless the compiler knows `<check expression>` does not have side effects, must execute it and can't optimize it out?

Correct, but the that hasn't been changed by `g_fuzzing` at all. It is not the execution of `<check expression>`, but the execution of the negation of it, and the corresponding expensive branch in hot paths which has to be emitted for the unlikely case that the negation evaluates to true.

💬 fanquake commented on pull request "depends: For mingw cross compile use -gcc-posix to prevent library conflict":
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2450135287)
Backporting to 28.x in #31104 for consistency.

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824651933)
You're right, I've used `xor_key` in the same case before

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824652007)
But done anyway

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824652156)
Removed, thanks

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824652216)
Simplified, thanks

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824652442)
Done, thanks

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824655597)
Removed in the end, not that important