💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008736607)
Extracted the BIP54-specific sigop counting into a separate function that can be moved into `consensus/tx_verify.cpp` with no modification if/when this is implemented in consensus, as suggested by @instagibbs.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008736607)
Extracted the BIP54-specific sigop counting into a separate function that can be moved into `consensus/tx_verify.cpp` with no modification if/when this is implemented in consensus, as suggested by @instagibbs.
💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169246397)
@davidgumberg I'm having doubts about this. FeeFrac takes an int32 for `size` attribute. I'm not sure it is a good idea to mix types between functions.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169246397)
@davidgumberg I'm having doubts about this. FeeFrac takes an int32 for `size` attribute. I'm not sure it is a good idea to mix types between functions.
💬 0xB10C commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008752031)
Why doesn't the description say it fixes a typo?
> * Replaced hardcoded row calculations with the `ROWS_AVAILABLE_FOR_LIST` variable for improved readability and maintainability.
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008752031)
Why doesn't the description say it fixes a typo?
> * Replaced hardcoded row calculations with the `ROWS_AVAILABLE_FOR_LIST` variable for improved readability and maintainability.
💬 sipa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169251427)
If you change `at_size` to be uint32, I don't think this check is still correct.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169251427)
If you change `at_size` to be uint32, I don't think this check is still correct.
💬 leopardracer commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008781449)
> Why doesn't the description say it fixes a typo?
>
> > * Replaced hardcoded row calculations with the `ROWS_AVAILABLE_FOR_LIST` variable for improved readability and maintainability.
@0xB10C description updated
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008781449)
> Why doesn't the description say it fixes a typo?
>
> > * Replaced hardcoded row calculations with the `ROWS_AVAILABLE_FOR_LIST` variable for improved readability and maintainability.
@0xB10C description updated
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008785284)
Although it's more readable, the downside of this approach is that we're looping over tx.vin twice and, probably much worse, calling `AccessCoin` twice.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008785284)
Although it's more readable, the downside of this approach is that we're looping over tx.vin twice and, probably much worse, calling `AccessCoin` twice.
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169298815)
Done I used `GetVirtualTransactionSize(*tx)`, check [83b6253](https://github.com/bitcoin/bitcoin/pull/32800/commits/83b6253eb3921a79231f450bb43e1977ea835b5e). Thanks!
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169298815)
Done I used `GetVirtualTransactionSize(*tx)`, check [83b6253](https://github.com/bitcoin/bitcoin/pull/32800/commits/83b6253eb3921a79231f450bb43e1977ea835b5e). Thanks!
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169301179)
Done in [83b6253](https://github.com/bitcoin/bitcoin/pull/32800/commits/83b6253eb3921a79231f450bb43e1977ea835b5e). Thanks!
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169301179)
Done in [83b6253](https://github.com/bitcoin/bitcoin/pull/32800/commits/83b6253eb3921a79231f450bb43e1977ea835b5e). Thanks!
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3008848042)
Ready for review!
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3008848042)
Ready for review!
💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169312647)
Actually I think nSize is not needed at all and the check can be deleted. If `EvaluateFeeUp` is taking an uint32 there's no need to convert `num_bytes` to other types!!
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169312647)
Actually I think nSize is not needed at all and the check can be deleted. If `EvaluateFeeUp` is taking an uint32 there's no need to convert `num_bytes` to other types!!
💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169317441)
You mean the commented Assume can be deleted?
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169317441)
You mean the commented Assume can be deleted?
💬 sipa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169326321)
No, the `if` condition is wrong now. The result can overflow.
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169326321)
No, the `if` condition is wrong now. The result can overflow.
💬 fanquake commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3008874065)
https://github.com/bitcoin/bitcoin/actions/runs/15904154552/job/44856855999?pr=32818#step:12:437:
```bash
test 2025-06-26T14:50:15.271166Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 190, in main
self.run_test()
~~~~~~~~~~~~~^^
...
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3008874065)
https://github.com/bitcoin/bitcoin/actions/runs/15904154552/job/44856855999?pr=32818#step:12:437:
```bash
test 2025-06-26T14:50:15.271166Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 190, in main
self.run_test()
~~~~~~~~~~~~~^^
...
💬 HowHsu commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008895117)
Hi folks, any hints on how to see node logs of failed CI tests: [https://github.com/bitcoin/bitcoin/pull/32791/checks?check_run_id=44855035637#:~:text=%5B11%3A09%3A03.884%5D%20%EF%BF%BD%5B0%3B36m%20test%20%202025%2D06%2D26T15%3A09%3A03.312746Z%20TestFramework%20(ERROR)%3A%20Hint%3A%20Call%20/ci_container_base/test/functional/combine_logs.py%20%27/ci_container_base/ci/scratch/test_runner/test_runner_%E2%82%BF_%F0%9F%8F%83_20250626_150635/wallet_fundrawtransaction_252%27%20to%20consolidate%20all%2
...
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008895117)
Hi folks, any hints on how to see node logs of failed CI tests: [https://github.com/bitcoin/bitcoin/pull/32791/checks?check_run_id=44855035637#:~:text=%5B11%3A09%3A03.884%5D%20%EF%BF%BD%5B0%3B36m%20test%20%202025%2D06%2D26T15%3A09%3A03.312746Z%20TestFramework%20(ERROR)%3A%20Hint%3A%20Call%20/ci_container_base/test/functional/combine_logs.py%20%27/ci_container_base/ci/scratch/test_runner/test_runner_%E2%82%BF_%F0%9F%8F%83_20250626_150635/wallet_fundrawtransaction_252%27%20to%20consolidate%20all%2
...
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008910090)
Coins are already accessed multiple times in checking an unconfirmed transaction (which should cache them anyways), doing once more shouldn't introduce any noticeable overhead and i like the upside of facilitating review of a future consensus-touching PR. But if many feel strongly i'm happy to revert the previous approach, and i'm sure Greg would be fine too as he Concept ACK'd the previous approach. Let me know!
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3008910090)
Coins are already accessed multiple times in checking an unconfirmed transaction (which should cache them anyways), doing once more shouldn't introduce any noticeable overhead and i like the upside of facilitating review of a future consensus-touching PR. But if many feel strongly i'm happy to revert the previous approach, and i'm sure Greg would be fine too as he Concept ACK'd the previous approach. Let me know!
💬 maflcko commented on pull request "[DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation)":
(https://github.com/bitcoin/bitcoin/pull/30277#issuecomment-3008913285)
> I'm surprised our existing fuzz tests do not catch this (`process_messages` might but I haven't tried), but it looks like we actually never call `Minisketch::Deserialize` with bytes straight from the fuzzer but rather only with result from `Minisketch::Serialize` (maybe to avoid the assertion? not sure):
I tried this by starting 8 fuzz processes 5 days ago. 7 still run and one of them crashed after two days. I minimized the input:
```
$ git log -1
commit bd242fd6e3f42e03a8c8abf23f9e92
...
(https://github.com/bitcoin/bitcoin/pull/30277#issuecomment-3008913285)
> I'm surprised our existing fuzz tests do not catch this (`process_messages` might but I haven't tried), but it looks like we actually never call `Minisketch::Deserialize` with bytes straight from the fuzzer but rather only with result from `Minisketch::Serialize` (maybe to avoid the assertion? not sure):
I tried this by starting 8 fuzz processes 5 days ago. 7 still run and one of them crashed after two days. I minimized the input:
```
$ git log -1
commit bd242fd6e3f42e03a8c8abf23f9e92
...
💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169364191)
Oh you're right! I think adding an `Assume(at_size <= INT32_MAX)` should be enough?
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2169364191)
Oh you're right! I think adding an `Assume(at_size <= INT32_MAX)` should be enough?
👍 hebasto approved a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32810#pullrequestreview-2962594362)
ACK fe8034b09c53f49d02b54f4e55cfe11bbd113fed. I've backported all mentioned PRs locally and got zero diff with this branch.
(https://github.com/bitcoin/bitcoin/pull/32810#pullrequestreview-2962594362)
ACK fe8034b09c53f49d02b54f4e55cfe11bbd113fed. I've backported all mentioned PRs locally and got zero diff with this branch.
💬 maflcko commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008935991)
> Hi folks, any hints on how to see node logs of failed CI tests, It's not easy to create all the environements including OS and libs and to run it locally:
You can follow the " View more details on Cirrus CI " link: https://cirrus-ci.com/task/6753181050339328
It says system error, so you are likely launching enough threads to run into a system limit.
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008935991)
> Hi folks, any hints on how to see node logs of failed CI tests, It's not easy to create all the environements including OS and libs and to run it locally:
You can follow the " View more details on Cirrus CI " link: https://cirrus-ci.com/task/6753181050339328
It says system error, so you are likely launching enough threads to run into a system limit.
💬 maflcko commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008939307)
For the CI docs, see https://github.com/bitcoin/bitcoin/tree/master/ci#ci-scripts, but you'll likely have to run that on a 64 core system to recreate the system error
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3008939307)
For the CI docs, see https://github.com/bitcoin/bitcoin/tree/master/ci#ci-scripts, but you'll likely have to run that on a 64 core system to recreate the system error