💬 glozow commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1936129171)
> I meant that aborting due to missing inputs should be fast compared to full validation (signature checks), which will only be done once,
Ah ok same page 👍 was wondering if you were thinking of some other kind of caching
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1936129171)
> I meant that aborting due to missing inputs should be fast compared to full validation (signature checks), which will only be done once,
Ah ok same page 👍 was wondering if you were thinking of some other kind of caching
📝 brunoerg opened a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768)
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
(https://github.com/bitcoin/bitcoin/pull/31768)
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1936154607)
Yes, this makes it include the directory containing the binaries in the codesigning tarball.
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1936154607)
Yes, this makes it include the directory containing the binaries in the codesigning tarball.
📝 brunoerg converted_to_draft a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768)
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
(https://github.com/bitcoin/bitcoin/pull/31768)
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1936224461)
This was caught by running ci fuzz test in docker container:
# cd /ci_container_base/ci/scratch/build-aarch64-unknown-linux-gnu/test/fuzz
# ./test_runner.py -j8 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_corpora/ miniscript_smart --empty_min_time=60
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1936224461)
This was caught by running ci fuzz test in docker container:
# cd /ci_container_base/ci/scratch/build-aarch64-unknown-linux-gnu/test/fuzz
# ./test_runner.py -j8 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_corpora/ miniscript_smart --empty_min_time=60
💬 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
...
(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
...
(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.
(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).
(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?
...
(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
...
(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
...
(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)
...
(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
(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?
(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?
(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)
(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
(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.
(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)
(https://github.com/bitcoin/bitcoin/pull/31428)