💬 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#issuecomment-2449896533)
> change a "fix" is not entirely accurate
when xor is disabled we're not xor-ing now at all. Previously we did xor, so this change brings back the previous behavior when xor is disabled.
> The pull request title starts with "optimization", so I got the impression that a speedup is the main point.
Yes, please see the updated description with the benchmarks: https://github.com/bitcoin/bitcoin/pull/31144#issue-2610689777
> may not be visible at all if IO is completely taken out of the c
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449896533)
> change a "fix" is not entirely accurate
when xor is disabled we're not xor-ing now at all. Previously we did xor, so this change brings back the previous behavior when xor is disabled.
> The pull request title starts with "optimization", so I got the impression that a speedup is the main point.
Yes, please see the updated description with the benchmarks: https://github.com/bitcoin/bitcoin/pull/31144#issue-2610689777
> may not be visible at all if IO is completely taken out of the c
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824510925)
How about this description:
```
Avoid defining QT_NEEDS_QMAIN macro
Qt's QT_NEEDS_QMAIN macro renames our main() function to qMain(),
which breaks our assumptions regarding exported symbols. In particular,
the CONTROL_FLOW security check fails for Windows builds in Guix.
The QT_NEEDS_QMAIN macro is required for linking to the Qt DLLs only,
so we can safely disable it.
```
?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824510925)
How about this description:
```
Avoid defining QT_NEEDS_QMAIN macro
Qt's QT_NEEDS_QMAIN macro renames our main() function to qMain(),
which breaks our assumptions regarding exported symbols. In particular,
the CONTROL_FLOW security check fails for Windows builds in Guix.
The QT_NEEDS_QMAIN macro is required for linking to the Qt DLLs only,
so we can safely disable it.
```
?
💬 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_r1824518058)
[Done](https://github.com/bitcoin/bitcoin/compare/8696184c2fd183483fa843348323ffbc62c2de3c..33625e9d5327263c273583d7b3fe2f3114ebf12e), anything else?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824518058)
[Done](https://github.com/bitcoin/bitcoin/compare/8696184c2fd183483fa843348323ffbc62c2de3c..33625e9d5327263c273583d7b3fe2f3114ebf12e), anything else?
💬 ryanofsky commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449909613)
I think it is worth looking for a better way to fix this because of the clumsiness of needing to compile two different versions of libraries when fuzzing is used.
It is not true that in general that `Assume` checks were "optimized out" before #31093 because the checks are performed before the `inline_assertion_check` function is called, so wouldn't be skipped in general just because the function is implemented differently. The compiler would be able to skip the checks if it knows they don't h
...
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449909613)
I think it is worth looking for a better way to fix this because of the clumsiness of needing to compile two different versions of libraries when fuzzing is used.
It is not true that in general that `Assume` checks were "optimized out" before #31093 because the checks are performed before the `inline_assertion_check` function is called, so wouldn't be skipped in general just because the function is implemented differently. The compiler would be able to skip the checks if it knows they don't h
...
💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449990640)
> It is not true that in general that `Assume` checks were "optimized out" before #31093 because the checks are performed before the `inline_assertion_check` function is called, so wouldn't be skipped in general just because the function is implemented differently.
I may be missing something, but I don't think this is true. The condition `!val` is never evaluated (and the compiler doesn't has to account for it being evaluated and doesn't have to emit any statements for it) in the following co
...
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449990640)
> It is not true that in general that `Assume` checks were "optimized out" before #31093 because the checks are performed before the `inline_assertion_check` function is called, so wouldn't be skipped in general just because the function is implemented differently.
I may be missing something, but I don't think this is true. The condition `!val` is never evaluated (and the compiler doesn't has to account for it being evaluated and doesn't have to emit any statements for it) in the following co
...
💬 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
...
(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.
(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.
(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.
(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()) {
```
(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));
```
(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.
(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?
(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?
(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.
(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.
(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}}`
(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.
(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)
(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.
(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.