💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237502775)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237502775)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237503063)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237503063)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237503366)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237503366)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#issuecomment-3128453950)
Thanks for the feedback @glozow and @achow101! I'm converting this to a draft while I work on some additional test cases.
(https://github.com/bitcoin/bitcoin/pull/32896#issuecomment-3128453950)
Thanks for the feedback @glozow and @achow101! I'm converting this to a draft while I work on some additional test cases.
📝 ishaanam converted_to_draft a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896)
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from
...
(https://github.com/bitcoin/bitcoin/pull/32896)
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from
...
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2237520358)
Per the BIP, it does specify `coin_type` (which is defined in BIP44). BIP44 does reserve `1` as a registered coin type for Bitcoin test networks (i.e., testnet, signet). Revisiting the language in the BIP, I'm realising this is not worded strongly enough and should be updated.
I do think we should follow the convention of deriving mainnet keys and test keys from distinct chains in our tests. This also matches what we do in other functional tests, e.g., `wallet_descriptor.py`.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2237520358)
Per the BIP, it does specify `coin_type` (which is defined in BIP44). BIP44 does reserve `1` as a registered coin type for Bitcoin test networks (i.e., testnet, signet). Revisiting the language in the BIP, I'm realising this is not worded strongly enough and should be updated.
I do think we should follow the convention of deriving mainnet keys and test keys from distinct chains in our tests. This also matches what we do in other functional tests, e.g., `wallet_descriptor.py`.
💬 Kingsleyioryoo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3128469585)
Good
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3128469585)
Good
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2237526035)
That's definitely a good idea. Some hardware wallets may even refuse to sign if the coin type derivation is wrong, e.g. to prevent a testnet app from moving mainnet coins.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2237526035)
That's definitely a good idea. Some hardware wallets may even refuse to sign if the coin type derivation is wrong, e.g. to prevent a testnet app from moving mainnet coins.
💬 achow101 commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128482538)
> At this point, it's unclear whether incorporating these OpenSSL-based cryptographic functions directly into the Bitcoin Core codebase is practical or desirable.
I don't think it is desirable. OpenSSL is a massive codebase, and we only need a tiny part. We spent a very long time trying to remove OpenSSL as well.
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128482538)
> At this point, it's unclear whether incorporating these OpenSSL-based cryptographic functions directly into the Bitcoin Core codebase is practical or desirable.
I don't think it is desirable. OpenSSL is a massive codebase, and we only need a tiny part. We spent a very long time trying to remove OpenSSL as well.
📝 pablomartin4btc opened a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082)
Remove dead code due to legacy wallet removal.
Leftovers from previous #32481.
(https://github.com/bitcoin/bitcoin/pull/33082)
Remove dead code due to legacy wallet removal.
Leftovers from previous #32481.
💬 achow101 commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3128506203)
reACK d5104cfbaeb82081e4b00a5084516555e446dcdc
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3128506203)
reACK d5104cfbaeb82081e4b00a5084516555e446dcdc
💬 w0xlt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128518901)
> I don't think it is desirable. OpenSSL is a massive codebase, and we only need a tiny part. We spent a very long time trying to remove OpenSSL as well.
What I meant was reimplementing those OpenSSL functions I mentioned above in our codebase.
> A naive implementation with no proper review could be as bad as using an external library too.
That's my point. Adding cryptographic schemes that aren't directly part of the Bitcoin protocol can be difficult to obtain sufficient review or even
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128518901)
> I don't think it is desirable. OpenSSL is a massive codebase, and we only need a tiny part. We spent a very long time trying to remove OpenSSL as well.
What I meant was reimplementing those OpenSSL functions I mentioned above in our codebase.
> A naive implementation with no proper review could be as bad as using an external library too.
That's my point. Adding cryptographic schemes that aren't directly part of the Bitcoin protocol can be difficult to obtain sufficient review or even
...
🚀 achow101 merged a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279)
(https://github.com/bitcoin/bitcoin/pull/32279)
📝 pablomartin4btc converted_to_draft a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082)
Remove dead code due to legacy wallet removal.
Leftovers from previous #32481.
(https://github.com/bitcoin/bitcoin/pull/33082)
Remove dead code due to legacy wallet removal.
Leftovers from previous #32481.
💬 fanquake commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3128659016)
Fuzzer is still failing the same as #31367, will drop that out (https://cirrus-ci.com/task/5198999654760448?logs=ci#L5231):
```bash
[13:17:44.505] AddressSanitizer:DEADLYSIGNAL
[13:17:44.505] =================================================================
[13:17:44.505] ==9965==ERROR: AddressSanitizer: stack-overflow on address 0x7ffd88ed3db8 (pc 0x7f91daffff4f bp 0x7ffd88f4fec0 sp 0x7ffd88ed2dc0 T0)
[13:17:44.505] #0 0x7f91daffff4f in std::ostreambuf_iterator<char, std::char_traits<c
...
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3128659016)
Fuzzer is still failing the same as #31367, will drop that out (https://cirrus-ci.com/task/5198999654760448?logs=ci#L5231):
```bash
[13:17:44.505] AddressSanitizer:DEADLYSIGNAL
[13:17:44.505] =================================================================
[13:17:44.505] ==9965==ERROR: AddressSanitizer: stack-overflow on address 0x7ffd88ed3db8 (pc 0x7f91daffff4f bp 0x7ffd88f4fec0 sp 0x7ffd88ed2dc0 T0)
[13:17:44.505] #0 0x7f91daffff4f in std::ostreambuf_iterator<char, std::char_traits<c
...
💬 rebroad commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-3128668906)
> In many circumstances, using lots of our CPU is a desirable property and should be rewarded, not punished - also outside of IBD. Some of the most undesirable peers are spy nodes that never send us anything. But imagine a peer that is so well-connected and fast that it offers us many new transactions and blocks earlier than our other peers - wouldn't we want to be connected to this peer, even if it takes up much more CPU than the other peers?
>
> So I think we'd have to do one of two things:
>
...
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-3128668906)
> In many circumstances, using lots of our CPU is a desirable property and should be rewarded, not punished - also outside of IBD. Some of the most undesirable peers are spy nodes that never send us anything. But imagine a peer that is so well-connected and fast that it offers us many new transactions and blocks earlier than our other peers - wouldn't we want to be connected to this peer, even if it takes up much more CPU than the other peers?
>
> So I think we'd have to do one of two things:
>
...
💬 rebroad commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-3128696488)
NACK. cpu load alone is a useless metric. However, CPU load divided by TXs accepted into blocks (or mempool) would be a useful metric, IMHO.
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-3128696488)
NACK. cpu load alone is a useless metric. However, CPU load divided by TXs accepted into blocks (or mempool) would be a useful metric, IMHO.
💬 glozow commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128795026)
ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128795026)
ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b
💬 fanquake commented on pull request "[NO MERGE]: TSAN should fail":
(https://github.com/bitcoin/bitcoin/pull/33081#issuecomment-3128827488)
```bash
[15:14:29.653] node0 2025-07-28T19:14:23.812588Z [httpworker.12] [wallet/wallet.h:945] [void wallet::CWallet::WalletLogPrintf(util::ConstevalFormatString<sizeof...(Params)>, const Params &...) const [Params = <std::string>]] [w3] Releasing wallet w3..
[15:14:29.653] test 2025-07-28T19:14:28.406783Z TestFramework (ERROR): Unexpected exception
[15:14:29.653] Traceback (most recent call last):
[15:14:29.653] Fi
...
(https://github.com/bitcoin/bitcoin/pull/33081#issuecomment-3128827488)
```bash
[15:14:29.653] node0 2025-07-28T19:14:23.812588Z [httpworker.12] [wallet/wallet.h:945] [void wallet::CWallet::WalletLogPrintf(util::ConstevalFormatString<sizeof...(Params)>, const Params &...) const [Params = <std::string>]] [w3] Releasing wallet w3..
[15:14:29.653] test 2025-07-28T19:14:28.406783Z TestFramework (ERROR): Unexpected exception
[15:14:29.653] Traceback (most recent call last):
[15:14:29.653] Fi
...
💬 glozow commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3128957421)
> MempoolRejectedTx still inserts a tx-id into RecentRejectsFilter for TX_INPUTS_NOT_STANDARD, but After this patch AlreadyHaveTx no longer consults the filter by tx-id. Is that entry now redundant, or do other call-sites still depend on it?
Good question. I think we can remove that actually, since we are no longer interested in whether a transaction's txid is in the cache 🤔
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3128957421)
> MempoolRejectedTx still inserts a tx-id into RecentRejectsFilter for TX_INPUTS_NOT_STANDARD, but After this patch AlreadyHaveTx no longer consults the filter by tx-id. Is that entry now redundant, or do other call-sites still depend on it?
Good question. I think we can remove that actually, since we are no longer interested in whether a transaction's txid is in the cache 🤔