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

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

🤔 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

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(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?

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(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

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

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

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724864056)
added

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724778335)
Seems fine to do, but won't add it to this PR because I want to limit its scope. Agree just the last paragraph is sufficient.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724882313)
added

💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301941489)
> I don't think we need to do anything here

In the current state, the statement from the PR description:
> CET is enabled on Linux/x86

is not accurate.

💬 hebasto commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301960884)
> > I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.
>
> Looks like upstream either forgot about, or missed this, so I guess someone should followup.

It has been just [merged](https://github.com/zeromq/libzmq/commit/5f408ba371ae4789549fb4696dcccd2ac946b7eb).

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2301983925)
The naming of the files and tests is a bit inconsistent, I would suggest the following:

* `txdownloadman.h`, `txdownloadman.cpp`, `txdownloadman_impl.h` and `txdownloadman_impl.cpp` with the classes: `TxDownloadManager` and `TxDownloadManagerImpl`
* `fuzz/txdownloadman.cpp`: `txdownloadman` + `txdownloadman_impl`
* `fuzz/txdownloadman_one_honest_peer.cpp`: `txdownloadman_one_honest_peer`

I don't care about the specific names just the consistency, e.g. don't mix `tx_download` and `txdownl
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725039042)
think you added the other way, that if `should_validate` is false, it must have a package to validate, which is incorrect(and caught by our 1p1c tests)

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1725039993)
Sounds good, I'll address it.

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1725041385)
done

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2302041809)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1724897721

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725043966)
Even though the prior version had many ACKs, this reaches another level of awesome.
Initial version using `""_hex` now pushed in daba1a25a62e72e9797a134c6377d17a9274a25f with @l0rinc as main author of that commit! :tada:

Resolving this specific thread.