💬 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.
(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)
(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
(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)
(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
```
(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 opened a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190)
Addresses some remaining followups from #30110:
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819638959
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819634235
(https://github.com/bitcoin/bitcoin/pull/31190)
Addresses some remaining followups from #30110:
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819638959
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819634235
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1824397333)
picked up in #31190
(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
(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
(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
(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
(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
...
(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
(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
```
(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.
(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
...
(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
(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
(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?
(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
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1824439835)
Done