💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824400387)
since txdownloadman is locked with this mutex, and it's the only one with access to these data structures, we have the same locking happening. Since `m_tx_download_mutex` is external, we wouldn't be able to have these annotations I think

💬 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-2449786517)
> The CI failure in [bitcoin/bitcoin/runs/32217592364](https://github.com/bitcoin/bitcoin/runs/32217592364) might come from a bad rebase reconciliation with master?

That's an old build, right? Latest [Lint](https://github.com/bitcoin/bitcoin/pull/31144/checks?check_run_id=32217909313) seems fine

📝 maflcko opened a pull request: " Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz"
(https://github.com/bitcoin/bitcoin/pull/31191)
`g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See https://github.com/bitcoin/bitcoin/issues/31178

One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.

Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to
...

👍 dergoegge approved a pull request: "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz"
(https://github.com/bitcoin/bitcoin/pull/31191#pullrequestreview-2407916901)
utACK fafbf8acf419d5e2ca307e5804099361ca7471af

💬 dergoegge commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#discussion_r1824434342)
```suggestion
if constexpr (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING
```

💬 fanquake 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_r1824438854)
You can keep the std lib headers separated from our own headers.

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2449822242)
Thanks a lot @ryanofsky for the review. Applied almost all of your insightful changes, adding two new commits (adding you as coauthor).
You can view the diff with [`Compare`](https://github.com/bitcoin/bitcoin/compare/3b5b2457f713014fd5073da34b76a5a8cd796e4a..aa2f3139529c054b011a0f75ff314e6d63f0d977) or via `git range-diff 8e1381a..3b5b2457 a6921049..aa2f3139`

Added the following commits based on feedback:
* coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointe
...

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439691)
Done in previous step

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439724)
Done, 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_r1824439750)
Thank you for checking! Do you need any change here from me?

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439835)
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_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

💬 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.

💬 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`

💬 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.

💬 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

💬 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!

💬 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!

💬 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
...