💬 VzxPLnHqr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-2625486456)
I am not familiar with the intricacies of this PR, but is it possible for bitcoind to detect this impending crash before it happens and just return some sort of 500 error?

I ran into this issue (using v28.0) when running a straightforward script which was calling `getblock` for each block, starting from genesis. After a couple thousand blocks bitcoind crashed with the file descriptor error.

After some trial and error I have found that putting a 100 millisecond delay between http requests
...

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

I think we are not just creating a block template after ticks but on each iteration of this loop, which continues infinitely in the default case. Since we are holding `cs_main` continuously, wouldn't this cause resource contention and delay other components of the node?

My concern is that, regardless of whether the mempool improves, we keep creating block templates.

Currently there is no reliable wa
...

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

We already have the comments about the default in node types `BlockWaitOptions` docstring, writing it here is duplicating same information, when we update the default we have to update both places.

I think it will be better to just move the fee threshold description comment to node types `BlockWaitOptions` docstring and delete the duplicate here.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1936242611)
I tried to make it a quick TL&DR so someone can understand the most important behaviour, without having to go and read the entire struct doc (which might grow, and not all code let you hover and see the full thing in a popup).

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

> In "Add waitNext() to BlockTemplate interface" [a61fb2f](https://github.com/bitcoin/bitcoin/commit/a61fb2fd2d6b338c08c619e4605521a41bd3edd9)
>
> I think we are not just creating a block template after ticks but on each iteration of this loop, which continues infinitely in the default case. Since we are holding `cs_main` continuously, wouldn't this cause resource contention and delay other components of the node?


...

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