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

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096457403)
I disagree this is a more descriptive bitness check. The size of pointers is what we actually care about here. Shouldn't matter in practice but i don't even think it's strictly guaranteed that `SIZE_MAX == sizeof(void*)`. Finally, this is the exact same check we do elsewhere in this codebase to check for 32-bit systems. For these reasons, i kept it as-is.

Made `constexpr` as requested, though.

💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r2096471698)
Good idea - done!

👍 ryanofsky approved a pull request: "Add checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31981#pullrequestreview-2840680200)
Code review ACK c759ec1256a5cdc02b6eef1f4c0935c693bb8087. Since last review just rebased, applied some suggested changes to use BlockValidationState, and split the new python test into functions.

💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2089235016)
In commit "test: more template verification tests" (c759ec1256a5cdc02b6eef1f4c0935c693bb8087)

Maybe add comment `# solve block_2` if that would clarify dependency between pow_test and later tests.

(Alternately could change `pow_test` to return a different variable like `block_2_solved`)