✅ hebasto closed an issue: "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]"
(https://github.com/bitcoin/bitcoin/issues/32558)

🚀 hebasto merged a pull request: "Use WitnessV0KeyHash in TestAddAddressesToSendBook"
(https://github.com/bitcoin-core/gui/pull/875)

🤔 darosior reviewed a pull request: "script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2851775828)
This is a significant refactoring of tricky consensus critical routines that have essentially never been modified since Satoshi wrote them 15 years ago. The exploration is interesting but it seems ill-advised to completely overhaul this critical code for a marginal IBD speedup.

I feel bad for being stop energy here, but since i have been an advocate for us voicing our opinion instead of just ignoring PRs i'll go and own it. Concept NACK: i don't think the risk-benefit ratio of this change is
...

🤔 mzumsande reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2851588776)
Concept ACK

💬 mzumsande commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096386541)
This comment is outdated, because `DEFAULT_ANCESTOR_LIMIT = 25` is no longer after the last commit. Not sure about the reason why it was used before, is it no longer valid?

Also, changing DEFAULT_ANCESTOR_LIMIT to 100 could be mentioned in the commit message, it also affects the other subtests.

💬 mzumsande commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096229309)
I don't understand this comment. Did you use prioritisetransaction in an older version, but dropped that?

🚀 ryanofsky merged a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221)

✅ ryanofsky closed an issue: "wallet: wrong balance and crash after reorg and unclean shutdown"
(https://github.com/bitcoin/bitcoin/issues/31824)

💬 ryanofsky commented on pull request "script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2892128673)
Might also help to add all the benchmark and test changes and simpler improvements like inlining in a separate PR, so it's a more clear how tricky the sensitive changes here are, and if the sensitive changes are really worth making.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096403730)
I understand the slight additional complexity concern but for a transaction submitted to us with no PoW i think we should discard it as soon as we know we won't accept it. I really don't think we should keep doing unnecessary work by iterating, solving scripts and counting.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096405194)
Don't you think it would be confusing to point here since this function does not concern itself with the witness? Especially as it's pointed right before.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096415934)
Done, added a sentence to this effect.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416043)
Done.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416080)
That's actually what i had initially but i reduced it to `MAX_LEGACY_SIGOPS` in an attempt to avoid too long variable names. With the comment expliciting it's about transaction inputs, i think it should be fine as-is.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416254)
Expanded the test to check the original, non-standard, one can still be mined.

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096422126)
If it's set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash after having received enough blocks. 1 GiB seems like a reasonable maximum for a 32-bit system.

💬 fjahr commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2892174264)
ACK 7193245cd66791216d4e586ca09302b26d4b7528

Confirmed there are no such comments left outside of subtrees. Also seeing different full include lists suggested by IWYU but those seem exaggerated for this change here. Checked a hand full of the files that had bigger changes and didn't find anything wrong.

🤔 ryanofsky reviewed a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2851907313)
Concept ACK 218db39973736ac50e912bec440b1e512586d3b0

💬 ryanofsky commented on pull request "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors":
(https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2096430609)
In commit "wallet: Use util::Error throughout AddWalletDescriptor" (218db39973736ac50e912bec440b1e512586d3b0)

I think I understand the need to use std::reference_wrapper with util::Result. But it would seem good to replace `std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>>` here and other places with just `DescriptorScriptPubKeyMan*` for simplicity and avoid reference_wrapper except in the cases where it's really needed. It seems like doing this would make a lot of the test cha
...

🤔 instagibbs reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2840518650)
acb2b6a90a47567773268a024bcf2f1ddfcc60df

focused on motivation/structure/approach and test coverage for first pass. Haven't validated the actual trimming logic yet.