🤔 ryanofsky reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1523139721)
Updated 09938a41d904c05b4676b064da9baa85e53e3e6f -> 3e6b6877645f978b4999efdfb9e616f4dbf9dd19 ([`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3) -> [`pr/stopafter.4`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.3..pr/stopafter.4)) just fixing a spelling mistake that was pointed out
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1523139721)
Updated 09938a41d904c05b4676b064da9baa85e53e3e6f -> 3e6b6877645f978b4999efdfb9e616f4dbf9dd19 ([`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3) -> [`pr/stopafter.4`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.3..pr/stopafter.4)) just fixing a spelling mistake that was pointed out
💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259001998)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258600239
> I don't see in which scenario `blockTip()` would return `kernel::Interrupted` here, but the docs seem to indicate that it could. I'd suggest to either update the docs to reflect that we never expect this to interrupt, or add a logging statement similar to what's done in `ThreadImport`?
I'm happy to update the comment, but it would help to have a specific suggestion, because to me this is only saying that a kernel::I
...
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259001998)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258600239
> I don't see in which scenario `blockTip()` would return `kernel::Interrupted` here, but the docs seem to indicate that it could. I'd suggest to either update the docs to reflect that we never expect this to interrupt, or add a logging statement similar to what's done in `ThreadImport`?
I'm happy to update the comment, but it would help to have a specific suggestion, because to me this is only saying that a kernel::I
...
🤔 ismaelsadeeq reviewed a pull request: "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/28027#pullrequestreview-1523177321)
Running this test failed on my device not sure if it's from this PR though.
```
2023-07-11T00:04:00.468000Z TestFramework (INFO): PRNG seed is: 1124432296273551722
2023-07-11T00:04:00.468000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_banbrcm1
2023-07-11T00:04:00.472000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 550, in start_n
...
(https://github.com/bitcoin/bitcoin/pull/28027#pullrequestreview-1523177321)
Running this test failed on my device not sure if it's from this PR though.
```
2023-07-11T00:04:00.468000Z TestFramework (INFO): PRNG seed is: 1124432296273551722
2023-07-11T00:04:00.468000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_banbrcm1
2023-07-11T00:04:00.472000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 550, in start_n
...
💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259010439)
The v19 test pubkeys `0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52` and `037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073` are they relevant to the test? I see they are the same as the one reported in #18075
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259010439)
The v19 test pubkeys `0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52` and `037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073` are they relevant to the test? I see they are the same as the one reported in #18075
💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259015544)
Why are we no longer using the wallet?
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259015544)
Why are we no longer using the wallet?
💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259013655)
`180000`, `210000`, e.t.c
To reduce the magic numbers we can create a variable for the versions, e.g `v18 = 180000`and reuse it throughout the tests?
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259013655)
`180000`, `210000`, e.t.c
To reduce the magic numbers we can create a variable for the versions, e.g `v18 = 180000`and reuse it throughout the tests?
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259064139)
So `m_recent_rejects_reconsiderable` logs transaction on its wtxid (which is good to avoid witness inflation changing the result) however isn’t that two restrictive if you have PKG1 submitted with A+B, A is too low with B and then you have A+B’ where A is good enough ? Or is that `TX_LOW_FEE` when the transaction isn’t good enough to meet “mempool min fee not meet” / “min relay fee not met” ?
If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s
...
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259064139)
So `m_recent_rejects_reconsiderable` logs transaction on its wtxid (which is good to avoid witness inflation changing the result) however isn’t that two restrictive if you have PKG1 submitted with A+B, A is too low with B and then you have A+B’ where A is good enough ? Or is that `TX_LOW_FEE` when the transaction isn’t good enough to meet “mempool min fee not meet” / “min relay fee not met” ?
If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s
...
✅ fanquake closed a pull request: "Create generator-generic-ossf-slsa3-publish.yml"
(https://github.com/bitcoin-core/gui/pull/746)
(https://github.com/bitcoin-core/gui/pull/746)
📝 fanquake locked a pull request: "Create generator-generic-ossf-slsa3-publish.yml"
(https://github.com/bitcoin-core/gui/pull/746)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that impr
...
(https://github.com/bitcoin-core/gui/pull/746)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that impr
...
💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1630123482)
I think you'll still need to rebase or fix the compile error
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1630123482)
I think you'll still need to rebase or fix the compile error
💬 MarcoFalke commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630141468)
See https://github.com/bitcoin/bitcoin/issues/17779 for further steps
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630141468)
See https://github.com/bitcoin/bitcoin/issues/17779 for further steps
✅ MarcoFalke closed an issue: "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin"
(https://github.com/bitcoin/bitcoin/issues/28049)
(https://github.com/bitcoin/bitcoin/issues/28049)
💬 dsldsl commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630168865)
that's helpful thank you
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630168865)
that's helpful thank you
⚠️ meglio opened an issue: "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)"
(https://github.com/bitcoin/bitcoin/issues/28064)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)
when calling through rpc api, throws an error:
```
Internal bug detected: "isValid == error_msg.empty()"
rpc/output_script.cpp:64 (operator())
Bitcoin Core v25.0.0
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
```
### Expected behaviour
Address validation to return is_v
...
(https://github.com/bitcoin/bitcoin/issues/28064)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)
when calling through rpc api, throws an error:
```
Internal bug detected: "isValid == error_msg.empty()"
rpc/output_script.cpp:64 (operator())
Bitcoin Core v25.0.0
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
```
### Expected behaviour
Address validation to return is_v
...
💬 MarcoFalke commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630193718)
This will be fixed in 25.1, see 725c3dc2dd1402d631e05ef461fe2abbac9a008f
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630193718)
This will be fixed in 25.1, see 725c3dc2dd1402d631e05ef461fe2abbac9a008f
💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259265158)
agree, you can resolve this
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259265158)
agree, you can resolve this
💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1630248097)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1630248097)
Approach ACK
💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259300260)
In theory the waste score should be the same.
Because not all of the params are available within BnB we call
`result.ComputeAndSetWaste(cost_of_change, cost_of_change, 0);`
instead of
`result.ComputeAndSetWaste(min_viable_change, cost_of_change, change_fee);`
I believe this should be fine as BnB will not produce results with more than `cost_of_change` excess and therefore `GetChange()` will always return 0 and then `change_fee` doesn't matter.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259300260)
In theory the waste score should be the same.
Because not all of the params are available within BnB we call
`result.ComputeAndSetWaste(cost_of_change, cost_of_change, 0);`
instead of
`result.ComputeAndSetWaste(min_viable_change, cost_of_change, change_fee);`
I believe this should be fine as BnB will not produce results with more than `cost_of_change` excess and therefore `GetChange()` will always return 0 and then `change_fee` doesn't matter.
💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259306993)
Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that `GetSelectionWaste` doesn't use `SelectionResult.GetEffectiveValue()` which already accounts for bump fee discount.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259306993)
Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that `GetSelectionWaste` doesn't use `SelectionResult.GetEffectiveValue()` which already accounts for bump fee discount.
💬 fanquake commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630335753)
Closing, given the fix has been backported. @meglio if you're able to compile the 25.x branch, and confirm this fixes your issue, please do.
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630335753)
Closing, given the fix has been backported. @meglio if you're able to compile the 25.x branch, and confirm this fixes your issue, please do.