💬 fjahr commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2571340870)
ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
Reviewed and verified that the test works.
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2571340870)
ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
Reviewed and verified that the test works.
🤔 furszy reviewed a pull request: "descriptor: check whitespace in `ParsePubkeyInner`"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2530760911)
As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
Will either need a descriptor upgrade procedure or a compatibility + warning window.
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2530760911)
As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
Will either need a descriptor upgrade procedure or a compatibility + warning window.
💬 hugohn commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571350247)
@starius Kind of. We build on top of the descriptor template defined in [BIP 129 (BSMS)](https://github.com/bitcoin/bips/blob/master/bip-0129.mediawiki#user-content-Descriptor_Template). The above snippet is part of a larger BSMS wallet configuration file, which includes derivation path restrictions.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571350247)
@starius Kind of. We build on top of the descriptor template defined in [BIP 129 (BSMS)](https://github.com/bitcoin/bips/blob/master/bip-0129.mediawiki#user-content-Descriptor_Template). The above snippet is part of a larger BSMS wallet configuration file, which includes derivation path restrictions.
👍 TheCharlatan approved a pull request: "ci: Run functional tests in msan task"
(https://github.com/bitcoin/bitcoin/pull/31592#pullrequestreview-2530766486)
ACK fa0411ee305fe04800c54d141fbbeac342eaa764
(https://github.com/bitcoin/bitcoin/pull/31592#pullrequestreview-2530766486)
ACK fa0411ee305fe04800c54d141fbbeac342eaa764
💬 mzumsande commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2571386445)
Why? While we are currently relying on the `short` python `repr()` algorithm for floats, this has been stable and reliable since it's been introduced in python 3.1 as far as I know - so I don't really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been bugs related to this in the past?
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2571386445)
Why? While we are currently relying on the `short` python `repr()` algorithm for floats, this has been stable and reliable since it's been introduced in python 3.1 as far as I know - so I don't really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been bugs related to this in the past?
📝 epysqyli opened a pull request: "test: fix typo in mempool_ephemeral_dust"
(https://github.com/bitcoin/bitcoin/pull/31604)
The `test_node_restart` test in `test/functional/mempool_ephemeral_dust.py` has a repetition in the comment.
(https://github.com/bitcoin/bitcoin/pull/31604)
The `test_node_restart` test in `test/functional/mempool_ephemeral_dust.py` has a repetition in the comment.
💬 starius commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571430256)
@hugohn I built bitcoin core using this PR rebased on master.
I tried the descriptor from your message, replacing `/**/` with `/0/*` and `/1/*`. It works!
```
getdescriptorinfo "tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G1aVkND5Rzf37uhwhs8o2Lvq22iRpWwcbNGCrYxAozQfYQYi1eduES/0/*,[7f15646b/87'/0'/0']xpub6ChFTmSdBrhN3D16Rna7hJVQe4w56Gx83U4uNhT3oJaEXiPv7LKnY2gXi3FbbusCb145c3SMEUsSLMRdkxa82MNKqkatnK5b77BXPc3aK8h/0/*),{{{pk(musig([15d62cdf/87'/0'/0']xpu
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571430256)
@hugohn I built bitcoin core using this PR rebased on master.
I tried the descriptor from your message, replacing `/**/` with `/0/*` and `/1/*`. It works!
```
getdescriptorinfo "tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G1aVkND5Rzf37uhwhs8o2Lvq22iRpWwcbNGCrYxAozQfYQYi1eduES/0/*,[7f15646b/87'/0'/0']xpub6ChFTmSdBrhN3D16Rna7hJVQe4w56Gx83U4uNhT3oJaEXiPv7LKnY2gXi3FbbusCb145c3SMEUsSLMRdkxa82MNKqkatnK5b77BXPc3aK8h/0/*),{{{pk(musig([15d62cdf/87'/0'/0']xpu
...
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571431685)
> @starius Kind of. We build on top of the descriptor template defined in [BIP 129 (BSMS)](https://github.com/bitcoin/bips/blob/master/bip-0129.mediawiki#user-content-Descriptor_Template). The above snippet is part of a larger BSMS wallet configuration file, which includes derivation path restrictions.
@hugohn: FYI [BIP-388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) generalizes descriptor templates to arbitrary wallets, including with miniscript and musig; it should be e
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571431685)
> @starius Kind of. We build on top of the descriptor template defined in [BIP 129 (BSMS)](https://github.com/bitcoin/bips/blob/master/bip-0129.mediawiki#user-content-Descriptor_Template). The above snippet is part of a larger BSMS wallet configuration file, which includes derivation path restrictions.
@hugohn: FYI [BIP-388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) generalizes descriptor templates to arbitrary wallets, including with miniscript and musig; it should be e
...
💬 starius commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571497025)
I attempted to test this on Signet with a 2-of-2 MuSig2 Taproot address (without script leaves).
Succeeded using `walletprocesspsbt`, but failed when using GUI "Load PSBT from keyboard" option.
**Setup:**
- **Node 1**: Watch-only, connected to the network.
- **Node 2**: Offline, holds the first private key.
- **Node 3**: Offline, holds the second private key.
**Steps to Reproduce:**
1. Imported descriptors for each node:
```
node 2 (first private key):
importdescrip
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2571497025)
I attempted to test this on Signet with a 2-of-2 MuSig2 Taproot address (without script leaves).
Succeeded using `walletprocesspsbt`, but failed when using GUI "Load PSBT from keyboard" option.
**Setup:**
- **Node 1**: Watch-only, connected to the network.
- **Node 2**: Offline, holds the first private key.
- **Node 3**: Offline, holds the second private key.
**Steps to Reproduce:**
1. Imported descriptors for each node:
```
node 2 (first private key):
importdescrip
...
💬 1440000bytes commented on issue "Fake bitcoin core website at the top of duckduckgo":
(https://github.com/bitcoin/bitcoin/issues/31602#issuecomment-2571545965)
> I haven't actually visited the site but I am assuming it is offering malware downloads to the unsuspecting. Is there anything that can be done about this?
Yes, downloads look malicious because I see different checksum for v27.0
You can report the search results: https://duckduckgo.com/duckduckgo-help-pages/company/contact-us
(https://github.com/bitcoin/bitcoin/issues/31602#issuecomment-2571545965)
> I haven't actually visited the site but I am assuming it is offering malware downloads to the unsuspecting. Is there anything that can be done about this?
Yes, downloads look malicious because I see different checksum for v27.0
You can report the search results: https://duckduckgo.com/duckduckgo-help-pages/company/contact-us
💬 TheCharlatan commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2571595908)
> Additionally this PR adjusts the timewarp test to better illustrate the griefing attack discussed here
I think adding the example to the functional test is good, but it is not clear from the description what the griefing attack is that it seeks to prevent.
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2571595908)
> Additionally this PR adjusts the timewarp test to better illustrate the griefing attack discussed here
I think adding the example to the functional test is good, but it is not clear from the description what the griefing attack is that it seeks to prevent.
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2571604696)
Putting in draft while I fix failing CI
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2571604696)
Putting in draft while I fix failing CI
📝 Eunovo converted_to_draft a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)
This PR implements a fix for the issue described in https://github.com/bitcoin/bitcoin/issues/29435.
The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here, augments the mempool transaction `REPLACED` signal with the double-spending transaction which the wallet stores and watches for in Block notifications. A map is added to the wallet to track conflicting tx ids and their child transactions. The entry is erased wh
...
(https://github.com/bitcoin/bitcoin/pull/29680)
This PR implements a fix for the issue described in https://github.com/bitcoin/bitcoin/issues/29435.
The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here, augments the mempool transaction `REPLACED` signal with the double-spending transaction which the wallet stores and watches for in Block notifications. A map is added to the wallet to track conflicting tx ids and their child transactions. The entry is erased wh
...
🤔 TheCharlatan reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2530880952)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2530880952)
Approach ACK
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903255292)
Typo nit: `s/for coinbase/for the coinbase/`
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903255292)
Typo nit: `s/for coinbase/for the coinbase/`
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903257389)
Nit: I don't think adding these comments after an include is useful. They can become stale easily and it adds to the general inconsistencies of the codebase. Can you remove it again?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903257389)
Nit: I don't think adding these comments after an include is useful. They can become stale easily and it adds to the general inconsistencies of the codebase. Can you remove it again?
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903271412)
Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight? Could more exact weight accounting where the total weight is checked with `assert_equal` instead of `assert_greater_than_or_equal` in `verify_block_template` be useful?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903271412)
Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight? Could more exact weight accounting where the total weight is checked with `assert_equal` instead of `assert_greater_than_or_equal` in `verify_block_template` be useful?
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903265703)
Nit: Since you are creating a constant for the normal feerate, maybe create one for the fee rate of the large transactions too?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903265703)
Nit: Since you are creating a constant for the normal feerate, maybe create one for the fee rate of the large transactions too?
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903276616)
Does this have an effect on the log line in `miner.cpp::158`? Maybe clarify there that the numbers are given without the coinbase?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903276616)
Does this have an effect on the log line in `miner.cpp::158`? Maybe clarify there that the numbers are given without the coinbase?
💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301)
Nit: This is a bit confusing, I think something like `(LARGE_TXS_COUNT - 1) + NORMAL_TXS_COUNT` would be more accurate, or just give the number like you do in the next test block.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301)
Nit: This is a bit confusing, I think something like `(LARGE_TXS_COUNT - 1) + NORMAL_TXS_COUNT` would be more accurate, or just give the number like you do in the next test block.