💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936250003)
Did you mean a61fb2fd2d6b338c08c619e4605521a41bd3edd9?

`cs_main` is only held briefly, _after_ each 1 second wait, once per loop.

This PR currently makes 1 template per second to get the fees. Once we have cluster mempool, I'm hoping there will be a lighter weight method to see fees in the top 4 mega-weight units have risen. cc @sipa / @sdaftuar

One of the stratum v2 goals is to push new templates faster, so I'd rather keep the tick at 1 second unless it causes problems on slow hardwar
...

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936256452)
> The m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline)) call on [line 957](https://github.com/bitcoin/bitcoin/blob/6bf77e5a376804c0681897fa032537ba16e9a99a/src/node/interfaces.cpp#L957C1-L957C24) is what is supposed to prevent this.

> cs_main is only held briefly, after each 1 second wait, once per loop.

Thanks for explaining I missed this in first pass.
Thanks for the clarifications my concern is addressed.

I imagine still there will be redundant work in doing this each
...

💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2625576039)
> Update: it doesn't, at least not for centos: https://github.com/bitcoin/bitcoin/pull/30975/checks?check_run_id=36436390795

Thanks I don't see what else could be causing it and I can't reproduce it locally anymore. If we can add a few debug lines that might provide some insight:

```diff
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -251,8 +251,9 @@ endef

define check_or_remove_sources
mkdir -p $($(package)_source_dir); cd $($(package)_source_dir); \
- test -f $($(package)
...

🤔 ismaelsadeeq reviewed a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2585059647)
Code Review 6bf77e5a376804c0681897fa032537ba16e9a99a

Just a question and a nit

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936317810)
In "On testnet create min diff template after 20 mins" 6bf77e5a376804c0681897fa032537ba16e9a99a

nit:
@Sjors should this commit message and comments be updated to reflect the actual change something like

"miner: update `waitNext` to return a new block if 20 minutes have elapsed since the tip"

I think the above discription is exactly what this commit does and does not enable the creation of the min diff template, it's done internally by the block assembler?

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936297849)
In "Add waitNext() to BlockTemplate interface" a61fb2fd2d6b338c08c619e4605521a41bd3edd9

@Sjors IIRC block assembler has a total fees field `nFees` that also skips coinbase transaction, why aren't we using that? could expose it and use it, this will prevent the iterating through all the block transactions to compute the total fees during each tick?

👋 brunoerg's pull request is ready for review: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768)

💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2625760214)
@eriknylund and @darosior : I have reproduced the issue on CI fuzz test and made the fix. Now, it passed all checks. Please review when you are free. Thanks.

CC: @hodlinator

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2625954984)
* Introduce vsize and weight tagged versions of FeeFrac, to avoid accidental type confusion.
* Convert all of txgraph to work using FeePerWeight, rather than bare FeeFrac.

🚀 ryanofsky merged a pull request: "ci: Allow build dir on CI host"
(https://github.com/bitcoin/bitcoin/pull/31428)

👍 tdb3 approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2585444075)
ACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

Nice cleanup

💬 tdb3 commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1936523040)
nit: Not a blocker, but it seems like it would make sense for all of these types of constants to live in `p2p.py`.

💬 tdb3 commented on pull request "test: Added coverage to the waitfornewblock rpc":
(https://github.com/bitcoin/bitcoin/pull/31746#issuecomment-2626098245)
> You could change it to:
>
> ```
> if (timeout < 0) {
> throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout");
> }
> ```
>
> In which case coverage would go up, but that seems unnecessary.

Maybe my preference for multi line if-statements is biasing me, but in general this seems like it could be an argument for preferring multi line if-statements over single line ones.

The current [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-note
...

👍 tdb3 approved a pull request: "test: Added coverage to the waitfornewblock rpc"
(https://github.com/bitcoin/bitcoin/pull/31746#pullrequestreview-2585483278)
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

Thanks for adding coverage. Induced test failure by commenting out https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/rpc/blockchain.cpp#L284

👋 pablomartin4btc's pull request is ready for review: "Add new "address type" column to the "receiving tab" address book page"
(https://github.com/bitcoin-core/gui/pull/753)

💬 pablomartin4btc commented on pull request "Add new "address type" column to the "receiving tab" address book page":
(https://github.com/bitcoin-core/gui/pull/753#issuecomment-2626204992)
The [issue](https://github.com/bitcoin-core/gui/pull/753#issuecomment-2598800192) has been fixed (+ manually tested - transparent/ no impact on the use case).

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936795545)
(and these two defaults are very unlikely to change).

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936806097)
Good point. `BlockAssembler::CreateNewBlock()` uses `nFees` internally. It's updated in `AddToBlock`. I could expose it via `CBlockTemplate`.

However this code will hopefully go away soon(tm) with Cluster Mempool, and I suspect it's extremely fast. So it's perhaps not worth refactoring. In any case it could be done in a followup without changing the interface.

💬 Sjors commented on pull request "test: Added coverage to the waitfornewblock rpc":
(https://github.com/bitcoin/bitcoin/pull/31746#issuecomment-2626512279)
> it could be an argument for preferring multi line if-statements over single line ones

Maybe, but it wouldn't catch `?` branches either. You could propose a linter change that insists on multi line if-statements. Though I expect ... strong opinions :-)

👋 hebasto's pull request is ready for review: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)