🤔 hodlinator reviewed a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2725634006)
Latest push:
* Removes `test_instant_rpc_timeout` which was artificially setting `rpc_timeout = 0`, which is not a supported case and which caused different behavior on Windows network stacks compared to Linux. That test was a rough subset of `test_wrong_rpc_port`. Removing `test_instant_rpc_timeout` allowed dropping the commit with the workaround for socket timeout with unset errno entirely. **Special thanks to @maflcko for [insisting](https://github.com/bitcoin/bitcoin/pull/30660#discussion_r
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018732697)
Dropped the test and workaround commit in latest push.

📝 willcl-ark opened a pull request: "net, pcp: handle multi-part responses and filter..."
(https://github.com/bitcoin/bitcoin/pull/32159)
...for default route in pcp pinholing.

Currently we only make a single recv call, which trucates results from large routing tables, or in the case the kernel may split the message into multiple responses (which may happen with `NLM_F_DUMP`).

We also do not filter on the default route. For IPv6, this led to selecting the first route with an `RTA_GATEWAY` attribute, often a non-default route instead of the actual default. This caused PCP port mapping failures because the wrong gateway was us
...

💬 willcl-ark commented on pull request "net, pcp: handle multi-part responses and filter...":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-2761490048)
cc @laanwj

This patch maintains the FreeBSD-style querying/filtering we have currently, but but increases the size of the response processed to a maximum of 1MB.

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2018767731)
> If you want to implement some version of it I'm happy to review, or we can defer it, not sure when I'll be able to get it into good shape.

Done in https://github.com/bitcoin/bitcoin/pull/32158. I ended up only taking `thread::scope` and `VecDeque` and restructured the rest. Lmk if this sounds good, or if you want to be listed as co-author.

💬 martinus commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2018784600)
@sipa thanks, you are of course right; I should have read a bit more about that before... Nevertheless I think it's at least interesting to see what difference a fast hash would make.

🤔 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