💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439862)
Yeah, but I don't like the format of that. If this fails, it's easy to debug.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439862)
Yeah, but I don't like the format of that. If this fails, it's easy to debug.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439886)
Hmm, so always check both? Ok, I don't mind, done
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439886)
Hmm, so always check both? Ok, I don't mind, done
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439939)
Good idea, did it in a separate commit, let's see what the CI and others think.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439939)
Good idea, did it in a separate commit, let's see what the CI and others think.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439987)
Was already removed in `coins: Remove direct GetFlags access`, but I've moved it up to `coins, refactor: Split up AddFlags to remove invalid states`
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439987)
Was already removed in `coins: Remove direct GetFlags access`, but I've moved it up to `coins, refactor: Split up AddFlags to remove invalid states`
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440072)
Or maybe even go one step further and make `SetDirty`/`SetFresh` static as well - thanks for the hint, pushed in a separate commit with your coauthorship.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824440072)
Or maybe even go one step further and make `SetDirty`/`SetFresh` static as well - thanks for the hint, pushed in a separate commit with your coauthorship.
💬 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)