💬 l0rinc commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3065413387)
Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly.
I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3065413387)
Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly.
I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-3065475639)
Redid the rebase, the conflicts seem to have been solved correctly.
Changes since last ACK:
waitNext, GetMinimumTime,DeriveTarget,blockToJSON,blockheaderToJSON,GetTarget,CreateDescriptor were also less-wrong-const-ed™.
<details>
<summary>Details</summary>
```patch
diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 150295e5b7..f8ae9a261f 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -71,7 +71,7 @@ public:
* On testnet this will addi
...
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-3065475639)
Redid the rebase, the conflicts seem to have been solved correctly.
Changes since last ACK:
waitNext, GetMinimumTime,DeriveTarget,blockToJSON,blockheaderToJSON,GetTarget,CreateDescriptor were also less-wrong-const-ed™.
<details>
<summary>Details</summary>
```patch
diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 150295e5b7..f8ae9a261f 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -71,7 +71,7 @@ public:
* On testnet this will addi
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202682814)
I just noticed a subtle bug in the test that I fixed. Let's see if anyone else notices :nerd_face:
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202682814)
I just noticed a subtle bug in the test that I fixed. Let's see if anyone else notices :nerd_face:
⚠️ anhilde opened an issue: "Bitcoin Core v29.0 incorrectly enters IBD mode when only ~600 blocks behind, preventing normal sync"
(https://github.com/bitcoin/bitcoin/issues/32955)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi, I have a full node on a development machine that is not always run. It runs a self compiled none modified v29.0 node as systemd service on an Ubuntu 24.04 system. I had it twice now that the node, after not using it for a few days would go into IBD mode. This time I tried rebuilding the chain state but it would only go as far as the same block it was stuck on before. By running `bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32955)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi, I have a full node on a development machine that is not always run. It runs a self compiled none modified v29.0 node as systemd service on an Ubuntu 24.04 system. I had it twice now that the node, after not using it for a few days would go into IBD mode. This time I tried rebuilding the chain state but it would only go as far as the same block it was stuck on before. By running `bitcoi
...
💬 yuvicc commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3065605849)
Concept ACK
> The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library
I agree with you, was having a similar thought on the internal library `libbitcoin_consensus` despite removal of external `libbitcoinconsensus` library. If we go this way then we can remove the `libbitcoin_consensus` from the `libraries.md` as well.
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3065605849)
Concept ACK
> The external consensus library was also removed while waiting for #29015. I wonder if we should remove the internal consensus library as well and drop its contents into the new internal kernel library
I agree with you, was having a similar thought on the internal library `libbitcoin_consensus` despite removal of external `libbitcoinconsensus` library. If we go this way then we can remove the `libbitcoin_consensus` from the `libraries.md` as well.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3065659959)
> as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit.
This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3065659959)
> as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit.
This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202762467)
I see. So `cost_of_change` is variable based on the change output size which is range bound `(0, MAX_SCRIPT_SIZE)` since the cost of creating the change output depends on the size of the change output created. I didn't know there was a max script size for an output!?
I think this type of thing might be worth adding in more detail to the commit message to help reviewers.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202762467)
I see. So `cost_of_change` is variable based on the change output size which is range bound `(0, MAX_SCRIPT_SIZE)` since the cost of creating the change output depends on the size of the change output created. I didn't know there was a max script size for an output!?
I think this type of thing might be worth adding in more detail to the commit message to help reviewers.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202763864)
Good call, I just noticed that as well.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202763864)
Good call, I just noticed that as well.
🤔 Prabhat1308 reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3013295386)
Concept ACK
I think removing is a better option. The only **weak** argument that we could have for not removing is that `BLOCK_FAILED_VALID` right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3013295386)
Concept ACK
I think removing is a better option. The only **weak** argument that we could have for not removing is that `BLOCK_FAILED_VALID` right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3065733833)
Thank you for the detailed review!
I think you're right, my current approach regarding named parameter handling is too verbose/complex, and I looked at your implementation, and I really liked the way you're handling named parameters in the `RPCConvertNamedValues` function. I will implement that approach in my next update commit. Besides that, I also looked at the json parsing issue you're mentioning, and it's easily reproducible. I experimented with both the approaches, the one you provided
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3065733833)
Thank you for the detailed review!
I think you're right, my current approach regarding named parameter handling is too verbose/complex, and I looked at your implementation, and I really liked the way you're handling named parameters in the `RPCConvertNamedValues` function. I will implement that approach in my next update commit. Besides that, I also looked at the json parsing issue you're mentioning, and it's easily reproducible. I experimented with both the approaches, the one you provided
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202897142)
I see. Thanks. This will be a slightly non-uniform pattern since the window is largest for the first selection `(1, MAX_MONEY - 16)` and then the next selection will have a smaller range maybe `(1, MAX_MONEY / 2)`. So the pool will tend to have the largest UTXO's first.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202897142)
I see. Thanks. This will be a slightly non-uniform pattern since the window is largest for the first selection `(1, MAX_MONEY - 16)` and then the next selection will have a smaller range maybe `(1, MAX_MONEY / 2)`. So the pool will tend to have the largest UTXO's first.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202900053)
Yes, that follows from my previous comment. I note that this works however the distribution will not be truly random since the chance of selecting a large UTXO is greatest the first time and so on.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202900053)
Yes, that follows from my previous comment. I note that this works however the distribution will not be truly random since the chance of selecting a large UTXO is greatest the first time and so on.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202900649)
Maybe randomize the pool once it's created so there is equal likely-hood of having a large UTXO at the back of the list.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202900649)
Maybe randomize the pool once it's created so there is equal likely-hood of having a large UTXO at the back of the list.
💬 sipa commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202902938)
scriptPubKeys of size above 10000 are consensus-unspendable.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202902938)
scriptPubKeys of size above 10000 are consensus-unspendable.
💬 tnndbtc commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3066264494)
@maflcko For "stdin is missing", I was referring to a suspicion that bitcoind failed to write an empty string to the pipe and caused signer.py waiting on `buffer = sys.stdin.read()`. However, I was wrong about that.
After I checked the call stack posted in https://github.com/bitcoin/bitcoin/issues/32524 , it indicated that bitcoind got stuck on a `read()` call from err_rd_pipe in `util::read_atmost_n`, which is to read from the child process, i.e., signer.py.
Also, the bitcoind process indeed
...
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3066264494)
@maflcko For "stdin is missing", I was referring to a suspicion that bitcoind failed to write an empty string to the pipe and caused signer.py waiting on `buffer = sys.stdin.read()`. However, I was wrong about that.
After I checked the call stack posted in https://github.com/bitcoin/bitcoin/issues/32524 , it indicated that bitcoind got stuck on a `read()` call from err_rd_pipe in `util::read_atmost_n`, which is to read from the child process, i.e., signer.py.
Also, the bitcoind process indeed
...
💬 Sammie05 commented on pull request "doc: mention key removal in rpc interface modification":
(https://github.com/bitcoin/bitcoin/pull/32867#issuecomment-3066331603)
I checked this branch locally and saw that it removes -Werror=dev from the CMake generation step.
Just to understand better: is this to prevent CI from failing on dev warnings? Looks reasonable to me, but curious if it’s meant to be temporary or permanent.
(https://github.com/bitcoin/bitcoin/pull/32867#issuecomment-3066331603)
I checked this branch locally and saw that it removes -Werror=dev from the CMake generation step.
Just to understand better: is this to prevent CI from failing on dev warnings? Looks reasonable to me, but curious if it’s meant to be temporary or permanent.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2203049928)
> In contrast to the existing fuzz test, this fuzz target generates inputs that have solutions and don’t have solution. It then asserts that when the brute force search indicates that a solution exists
On second thought, I don't see the value in a test that tests for results that "don't have solutions", The valuable test is testing the quality of any produced solution. Therefore, I'm not sure what the value is to using a brute force method to show that no solution is produced.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2203049928)
> In contrast to the existing fuzz test, this fuzz target generates inputs that have solutions and don’t have solution. It then asserts that when the brute force search indicates that a solution exists
On second thought, I don't see the value in a test that tests for results that "don't have solutions", The valuable test is testing the quality of any produced solution. Therefore, I'm not sure what the value is to using a brute force method to show that no solution is produced.
💬 Sammie05 commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#issuecomment-3066363528)
Checked this branch locally. the added note clarifies what happens if the legacy wallet only contains watch-only scripts.
It helps make migration behavior clearer for users. it actually Looks good to me!
Also noticed the LLM linter suggested rewording ‘which the wallet knows but is not watching…’ maybe worth considering for clarity.
Thanks for adding this
(https://github.com/bitcoin/bitcoin/pull/32866#issuecomment-3066363528)
Checked this branch locally. the added note clarifies what happens if the legacy wallet only contains watch-only scripts.
It helps make migration behavior clearer for users. it actually Looks good to me!
Also noticed the LLM linter suggested rewording ‘which the wallet knows but is not watching…’ maybe worth considering for clarity.
Thanks for adding this
💬 Sammie05 commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3066420791)
Thanks for adding these assertions!
I like how you explicitly test that coinbase witnesses don’t contribute to SigOp cost.
Makes it clearer for future maintainers.
LGTM 👍
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3066420791)
Thanks for adding these assertions!
I like how you explicitly test that coinbase witnesses don’t contribute to SigOp cost.
Makes it clearer for future maintainers.
LGTM 👍
💬 Sammie05 commented on pull request "test: add logging to mock external signers":
(https://github.com/bitcoin/bitcoin/pull/32928#issuecomment-3066460410)
Nice refactor! Moving the mock helpers and log formatter to utils makes things clearer and easier to reuse.
And also logging directly into test_framework.log feels like a practical way to debug signer issues that stdout/stderr can’t catch.
Thanks for making this more maintainable
(https://github.com/bitcoin/bitcoin/pull/32928#issuecomment-3066460410)
Nice refactor! Moving the mock helpers and log formatter to utils makes things clearer and easier to reuse.
And also logging directly into test_framework.log feels like a practical way to debug signer issues that stdout/stderr can’t catch.
Thanks for making this more maintainable