Bitcoin Core Github
45 subscribers
118K links
Download Telegram
💬 maflcko commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449726713)
Yeah, I am saying that the fix should be a complete one, at least when it comes to the `./src` directory. Otherwise it will have to be touched again. I am happy to submit a pull, or review one. But I think there have been enough issue and pull request threads about this topic and it would be good to wrap it up in at most one or two pulls.
dergoegge closed a pull request: "Revert "Introduce `g_fuzzing` global for fuzzing checks""
(https://github.com/bitcoin/bitcoin/pull/31189)
💬 dergoegge commented on pull request "Revert "Introduce `g_fuzzing` global for fuzzing checks"":
(https://github.com/bitcoin/bitcoin/pull/31189#issuecomment-2449729047)
> I am happy to submit a pull

Yes please
👋 fanquake's pull request is ready for review: "[27.x] rc2 or final"
(https://github.com/bitcoin/bitcoin/pull/31154)
💬 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#issuecomment-2449751980)
The CI failure in https://github.com/bitcoin/bitcoin/runs/32217592364 might come from a bad rebase reconciliation with master?
```
[12:44:30.723] Duplicate include(s) in src/bench/xor.cpp:
[12:44:30.723] #include <cstddef>
[12:44:30.723] #include <span.h>
[12:44:30.723] #include <streams.h>
[12:44:30.723] #include <random.h>
[12:44:30.723] #include <vector>
[12:44:30.723] #include <bench/bench.h>
[12:44:30.723]
[12:44:30.728] ^---- ⚠️ Failure generated from lint-includes.py
```
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824397333)
picked up in #31190
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824397572)
added in #31190
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824398039)
Agree the comment isn't 100% accurate, edited in #31190
💬 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