💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440193)
added refactor to the first two commits, thanks
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440193)
added refactor to the first two commits, thanks
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440235)
I was hesitant at first (mostly because this will be removed in `test: Validate error messages on fail`), but we want to always fail when these do, so you're right, thanks!
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440235)
I was hesitant at first (mostly because this will be removed in `test: Validate error messages on fail`), but we want to always fail when these do, so you're right, thanks!
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440291)
Good idea, done!
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440291)
Good idea, done!
💬 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-2449824362)
Taking a step back, I wonder if this is worth it. IIRC it gives a +1% speedup when run on spinning storage, so it seems a higher speedup is possibly visible on faster, modern storage. However, it would be good if any IO delay was taken out of IBD completely, so that the speed of storage and the speed of XOR is largely irrelevant.
I haven't looked, but this may be possible by asking the next block the be read into memory in the background, as soon as work on the current block begins.
Someth
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449824362)
Taking a step back, I wonder if this is worth it. IIRC it gives a +1% speedup when run on spinning storage, so it seems a higher speedup is possibly visible on faster, modern storage. However, it would be good if any IO delay was taken out of IBD completely, so that the speed of storage and the speed of XOR is largely irrelevant.
I haven't looked, but this may be possible by asking the next block the be read into memory in the background, as soon as work on the current block begins.
Someth
...
📝 hebasto opened a pull request: "depends, doc: List packages required to build `qt` package separately"
(https://github.com/bitcoin/bitcoin/pull/31192)
Suggested in https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791789613:
> There's probably enough GUI-only stuff here, i.e `bison`, `ninja-build`, `python3`, `xz-utils`, that this could be moved to it's own `#### Gui` section.
(https://github.com/bitcoin/bitcoin/pull/31192)
Suggested in https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791789613:
> There's probably enough GUI-only stuff here, i.e `bison`, `ninja-build`, `python3`, `xz-utils`, that this could be moved to it's own `#### Gui` section.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824445697)
See https://github.com/bitcoin/bitcoin/pull/31192.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824445697)
See https://github.com/bitcoin/bitcoin/pull/31192.
💬 fanquake commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824448241)
pkg-config can only be moved here after #31181?
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824448241)
pkg-config can only be moved here after #31181?
💬 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-2449836182)
> Taking a step back, I wonder if this is worth it. IIRC it gives a +1% speedup
That's not the main point, rather that we're storing the key in a value that can be short-circuited easily so that when key is 0 (i.e. xor is NOOP), we can skip it. Previously this would have only been possible by checking each byte of the key.
It's also a lot cleaner to store it in a primitive instead, which supports xor natively.
Xor comes up in every profiling I do, we shouldn't have a regression because of
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449836182)
> Taking a step back, I wonder if this is worth it. IIRC it gives a +1% speedup
That's not the main point, rather that we're storing the key in a value that can be short-circuited easily so that when key is 0 (i.e. xor is NOOP), we can skip it. Previously this would have only been possible by checking each byte of the key.
It's also a lot cleaner to store it in a primitive instead, which supports xor natively.
Xor comes up in every profiling I do, we shouldn't have a regression because of
...
💬 hebasto commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824451082)
For `make -C depends MULTIPROCESS=1 NO_QT=1` the `pkg-config` seems not required though.
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1824451082)
For `make -C depends MULTIPROCESS=1 NO_QT=1` the `pkg-config` seems not required though.
💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449853264)
Concept ACK.
As @fanquake mentioned above, we currently have:
```cmake
set_target_properties(bitcoin-chainstate PROPERTIES
SKIP_BUILD_RPATH OFF
)
```
Which I think can be removed now, no?
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449853264)
Concept ACK.
As @fanquake mentioned above, we currently have:
```cmake
set_target_properties(bitcoin-chainstate PROPERTIES
SKIP_BUILD_RPATH OFF
)
```
Which I think can be removed now, no?
💬 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
...