💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724777213)
nit: No longer needed:
```diff
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -7,9 +7,6 @@
#include <consensus/amount.h>
#include <random.h>
-#include <uint256.h>
-
-#include <cstdint>
enum class SeedRand {
ZEROS, //!< Seed with a compile time constant of zeros
```
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724777213)
nit: No longer needed:
```diff
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -7,9 +7,6 @@
#include <consensus/amount.h>
#include <random.h>
-#include <uint256.h>
-
-#include <cstdint>
enum class SeedRand {
ZEROS, //!< Seed with a compile time constant of zeros
```
💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724661382)
nit: No longer requires **test/util/random.h** and doesn't get it indirectly through **setup_common.h** since this is a benchmark:
```diff
-#include <test/util/random.h>
+#include <random.h>
```
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724661382)
nit: No longer requires **test/util/random.h** and doesn't get it indirectly through **setup_common.h** since this is a benchmark:
```diff
-#include <test/util/random.h>
+#include <random.h>
```
📝 hebasto converted_to_draft a pull request: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).
The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.
However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).
The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.
However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724794173)
> I understand this was the behavior in the past too, but should we do something else with `m_rng` if the function was called with `SeedRand::ZEROS`?
In theory one could reseed with a constant, but I don't see the benefits, given that it:
* Requires more code to handle both cases
* Is not a refactor
So maybe this can be done in a follow-up if relevant?
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724794173)
> I understand this was the behavior in the past too, but should we do something else with `m_rng` if the function was called with `SeedRand::ZEROS`?
In theory one could reseed with a constant, but I don't see the benefits, given that it:
* Requires more code to handle both cases
* Is not a refactor
So maybe this can be done in a follow-up if relevant?
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724800311)
> Data-members should be private by default:
Not sure in tests, because if an object of `TxOrphanageTest` exists, it can be possibly useful to have access to the objects.m_rng for mocking. Whereas there are very little downsides (apart from being possibly confusing to use the objects.m_rng instead of the "global" m_rng).
I'll leave this as-is for now, but I may change it later, because either way is fine and I don't think it matters much.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724800311)
> Data-members should be private by default:
Not sure in tests, because if an object of `TxOrphanageTest` exists, it can be possibly useful to have access to the objects.m_rng for mocking. Whereas there are very little downsides (apart from being possibly confusing to use the objects.m_rng instead of the "global" m_rng).
I'll leave this as-is for now, but I may change it later, because either way is fine and I don't think it matters much.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2301683678)
Thanks for checking the includes in the tests @hodlinator! I think I'll leave this as-is for now, because having leftover unused includes in tests should be harmless, apart from possibly being confusing, or needing more compile time. I have a follow-up to use `iwyu` (and enforce it), so that reviewers can use the re-review cycles to clean up includes for reviewing the code itself. However, this will be a follow-up, so I'll leave this pull request as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2301683678)
Thanks for checking the includes in the tests @hodlinator! I think I'll leave this as-is for now, because having leftover unused includes in tests should be harmless, apart from possibly being confusing, or needing more compile time. I have a follow-up to use `iwyu` (and enforce it), so that reviewers can use the re-review cycles to clean up includes for reviewing the code itself. However, this will be a follow-up, so I'll leave this pull request as-is for now.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724810488)
Good catch. I'll add a commit to this pull request, or create a follow-up, whichever happens earlier.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724810488)
Good catch. I'll add a commit to this pull request, or create a follow-up, whichever happens earlier.
💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301690194)
> > will work.
>
> What do you mean by "work" here?
By "work" I mean producing x86_64 binaries with the `IBT` and `SHSTK` markers in the `.note.gnu.property` section.
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301690194)
> > will work.
>
> What do you mean by "work" here?
By "work" I mean producing x86_64 binaries with the `IBT` and `SHSTK` markers in the `.note.gnu.property` section.
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2301700130)
> The media seems fine, it's just a 2.5" SSD, but I can try syncing to a different drive just in case the issue is with the media.
There are many known reasons for database corruptions, such as Apple or Windows filesystems that leveldb can't handle, or known broken hardware (shipped broken by the manufacturer), or otherwise broken hardware (overheating, etc...). So my recommendation would be to create a new issue for each new instance. Otherwise, it is hard to keep track of a single instance
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2301700130)
> The media seems fine, it's just a 2.5" SSD, but I can try syncing to a different drive just in case the issue is with the media.
There are many known reasons for database corruptions, such as Apple or Windows filesystems that leveldb can't handle, or known broken hardware (shipped broken by the manufacturer), or otherwise broken hardware (overheating, etc...). So my recommendation would be to create a new issue for each new instance. Otherwise, it is hard to keep track of a single instance
...
💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724840998)
Defaulting to private means that should something outside the class start needing access to it, that PR would need to expose it, thereby highlighting to reviewers what is actually happening.
I think it's a good principle even if this is test code, it is new in this PR after all. Maybe if you re-touch?
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724840998)
Defaulting to private means that should something outside the class start needing access to it, that PR would need to expose it, thereby highlighting to reviewers what is actually happening.
I think it's a good principle even if this is test code, it is new in this PR after all. Maybe if you re-touch?
💬 hebasto commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301763396)
> Could rebase the upstream pull, which currently fails CI, to pick up the upstream CI fixes?
Done.
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301763396)
> Could rebase the upstream pull, which currently fails CI, to pick up the upstream CI fixes?
Done.
💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2301792936)
> Are you referring to MAX_ADDR_TO_SEND? If yes, it could make sense to clarify this in the description or even in a code comment, given that the variable name seems to indicate it is only used for sending, not for processing/adding.
Yes, the name is a little bit confusing since it's the maximum number of addresses permitted in an ADDR message. So we use it for sending and receiving.
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2301792936)
> Are you referring to MAX_ADDR_TO_SEND? If yes, it could make sense to clarify this in the description or even in a code comment, given that the variable name seems to indicate it is only used for sending, not for processing/adding.
Yes, the name is a little bit confusing since it's the maximum number of addresses permitted in an ADDR message. So we use it for sending and receiving.
💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301815750)
> > Shouldn't binutils ... also be CET-enabled?
>
> What is a CET-enbled binutils?
Configured with `--enable-cet` option.
> If you mean it supporting CET functionality, then it is new enough.
As for commit 7bf1d7aeaffba15c4f680f93ae88fbef25427252, Guix provides `binutils` versions 2.38 and 2.33.1. According to the upstream commit history, both versions support Intel CET.
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301815750)
> > Shouldn't binutils ... also be CET-enabled?
>
> What is a CET-enbled binutils?
Configured with `--enable-cet` option.
> If you mean it supporting CET functionality, then it is new enough.
As for commit 7bf1d7aeaffba15c4f680f93ae88fbef25427252, Guix provides `binutils` versions 2.38 and 2.33.1. According to the upstream commit history, both versions support Intel CET.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724891863)
> highlighting to reviewers what is actually happening.
Personally I think this is already happening, because the code would be `orphanage.m_rng` vs `m_rng`, clarifying that `TxOrphanageTest::m_rng` is used, as opposed to `BasicTestingSetup::m_rng`.
I'll leave this as-is for now, but I may change it later, because either way is fine and I don't think it matters much.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724891863)
> highlighting to reviewers what is actually happening.
Personally I think this is already happening, because the code would be `orphanage.m_rng` vs `m_rng`, clarifying that `TxOrphanageTest::m_rng` is used, as opposed to `BasicTestingSetup::m_rng`.
I'll leave this as-is for now, but I may change it later, because either way is fine and I don't think it matters much.
💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301830456)
> Configured with --enable-cet option.
Similar to glibc, I'm pretty sure this autodetcted for some time, but has has been the default behaviour for at least the last ~5 years; so I don't think we need to do anything here (further evidenced by the fact that there is no such change in #30685).
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301830456)
> Configured with --enable-cet option.
Similar to glibc, I'm pretty sure this autodetcted for some time, but has has been the default behaviour for at least the last ~5 years; so I don't think we need to do anything here (further evidenced by the fact that there is no such change in #30685).
💬 maflcko commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1724897721)
nit, could clarify the link and where the value is taken from?
```suggestion
// Maximum number of addresses permitted in an ADDR message. (MAX_ADDR_TO_SEND)
```
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1724897721)
nit, could clarify the link and where the value is taken from?
```suggestion
// Maximum number of addresses permitted in an ADDR message. (MAX_ADDR_TO_SEND)
```
💬 l0rinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1724906265)
We're in 100% agreement here, thanks for detailing your view.
In this particular case my thinking was:
> initial code, parameter (copy of the original) is mutated locally, we have to keep track of state changes in our head to understand the method's behavior:
```C++
std::string_view RemovePrefixView2(std::string_view str, std::string_view prefix)
{
if (str.starts_with(prefix)) {
str.remove_prefix(prefix.size());
}
return str;
}
```
> adding const makes parameter
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1724906265)
We're in 100% agreement here, thanks for detailing your view.
In this particular case my thinking was:
> initial code, parameter (copy of the original) is mutated locally, we have to keep track of state changes in our head to understand the method's behavior:
```C++
std::string_view RemovePrefixView2(std::string_view str, std::string_view prefix)
{
if (str.starts_with(prefix)) {
str.remove_prefix(prefix.size());
}
return str;
}
```
> adding const makes parameter
...
🤔 glozow reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2250344089)
Pushed to address the first batch of comments, going to work on test parts next
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2250344089)
Pushed to address the first batch of comments, going to work on test parts next
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724880934)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724880934)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724779439)
What should the new name be?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724779439)
What should the new name be?