🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3351431350)
Focused on reviewing the RBF logic in e6315c24326016cfaee5bd046e8b2e4e1088ac6b

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440892816)
fba5200d59af87db240a1b061f995c63b0ed5ee8 nit: Could be "(Removed)" to be consistent with the other rules we no longer implement?

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440839627)
in e6315c24326016cfaee5bd046e8b2e4e1088ac6b

Nice! We should also remove "All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2." from the package RBF rules in policy/packages.md

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440894335)
```suggestion
* Feerate diagram policy enabled in conjunction with switch to cluster mempool as of **v31.0**.
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2441138984)
I think the package RBF logic can have one more tweak in e6315c24326016cfaee5bd046e8b2e4e1088ac6b, which I think @sipa brought up a few weeks ago:

We currently have the requirement that the package feerate is higher than the parent feerate because "we don't want the child to be only paying anti-DoS fees" but that's more restrictive than it needs to be. It's ok if the child has a lower feerate than the parent and only pays anti-DoS fees, as long as it pays for itself to be above the mempool mi
...

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2441096106)
e6315c24326: In both single and package settings, `ImprovesFeerateDiagram` is where cluster size limits are checked for the first time if RBF is happening. The check is cached and many RBF checks are cheaper than `CheckMemPoolPolicyLimits`, so that doesn't seem problematic.

However, this is assigning `TX_RECONSIDERABLE` as the error when cluster limits are exceeded, which will cause net_processing to give it some special treatment (we may redownload and revalidate in some circumstances, which
...

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440904975)
fba5200d59af87db240a1b061f995c63b0ed5ee8

Happy for this to go into a followup:

Maybe worth adding a bit more color for people who don't have any context: A singleton replacing a singleton must pay higher total fees and have a higher feerate (covers vast majority of cases). For more complex cases, see this link to more comprehensive explanation.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2441070793)
b9cbca76762 Reviewer note, I originally wanted to point out the extra commented line but then read the commit message and saw it changes back later in e6315c24326

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440825043)
I think the whole file can be deleted, as ancestor/descendant limits are now also obsolete.

I think we can write a new doc for cluster limits in a followup? And a release note (/me ducks)

⚠️ plebhash opened an issue: "[`v30.0`] `createNewBlock` never returns"
(https://github.com/bitcoin/bitcoin/issues/33647)
while connected to `testnet4` and letting the current implementation of SRI client connected for a while, `bitcoin-node` stops responding to IPC calls

steps to reproduce:
- build `bitcoin-node` from `v30.0` tag
- clone https://github.com/plebhash/sv2-bitcoin-core
- check out `2025-10-17-hanging-ipc` branch
- launch `bitcoin-node` with `-testnet4 -ipc-bind=unix -debug=ipc`
- launch `sv2-bitcoin-core` with `RUST_LOG=debug cargo run --example logger "/path/to/node.sock"`
- let it run for a while (
...

💬 theuni commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#issuecomment-3417291593)
My $.02 and repeating what I've said elsewhere... I still just don't think that this is the correct level of abstraction. IMO socket-based protocols are different enough to warrant their own logic. Either that, or the abstraction would need to provide a bazillion options and flags, ala libevent/libev/libuv/libkj/etc.

For ex: presumably in order to remain generic, `SendBytes` may "succeed" at sending only one byte of many. Meaning that implementations will have to maintain their own send buffe
...

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3417456349)
@hebasto Hello! The “CentOS, depends, gui” job fails and the reason is that it references ./ci/test/00_setup_env_native_centos.sh, which isn’t in this repo. Could we skip that job or rerun on Cirrus instead?

💬 151henry151 commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#issuecomment-3417599834)
> Can you please fix the PR description and commit message? `CMAKE_SKIP_INSTALL_RPATH` is removed from the build system, and `CMAKE_SKIP_RPATH` (which does more than `CMAKE_SKIP_INSTALL_RPATH`) is added to the Guix build. Can you also remove the (what looks like) AI generated content from the PR description and commit message. i.e the bullet pointed list of changes, for a 2 line diff.

I think I've addressed the inaccuracies; thanks for the feedback. Regarding the bullet pointed list of change
...

💬 dipankarjana3657-bot commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3417817836)
Hi

⚠️ dipankarjana3657-bot opened an issue: "Dj"
(https://github.com/bitcoin/bitcoin/issues/33648)
### Motivation

Hi

### Possible solution

_No response_

### Useful Skills

* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...


### Guidance for new contributors

Want to work on this issue?

For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2441590945)
I wanted to make it more realistic, but the tests do pass now without them, thanks for the feedback removed.

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-7.7x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2441593602)
Done, updated the commit messages and PR descriptions as well.

💬 purpleKarrot commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#issuecomment-3417857348)
ACK 4b41f99d57d822dfc258865d1dad03204fe0380f

It is a tiny step towards "variables that start with `CMAKE_` should not be set in `CMakeLists.txt`files".

💬 purpleKarrot commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3417898647)
> Given that the len argument is 0, memcpy() should not try to dereference the maybenull argument

According to the C standard (§7.26.1), the behaviour of `memcpy`is undefined if either `src`or `dst`are null. So it is not a bug.

✅ achow101 closed an issue: "Dj"
(https://github.com/bitcoin/bitcoin/issues/33648)