💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937777590)
Taken (with some changes)

💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937779528)
Got it.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2628133425)
I went back to using the `MAX_MONEY` magic value and added a helper `check_tip_changed()` to make sure we check for a tip update _before_ waiting.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937808429)
I added check before we start the wait.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937809044)
If that happens we'll catch it at the start of the next loop, with the above change.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628139870)
> I'm suggesting that the `WITH_LIBMULTIPROCESS` option can go away entirely.

If you look at the PR, you can see that `WITH_LIBMULTIPROCESS` option adds 5 lines of code and 2 of them are `endif()`. So I wouldn't consider that a real coup. Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see https://gith
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937818071)
`m_tip_block_mutex` and `cs_main` locks don't jive well, so I limited the places where we actually check

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937834069)
A comment to remember this.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937839814)
It would require a second thread, because the main thread of the unit test is waiting on `waitNext()`.

💬 achow101 commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2628184000)
ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

💬 achow101 commented on pull request "test: Added coverage to the waitfornewblock rpc":
(https://github.com/bitcoin/bitcoin/pull/31746#issuecomment-2628229678)
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

🚀 achow101 merged a pull request: "rpc: have getblocktemplate mintime account for timewarp"
(https://github.com/bitcoin/bitcoin/pull/31600)

🚀 achow101 merged a pull request: "test: Added coverage to the waitfornewblock rpc"
(https://github.com/bitcoin/bitcoin/pull/31746)

👍 pinheadmz approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2587643094)
ACK fd4af64d58901334d907dd0bd38efd303a21374a

This patch consolidates two 4000 WU reservations into one constant, and adjusts configuration parameters to maintain existing behavior. It allows more fine tuned control of block template creation and in particular will allow some mining pools to squeeze an extra 4000 WU of transaction data into new blocks.

Reviewed each commit (again). Built and ran unit and functional tests on macos/arm.
I left a few if-you-touch nits below to improve clarity
...

💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937881476)
c39ad81353d89cd60eabd6343263afed91d9060d

same thought here about using policy constant but describing consensus constant in the message

💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937875968)
c39ad81353d89cd60eabd6343263afed91d9060d

another nit-if-you-touch:

I think "...and the block header" is a little confusing in this sentence. Does `blockreseredweight` include the header or not?

If it does I think this is more clear:
> "Reserve space to account for the largest coinbase transaction and block header that mining software may add to the block"

💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937828541)
b7ec4bf672425c71211415c2ea83e05a246ea66d

nit only if you touch, I think it would make more sense to use the consensus value `MAX_BLOCK_WEIGHT` of course that is equal to the default policy value anyway (for now) but the error message specifically refers to the consensus limit

💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937885553)
c39ad81353d89cd60eabd6343263afed91d9060d

nit: Doesn't really matter but I'm just noticing you are instantiating an object just to get a constant from it. Feels like a cleaner option would be to define `DEFAULT_BLOCK_RESERVED_WEIGHT`

🤔 fjahr reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2587690660)
Concept ACK

💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937862071)
Couldn't you use `send_self_transfer_multi` instead?

💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937893671)
nit: not sure if this will need to be reformatted later. Usually there is just one bullet per PR/change and it seems like this could be just one block of text. Just to take some editing headache from whoever manages the release...