💬 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?
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724818171)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724818171)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724863898)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724863898)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724840174)
agree, though leaving out of this PR to limit its scope. Could add a nonsegwit parent + segwit child test to be added to p2p_orphan_handling.py. Maybe a good first issue.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724840174)
agree, though leaving out of this PR to limit its scope. Could add a nonsegwit parent + segwit child test to be added to p2p_orphan_handling.py. Maybe a good first issue.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724832923)
Renamed it to `first_time_failure` in a preceding commit.
Yeah that's correct. Basically, if this is the first time a tx is being rejected from mempool, we consider putting it in `vExtraTxnForCompact`. If it's not the first time, we don't need to do it again, presumably because we don't want duplicates (it's a ring buffer).
Same thing for adding something as an orphan. If this bool is false, it means the tx is already an orphan or it's the low feerate parent in a package. Either way we don
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724832923)
Renamed it to `first_time_failure` in a preceding commit.
Yeah that's correct. Basically, if this is the first time a tx is being rejected from mempool, we consider putting it in `vExtraTxnForCompact`. If it's not the first time, we don't need to do it again, presumably because we don't want duplicates (it's a ring buffer).
Same thing for adding something as an orphan. If this bool is false, it means the tx is already an orphan or it's the low feerate parent in a package. Either way we don
...