💬 marcofleon commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#discussion_r1824566276)
Do we need the `std::is_constant_evaluated()` then? I'm getting this warning everywhere when I build
```
../../../../src/util/check.h:53:32: warning: 'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression [-Wconstant-evaluated]
53 | if constexpr (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING
| ^
../../../../src/util/check.h:53:32: warning: 'std::is_constant_evaluated' will always evaluate
...

💬 ryanofsky commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450066348)
I could also be missing something, but doesn't `Assume(<check expression>)` just expand to `inline_assertion_check<false>(<check expression>, __FILE__, __LINE__, __func__, "<check expression>")` so unless the compiler knows `<check expression>` does not have side effects, must execute it and can't optimize it out? If `Assume` macro were implemented differently it could skip evaluating `<check expression>` at all.

💬 fanquake commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824611469)
It would be more of a convinience. I guess this doesn't make too much difference as 31181 is likely to be merged soon in any case.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824611784)
This seems more clear.

💬 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_r1824429398)
While `key_exists` is only created for this ìf`-block, there are other conditions involved, and we test for the negation of the value, so I find it less surprising to revert to the previous approach.
```suggestion
bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key_vector);
if (!key_exists && params.obfuscate && IsEmpty()) {
```

💬 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_r1824438833)
I find it more useful to (static) assert that the `std::array` size matches the uint64 size directly. Also don't see a point in zeroing out the local variable before returning?
```suggestion
uint64_t key;
static_assert(xor_key.size() == sizeof(key));
std::memcpy(&key, xor_key.data(), sizeof(key));
```

💬 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_r1824457332)
In 850214ffd9f56e887a18d0428d5881e6c1ee8652:
Single/Multi character key comments don't make sense inside of this commit.

💬 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_r1824551920)
In 4437d4379c42a7b87bd01ad5ea6c450a732f4f95:
Don't see the point of this diff, must be in preparation of another commit?

💬 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_r1824553664)
Guess the point of adding this assert is to prove that it doesn't break anything before switching to uint64?

💬 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_r1824605285)
Should not use `m_`-prefix for non-member variables.

💬 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