💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319357837)
5bc5edf8743408979add1b15d23cc8ab7b948db Checking limits using changeset is so nice. It could be helpful to add a comment that `changeset` is automatically destroyed at the end of this function and thus doesn't need to be handled/aborted.

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319334033)
e72b0b1a7eb31078c998d95b4cfffe9cd1f9113d re-adds `-addrmantest` which was removed in #29007

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319516863)
nit in aa8eb8c3ab1 Fix miniminer_tests to work with cluster limits, I think it's txp0 to txp31 and txc0 to txc30?

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319578206)
Perhaps also add `(maximum: %u), MAX_CLUSTER_COUNT_LIMIT`

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319395594)
For f172ac5e6c37f446bc3c38419cf40f3adac3ddd8 Use cluster linearization for transaction relay sort order

In addition to changing the inv order, should we also use the transaction's chunk feerate for fee filtering?

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2319721247)
34251141eca785578a0b2ca6d1c0932d34b2418d Do not allow mempool clusters to exceed configured limits

This workaround ("Temporarily increase the maximum permitted cluster count so that feature_rbf.py passes. ") didn't seem ideal to me. I shuffled around some of the changes so that feature_rbf is reworked before this commit: https://github.com/glozow/bitcoin/commits/2025-09-cluster-feature_rbf/

It just deletes one of the tests (also see https://github.com/bitcoin/bitcoin/pull/28676#discussion_
...

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319744875)
Yes, the specific failure we want to know about is when there is no rescan. Rescanning hides the problem.

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319750635)
There's nothing to assert, it either error and throws, or it succeeds and returns. The return value can never be false-y.

📝 Crypt-iQ opened a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296)
This avoids an Assume crash if multiple blocktxn messages are received. The first call to `FillBlock` would make the header empty via `SetNull` and the second call to `FillBlock` would crash [here](https://github.com/bitcoin/bitcoin/blob/689a32197638e92995dd8eb071425715f5fdc3a4/src/net_processing.cpp#L3333) since `LookupBlockIndex` won't find anything. Fix that by checking for an empty header before the Assume.

💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2319776496)
might note that the header is blanked in FillBlock, but the partial block never deleted, which is why this would catch the behavior

👍 ryanofsky approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3181743977)
Code review ACK 235016f5b78ba9f472b56df0825690307fffc7e6. Nice to have this ability to test the interfaces directly. Left a few comments but none are important

💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319744962)
In commit "ci: enable IPC tests in CI" (7eb71775dce6f3f2fd0e4b62f439aab86d1b65a0)

Would seem good to drop --break-system-packages here and only add it on the platforms where it's needed following:

https://github.com/bitcoin/bitcoin/blob/689a32197638e92995dd8eb071425715f5fdc3a4/ci/test/00_setup_env_mac_native.sh#L9-L12

It's also not clear what the problem is if --break-system-packages is not used. Does the package fail to install otherwise?

💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319703609)
In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)

capnp_dir is listed twice here

💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319697575)
In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)

Does this need to say `pip install ./pycapnp`? It seems like on my system without `./` pip ignores the git clone and downloads a wheel instead. Same might apply to the venv instructions below.

💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319701840)
In commit "tests: add functional tests for IPC interface" (592e253f76764f108e9c34fa0da32451091b946a)

s/there is/there is no/

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319788315)
The test coverage for this commit is provided in the first commit of the PR. The point of that test is to ensure that behavior doesn't change for commit.

💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2319795093)
Yup good point, will do.

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319820571)
Done as suggested

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319820786)
Moved

💬 achow101 commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2319821210)
Done