💬 darosior commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449860078)
Thanks Maflcko, i think it's a good short term solution to fix what i just broke. I'll open a PR shortly.
That said, it does seem unknown entries in `settings.json` should be ignored by the GUI as it's a file the user isn't supposed to edit? Right now it's assuming we will never remove a startup option, which isn't very robust.
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449860078)
Thanks Maflcko, i think it's a good short term solution to fix what i just broke. I'll open a PR shortly.
That said, it does seem unknown entries in `settings.json` should be ignored by the GUI as it's a file the user isn't supposed to edit? Right now it's assuming we will never remove a startup option, which isn't very robust.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824475492)
> Could you better explain this patch?
Sure.
> The description says that using the `QT_NEEDS_QMAIN` macro is required...
... "for linking to the Qt DLL on Windows", which is not our use case.
> ... but using it breaks things, so you're just getting rid of it? So how do things continue to work, if you're removing the required macro?
Using static libraries for Windows does not require all Windows-specific stuff with export/import declarations, entry points etc.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824475492)
> Could you better explain this patch?
Sure.
> The description says that using the `QT_NEEDS_QMAIN` macro is required...
... "for linking to the Qt DLL on Windows", which is not our use case.
> ... but using it breaks things, so you're just getting rid of it? So how do things continue to work, if you're removing the required macro?
Using static libraries for Windows does not require all Windows-specific stuff with export/import declarations, entry points etc.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824479383)
It'd be good to add all the relevant information and context to the patch. Can you also explain "breaks the CONTROL_FLOW security check during Guix build for Windows."
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824479383)
It'd be good to add all the relevant information and context to the patch. Can you also explain "breaks the CONTROL_FLOW security check during Guix build for Windows."
💬 maflcko 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-2449867757)
> we shouldn't have a regression because of #28207 - this PR solves that.
It is not possible to do XOR without any cost at all. There will always be an overhead and I think calling #28207 a regression and this change a "fix" is not entirely accurate. This change reduces the overhead, according to the benchmarks.
> That's not the main point
The pull request title starts with "optimization", so I got the impression that a speedup is the main point.
The reason that `std::vector` was pic
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449867757)
> we shouldn't have a regression because of #28207 - this PR solves that.
It is not possible to do XOR without any cost at all. There will always be an overhead and I think calling #28207 a regression and this change a "fix" is not entirely accurate. This change reduces the overhead, according to the benchmarks.
> That's not the main point
The pull request title starts with "optimization", so I got the impression that a speedup is the main point.
The reason that `std::vector` was pic
...
🚀 fanquake merged a pull request: "[27.x] rc2 or final"
(https://github.com/bitcoin/bitcoin/pull/31154)
(https://github.com/bitcoin/bitcoin/pull/31154)
💬 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.