💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2441463955)
My Guix build:
```
aarch64
fba7e542bc6a35c65a179f9c8cd3ae2b0f8b12cd5895fa255a2d344d576d0215 guix-build-915640e191b6/output/aarch64-linux-gnu/SHA256SUMS.part
c9c1d47120de9a45aa5da1c17cc8f4860f2268bfe413d38e1b6f67ea96bd4f09 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu-debug.tar.gz
c1775debe30e5a28d981720582c5e6f36ea33939169a429edc48c4bb8ebc3688 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu.tar.gz
870a9d4f
...

💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441473829)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2441487157)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.

🤔 naumenkogs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2398752467)
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818860807)
now that i realize it's a very internal thing to the download manager, i understand why this name was chosen. resolve pls.

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818926518)
nit

f497414ce76a4cf44fa669e3665746cc17710fc6 still has `Assume` removed

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818882978)
042a97ce7fc672021cdb1dee62a550ef19c208fb

code-wise i think this better belongs around `MakeAndPushMessage` (what if the caller decides to postpone the actual sending), but no big deal.

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833)
Okay, so

` /** New inv has been received. May be added as a candidate to txrequest. */` is inaccurate, right? There is no new INV received in `p2p_inv=false` case, but rather it's called from orphan consideration after processing a TX?

👍 TheCharlatan approved a pull request: "build: Rename `PACKAGE_*` variables to `CLIENT_*`"
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2399049784)
Re-ACK 70713303b6399e0627c1960da4fbab48f9cf71e7

🤔 stickies-v reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554)
Note: `doc/bips.md` needs updating, "opt-in" is no longer accurate: https://github.com/bitcoin/bitcoin/blob/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001/doc/bips.md?plain=1#L37

💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819051835)
Keeping this section here feels unnecessarily confusing to me. Since it's no longer relevant, I'd just remove the entire paragraph, and potentially add a line in the `## History` section?

💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1818979310)
nit: suggestion to provide a bit more context. Ideally, at least the PR number is added.
```suggestion
Starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of this policy, users no longer
benefit from disabling it, so the option has been removed, making full
replace-by-fee the standard behavior. (#30592)
```

💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1815326806)
nit: would just drop this line (+newline entirely), already covered in `self.log.info('A transaction that replaces a mempool transaction')`

💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819055432)
(note: this did not get fixed - it's a trivial re-ack, would be ideal to get this in this PR - it's very confusing when reading the code)

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819093318)
Misunderstood the code at time of writing this comment, mark as resolved

👍 stickies-v approved a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2399208958)
ACK d295638890719a009b480148545288e2dbdd1a5a - although I'll quickly re-ACK if you address my [outstanding comments](https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554) too.

Nice clean-up, both for users and the codebase, I don't see the point of keeping this option around any longer.

> if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares.

I don't
...

💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819160027)
Sure I'll use `ToString()` here. Going to leave the hyphens though because there are several other hyphenated fields in the response object already... but I do see lots of underscores in other RPC responses 😬

💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819194151)
fixed thanks

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819197161)
This is updated -- does this seem sufficient now, or should I add more documentation about how reorgs work?

💬 hebasto commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2441827065)
> > However, this assumption can be wrong in environments where the [CMAKE_GENERATOR](https://cmake.org/cmake/help/latest/envvar/CMAKE_GENERATOR.html) variable is set.
>
> Generally, this holds for many CMake variables. Wouldn't a better approach be facilitating it working, rather than guaranteeing these environments never get the expected bahaviour.

Such behaviour can be achieved by switching from `$(MAKE)` to `cmake --build ...` in the `$(package)_build_cmds`. However, it would be a much
...