💬 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
...

🤔 glozow reviewed a pull request: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398#pullrequestreview-2725923391)
Thanks for rebasing! Needs release note.

💬 glozow commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#discussion_r2018888797)
This looks incorrect, it's still using <=64

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2761728012)
@hodlinator
> > A new style plugin causes minor visual glitches that will be fixed in follow-ups.
>
> Did not notice glitches, what are you referring to?

I've added a screenshot to the PR description.