💬 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`)

💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337)
In commit "Add checkBlock to Mining interface" (116a9d2daced606e5f35896372da39fd914d3da1)

Just a thought, but it doesn't seem to me that the changes to rpc/mining.cpp here and below in getblocktemplate are really improvements. They are making code more verbose and changing behavior in ways that don't necessarily seem better (producing an error message with a trailing " - " if debug string is empty here, adding a new LogDebug statement below with only the debug string, not the reason string).
...

🤔 ismaelsadeeq reviewed a pull request: "multiprocess: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2851969416)
Concept ACK

This can serve as a usage example for the interfaces.

I've rebased this on `master`: https://github.com/ismaelsadeeq/bitcoin/tree/05-2025-bitcoin-mine-rebase and did a few tests.

1 - Ran `bitcoin-mine` without a `bitcoin-node` process — it executed correctly and showed an appropriate error message.
<details>
<summary>logs</summary>

```terminal
2025-05-19T21:00:18Z Default data directory /Users/abubakarismail/Library/Application Support/Bitcoin
2025-05-19T21:00:1
...

💬 ismaelsadeeq commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2096479030)
In " multiprocess: Add bitcoin-mine test program " 8e31982d5a940f04a815ec235cb55c0dc2f870d7

I think it could be helpful to make the name a bit more generic, since the program demonstrates IPC usage rather than actual mining. Maybe something like bitcoin-interfaces-client or interfaces-client?

💬 ismaelsadeeq commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2096468681)
In " multiprocess: Add bitcoin-mine test program " 8e31982d5a940f04a815ec235cb55c0dc2f870d7

I think this commit should be split into two, the first that introduces `src/init/bitcoin-mine.cpp` and then next commit should introduce `src/bitcoin-mine.cpp`

💬 fjahr commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2892383304)
utACK 33332829333b589420f8038541d04ec6970f051d

I think I found two leftover mentions of the `ParseInt` in documentation: https://github.com/fjahr/bitcoin/commit/1f2f1a432b0fdb3d84eecc9dc3a2db34d88dc892 I can option a follow-up if you don't want to address it here.