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

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089137415)
462429bc159b1b5ddb5443c484c9bdeae985ad26

nitty: max_agg_size

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089151874)
462429bc159b1b5ddb5443c484c9bdeae985ad26

misinterpreted the commit message that the _aggregate_ size should not be hit when adding transaction, rather than each individual txn shouldn't exceed this limit. Maybe make this crystal clear.

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089169224)
462429bc159b1b5ddb5443c484c9bdeae985ad26

nit: docstring for `m_group_oversized` should be updated

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089187012)
462429bc159b1b5ddb5443c484c9bdeae985ad26

Was a bit confusing at first, but this must be true because clusters are never merged (via dependency application) when to-be-merged-clusters would be oversized.

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089177610)
and can't also be 0? Didn't double check this was already the case, but an assertion is added in 462429bc159b1b5ddb5443c484c9bdeae985ad26

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089165076)
462429bc159b1b5ddb5443c484c9bdeae985ad26

nitty: GetTotalTxSize? GetAggTxSize? GetTxsSize?

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089210412)
439ccf713b8f9de1dff1d02678bb68d0f29ea175

nit: `SINGLETON_OVERSIZED` to reduce later mental gymnastics

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089208525)
439ccf713b8f9de1dff1d02678bb68d0f29ea175

commit message maybe could get beefed up a bit with motivation since at first blush it seems odd to add singletons we wouldn't have relayed in the first place

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089215165)
439ccf713b8f9de1dff1d02678bb68d0f29ea175

please add documentation on what `oversized_tx` should be

👍 ryanofsky approved a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2851941615)
Code review ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851. Nice new test coverage and simple optimization

💬 ryanofsky commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2096452073)
re: https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165

I don't think I understand suggestion to use Assume. But if the hash is always available I guess expected_hash could be a required parameter instead of an optional one. Would seem fine to expand the PR, or just keep it focused on not calculating the hash twice.

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096454785)
If it's set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash once there is high transaction traffic. Really if someone wants to use a large mempool they should use a 64-bit system.

I don't think this constant should be a `size_t` because it gets compared to a signed integer.

💬 instagibbs commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096455750)
Kept the test setup as is, since it tests a meaningful 3 block reorg. Changed the comment because the motivation is different.

💬 instagibbs commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096455892)
oops, yep, removed

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096457039)
Sjors, i took your suggestion. L0rinc, as explained above i don't think this is an improvement.

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096457284)
I adapted to the surrounding code. For `-dbcache` we trunk so i did the same. For `-maxmempool` we return an error so i did the same.