💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1953250620)
These are now missing `DEPENDS_EXPLICIT_OPT` from #30911.

💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#discussion_r1953249473)
I think this would make sense as 2 separate functions. Seems a weird mix of args otherwise.
`target_raw_data_source` and `target_json_data_source`?

I think it'd be fine to call both funcs for targets that need both types of files generated.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953254091)
:man_shrugging: I hope the code is clear enough that reviewers are convinced this is impossible. The `Assume` hopefully helps with that convincing. I hope an assertion doesn't add to that.

💬 theuni commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2654622689)
Also, since you're messing with this, please consider the `CODEGEN` option for `add_custom_command`, which would need feature-gating same as `DEPENDS_EXPLICIT_OPT`.

👍 laanwj approved a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2612972081)
re-ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2654640516)
@maflcko Mind taking a look at the first commit here to see if you'd prefer a different approach?

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2612848923)
Code review ACK ce9c2e17d5d4ef99cba82521f2515e34130a02a1. I don't think this needs to depend on the other PR #31785, because I don't see how the assume check https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937596836 could fail. Left some minor comments below though, including one about how assume check could be handled better.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953193832)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

This may all be true, but I don't think it clearly explains the behavior when MAX_MONEY is specified. Would suggest a simpler comment:

* The wait method will not return a new template unless it has fees at least `fee_threshold` sats higher than the current template, or unless the chain tip changes and the previous template is no longer valid. Determining whether `fee_threshold` is reached is ex
...

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953241280)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

Any thoughts on whether it makes sense for this polling interval to be configurable? I could imagine a miner not really caring how expensive this check is and just wanting it to be as fast as possible?

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953250463)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

The "// This just adds coverage" comment should be moved above the waitTipChanged call since it does not apply to the waitNext call, which is needed to set `block_template` for the next loop iteration.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953227295)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760

This discussion is interesting but I don't understand the line

https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/node/miner.cpp#L157

at all. Why does it even make sense?

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953213920)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

IMO would be slightly better to write this as `options.fee_threshold < MAX_MONEY || tip_changed`, so MAX_MONEY is not a magic value, just an optimization cutoff for the point where we stop checking if fee_threshold has been reached because we know it is too high to be reached.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953206134)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

IMO it is confusing to define an extra lambda here that is only called in one place (inside another lambda). It would be less confusing to just inline `check_tip_changed` in the place where it is used, to remove a level of indirection and make the order code is written match the order that it executes. Would suggest


<details><summary>diff</summary>
<p>

```diff
--- a/src/node/interfaces.c
...

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953247978)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

I found this comment pretty confusing and would suggest making it more specific like "// if current_height is odd, block_template will have already been set at the end of the previous loop."

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953275413)
In commit "Add waitNext() to BlockTemplate interface" (7f6f58bad8e56c9a25ffbc0d105bed37e9958fef)

Seem like it would be more correct (and more compact) to write this as:

tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;

Since if tip_block is null the right thing to do is just wait for it to be non-null, not set tip_changed to true and try to return a new block template below.

👍 brunoerg approved a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2613027538)
code review ACK af76664b12d8611b606a7e755a103a20542ee539

💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2654723569)
Rebased on master. Thanks @marcofleon for fixing the fuzz target for me, i addressed your comments.

💬 darosior commented on pull request "Doc: add a comment referencing past vulnerability next to where it was fixed":
(https://github.com/bitcoin/bitcoin/pull/30538#issuecomment-2654741147)
Rebased.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953374431)
I don't feel that's necessary. The calling code is wrong if it's called with an empty `to_remove`. It's hopefully sufficient to document this, so reviewers can be convinced this isn't being violated.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1953384987)
Done.