💬 willcl-ark commented on issue "Feature request: bitcoin-wallet tool: don't modify files unless requested":
(https://github.com/bitcoin/bitcoin/issues/15608#issuecomment-3008635500)
Opened #32818 to address this.
(https://github.com/bitcoin/bitcoin/issues/15608#issuecomment-3008635500)
Opened #32818 to address this.
💬 leopardracer commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008638712)
> This just fixes a typo in AVALIABLE -> AVAILABLE. I don't think this is good use of reviewer time. Functionally is not affected.
@0xB10C totally get that this seems minor, but I think even small things like typos matter, especially in a repo like Bitcoin. It's such a high-profile project that sets the tone for the whole ecosystem
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008638712)
> This just fixes a typo in AVALIABLE -> AVAILABLE. I don't think this is good use of reviewer time. Functionally is not affected.
@0xB10C totally get that this seems minor, but I think even small things like typos matter, especially in a repo like Bitcoin. It's such a high-profile project that sets the tone for the whole ecosystem
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169183653)
Q1. Yeah there's no comment about that in [#27591](https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1376980882), Am still skeptical about conditionally reporting the `sigosize` , the initial idea's to check when txSize which is the transaction in the mempool (which might be sigops adjusted from `entry->GetTxSize()`) is less than vSize `GetVirtualTransactionSize(txSize, 0, 0)` which is not sigop adjusted. But I think it will make more sense to remove the codition and report the` sigopsi
...
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169183653)
Q1. Yeah there's no comment about that in [#27591](https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1376980882), Am still skeptical about conditionally reporting the `sigosize` , the initial idea's to check when txSize which is the transaction in the mempool (which might be sigops adjusted from `entry->GetTxSize()`) is less than vSize `GetVirtualTransactionSize(txSize, 0, 0)` which is not sigop adjusted. But I think it will make more sense to remove the codition and report the` sigopsi
...
💬 Sjors commented on pull request "[29.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/32810#issuecomment-3008658529)
Tested on M4 macOS 15.5 that with 5987c1b6abaefad61d8d2cca605349354432398a having qt5 and qt6 (`qt@5` and `qt` via Homebrew) leads to the issues described in #31009, but with fe8034b09c53f49d02b54f4e55cfe11bbd113fed it's happy.
Will test on Intel later.
(https://github.com/bitcoin/bitcoin/pull/32810#issuecomment-3008658529)
Tested on M4 macOS 15.5 that with 5987c1b6abaefad61d8d2cca605349354432398a having qt5 and qt6 (`qt@5` and `qt` via Homebrew) leads to the issues described in #31009, but with fe8034b09c53f49d02b54f4e55cfe11bbd113fed it's happy.
Will test on Intel later.
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169188242)
Yeah, I don't think it's necessary that's why I decide to report it (sigopsize) only in `getrawtransaction` RPC
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169188242)
Yeah, I don't think it's necessary that's why I decide to report it (sigopsize) only in `getrawtransaction` RPC
💬 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!