🤔 sipa reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2725731979)
Changes:
* `BlockBuilder` interface was changed, it now just has:
* `std::optional<std::pair<std::vector<Ref*, FeePerWeight>>> GetCurrentChunk()`
* `void Include()`
* `void Skip()`
Which avoids the need for it to store `Ref*` internally.
* Improved `GetMainStagingDiagrams` testing, to verify that unmodified clusters are indeed not included in the diagrams.
* Split off the introduction of the chunk index observers to a separate commit.
* Split off the generalization of `GetCluster
...

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018792783)
This code is gone, but no, I did mean `>= 2` here (you cannot call GetMainStagingDiagrams unless you have at least 2 diagrams).

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018784874)
I have added a more accurate approach, I think: the staging trim graph now keeps track of which transactions have been potentially modified since being copied from main. The final comparison checks that the sections of the diagram missing from both `GetMainStagingDiagrams` results includes at least all those that have definitely not been modified.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018792381)
Done (a while ago, forgot to respond).

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018785993)
This is no longer needed now that `BlockBuilderImpl` no longer holds on to a vector of `Ref*`, I think.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018797415)
I have moved the introduction of `m_chunkindex_observers` to its own commit.

👍 TheCharlatan approved a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2725555411)
ACK 17718660b8c95d1c12124ba2f38baf286a3bddf2

This looks fine, but I think the commit messages could all be a bit clearer.

💬 TheCharlatan commented on pull request "validation: set BLOCK_FAILED_CHILD correctly":
(https://github.com/bitcoin/bitcoin/pull/31835#discussion_r2018682489)
In commit 27b5abf946e737940bba5ea4b82385a100a35156:

The diagram in the commit message is a bit confusing. Are the numbers supposed to be block heights? Should the arrows be inverted then?

💬 TheCharlatan commented on pull request "validation: set BLOCK_FAILED_CHILD correctly":
(https://github.com/bitcoin/bitcoin/pull/31835#discussion_r2018755035)
Nit: Should this say "Check all ancestors of the invalidated block are validated up to..."?

⚠️ glozow opened an issue: "Test Package Accept"
(https://github.com/bitcoin/bitcoin/issues/32160)
### Please describe the feature you'd like to see added.

**Description of how the feature should work:**
The `testmempoolaccept` RPC should continue to take a list of hex transactions with minimal restrictions (no duplicates, no conflicting transactions, not too many). It should allow singleton transactions, transactions already in mempool, and any topology (multiple disconnected components should be ok). It should attempt to validate as much as possible and return each transaction's validation
...

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-2761607177)
Concept ACK

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2725831188)
Code review ACK fc88c2ec297dc93ba06008bd5ae10798e9f6aeac. Just dropped 0 timeout workaround and instant timeout test since the last review.

Ideally, I would want setting a 0 timeout to result in a comprehensible error message lke "Unable to connect to bitcoind after 0s" and not some harder to debug behavior, but it wouldn't be worth restructuring the code or adding a special case for.

💬 hebasto commented on pull request "depends: patch Qt rounding bugs":
(https://github.com/bitcoin/bitcoin/pull/32081#issuecomment-2761649016)
> I guess this won't be needed after #30997 (assuming that it makes it in for the next release)

I agree: https://github.com/bitcoin/bitcoin/blob/9ca76aa4210d15060fe35eb95da0862af57a8cc9/depends/patches/qt/normalize_round.patch#L3

💬 Lagrang3 commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2761649127)
I've upgraded my mainnet node to v29.0rc2 with no issues running the following services:
- electrs
- datum
- and core-lightning

🚀 ryanofsky merged a pull request: "wallet: remove redundant `Assert` call when block is disconnected"
(https://github.com/bitcoin/bitcoin/pull/32153)

💬 Lagrang3 commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2761677085)
> [@Lagrang3](https://github.com/Lagrang3) thanks for the testing. I assume like other Archer routers it was enabled by default?

Yes. Out of the box.

💬 dergoegge commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2761681405)
lgtm, afaiu this shouldn't have any downsides to existing infra

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2018881733)
Is it guaranteed that the reponse to `NLM_F_DUMP` is always multipart? Or do we need to check `nlmsg_flags` for `NLM_F_MULTI`, and if not, break after the first packet?

This is not clear to me from the documentation:
- https://man7.org/linux/man-pages/man7/netlink.7.html
- https://man7.org/linux/man-pages/man7/rtnetlink.7.html

👍 ryanofsky approved a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2725913318)
Code review ACK fc88c2ec297dc93ba06008bd5ae10798e9f6aeac. Just rebased and added f-string since last review.

💬 ryanofsky commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018882591)
re: https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2018038275

I do like error_message parameter idea, fwiw. I could see these making tests more readable if they started being used for more important or more confusing checks. Tests are often written with some important checks directly related to the thing being tested, and other less important checks to see if other things are consistent. Having a place to indicate what it actually means when a check fails could be helpful for more
...