💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1782505271)
Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.
```suggestion
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1782953943)
done

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2386103070)
@maflcko done, thanks

💬 Sjors commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2386113770)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1782965796)
Exactly like that, thanks!

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1782975091)
Ah, yeah, `mempool_util.py` looks like a better alternative than `blocktools.py`.

`test_node.py` feels more like the high-level P2P stuff to me at least + lower level dynamic RPC invocation.

💬 Sjors commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386200566)
utACK fd38711217cafbd62e8abd22d2b43f85fede8cde

I suppose the alternative would be to move `netaddress.h` to the util library.

The `libraries.md` document is a bit vague on what belongs to `util` vs `common`. It just says "low level". Since util is also used by `libbitcoin_kernel` it could make sense to move things out of it that the kernel doesn't need, but #28690 introduces a whole new `kernel_util` for that purpose.

💬 hebasto commented on pull request "build, qt: Add Wayland support for Linux builds with depends":
(https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-2386230968)
This PR might be reconsidered along with https://github.com/bitcoin/bitcoin/issues/29914.

👍 theuni approved a pull request: "depends: Print ready-to-use `--toolchain` option for CMake invocation"
(https://github.com/bitcoin/bitcoin/pull/31008#pullrequestreview-2340626823)
ACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5

💬 ryanofsky commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2386241193)
> MSAN says https://cirrus-ci.com/task/6248232848719872

Thanks! I created https://github.com/chaincodelabs/libmultiprocess/issues/115 with details about this, and I think I have a potential fix.

💬 ryanofsky commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386313836)
> utACK [fd38711](https://github.com/bitcoin/bitcoin/commit/fd38711217cafbd62e8abd22d2b43f85fede8cde)
>
> I suppose the alternative would be to move `netaddress.h` to the util library.

I think that might work, but I didn't look into it because I don't know if netaddress.h depends on other things in common that would require more dependencies to be moved. Also the pcp/netif files were added recently in #30043, while netaddress.h is older so I assume it is more likely netaddress is in the pl
...

💬 petertodd commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2386377588)
@arik-so FWIW I actually fix this loophole in Libre Relay by prohibiting replacements that add unconfirmed inputs to have multiple conflicts: https://github.com/petertodd/bitcoin/commit/b2b867f458388d5177aec378f95a874917fd6442

Libre Relay needs to do that because it implements replace-by-fee-rate, allowing transactions to be replaced if the fee-rate is increased. This loophole allows fee-rates to be decreased, which would allow an infinite replacement cycle.

Transactions violating this new
...

💬 theuni commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2386390637)
@davidgumberg Thanks for the very detailed description!

I suspect the speedup here is going to be very dependent on the architecture/environment, but it's not clear to me exactly what variables would matter most.

Would you be interested in working up a benchmark that illustrates the difference? If we knew exactly where the speedup was coming from, we could make a more informed decision about what/where to optimize.

👍 dergoegge approved a pull request: "fuzz: fix bug in p2p_headers_presync harness"
(https://github.com/bitcoin/bitcoin/pull/30980#pullrequestreview-2340806208)
utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2386452790)
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2385186175

Thanks for the review! Fixed the problem with `-debug=ipc` now. This was missing a call to parse `-debug` arguments.

Updated 2227afac0372358287fb879f3b0bd07fd653f3f8 -> 2f9c2f4c6dcf8c45514e99d1018301aba61940fc ([`pr/mine.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.11) -> [`pr/mine.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/min
...

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2386473736)
ACK 2f9c2f4c6dcf8c45514e99d1018301aba61940fc

Logging works now.

💬 Sjors commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386481778)
> I think it util is a good place for things that are general purpose and can be broadly useful, and common is a better place for things that are hacky or more specific to the bitcoin core codebase.

The PCP code is not Bitcoin specific. Some things in `netaddress.h` are though, but tearing that file apart doesn't seem worth the effort for now.

💬 ryanofsky commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2386491429)
Could you try cherry-picking following commit to fix the MSAN failure? commit 7363a3eb8a1771b862febf3bd7257ad093887485 ([branch](https://github.com/ryanofsky/bitcoin/commits/pr/downdep))

📝 brunoerg opened a pull request: "net: fuzz: bypass network magic and checksum validation"
(https://github.com/bitcoin/bitcoin/pull/31012)
Fixes #30957

We currently have some instructions on the documentation about how to fuzz the Bitcoin Core P2P layer using Honggfuzz NetDriver. It includes a patch to be applied to bypass network magic and checksum validation, that is outdated. However, we can change the code to bypass them using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2386500231)
cc: @dergoegge