💬 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
💬 Sammie05 commented on pull request "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)":
(https://github.com/bitcoin/bitcoin/pull/32924#issuecomment-3066510113)
Thanks for adding coverage for this very specific edge case.
Using a real testnet transaction is a solid way to ensure the test reflects actual chain data.
Also appreciate the clear comments explaining why this minimal 8-byte DER signature can appear and why it matters.
(https://github.com/bitcoin/bitcoin/pull/32924#issuecomment-3066510113)
Thanks for adding coverage for this very specific edge case.
Using a real testnet transaction is a solid way to ensure the test reflects actual chain data.
Also appreciate the clear comments explaining why this minimal 8-byte DER signature can appear and why it matters.
📝 Sammie05 opened a pull request: "doc: clarify note about backup after migratewallet"
(https://github.com/bitcoin/bitcoin/pull/32956)
Added a clarifying note that after migration, the original legacy wallet file remains unchanged on disk as a backup. This helps users understand that migration is non-destructive.
(https://github.com/bitcoin/bitcoin/pull/32956)
Added a clarifying note that after migration, the original legacy wallet file remains unchanged on disk as a backup. This helps users understand that migration is non-destructive.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2203147913)
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 and
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2203147913)
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 and
...
💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3066701981)
So far I found:
- `python-scikit-build-core` builds fine
- `python-pydantic-2` fails
- `python-pydantic-core` fails
My current suspicion is that `Python-2.7.18` / `python2-minimal` is the culprit, based on what was being built around the time it fails.
However I'm unable to produce the failure if I just insert `python-2.7` somewhere in the manifest. Part of the problem is that I don't really understand how these manifest files work.
But if my suspicion is current, then it might be a matter o
...
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3066701981)
So far I found:
- `python-scikit-build-core` builds fine
- `python-pydantic-2` fails
- `python-pydantic-core` fails
My current suspicion is that `Python-2.7.18` / `python2-minimal` is the culprit, based on what was being built around the time it fails.
However I'm unable to produce the failure if I just insert `python-2.7` somewhere in the manifest. Part of the problem is that I don't really understand how these manifest files work.
But if my suspicion is current, then it might be a matter o
...