💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1418924286)
6d7163d8e8127a18a299eaa1309bf7d613713d51: maybe assert that either `key` or `crypted_key` is provided
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1418924286)
6d7163d8e8127a18a299eaa1309bf7d613713d51: maybe assert that either `key` or `crypted_key` is provided
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1418893185)
e491e5c4e6eb2f9b9ec53f4c2df602a755178726: I thought about adding an additional warning here for the scenario where a user downgraded, encrypted and upgraded. However, for that we need to know if the descriptor is active, which this function (`LoadDescriptorWalletRecords`) doesn't know. The caller `LoadWallet` won't know it until `LoadActiveSPKMs`. So it would involve passing a bunch of information around.
Anyway, in principle we could count the number of active descriptors for which `pubkey =
...
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1418893185)
e491e5c4e6eb2f9b9ec53f4c2df602a755178726: I thought about adding an additional warning here for the scenario where a user downgraded, encrypted and upgraded. However, for that we need to know if the descriptor is active, which this function (`LoadDescriptorWalletRecords`) doesn't know. The caller `LoadWallet` won't know it until `LoadActiveSPKMs`. So it would involve passing a bunch of information around.
Anyway, in principle we could count the number of active descriptors for which `pubkey =
...
💬 maflcko commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418994026)
nit: This is already mentioned above, no? `- Ensure the "Needs release note" label is removed from all relevant pull requests and issues.`
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418994026)
nit: This is already mentioned above, no? `- Ensure the "Needs release note" label is removed from all relevant pull requests and issues.`
💬 fanquake commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418997032)
Ah, I guess it might be more useful in the section about writing the release notes.
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418997032)
Ah, I guess it might be more useful in the section about writing the release notes.
💬 fanquake commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418999090)
I'll just remove my addition. I guess the onus is on PR authors to make sure they write release notes for their changes.
(https://github.com/bitcoin/bitcoin/pull/29023#discussion_r1418999090)
I'll just remove my addition. I guess the onus is on PR authors to make sure they write release notes for their changes.
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419002221)
I think @josibake is making a good point here. Algorithm and waste score by itself would probably still make it difficult to learn something about what went wrong (although they would be useful if the issue filer also provided the corresponding transaction). It might be better to also log the parameters for the coin selection attempt:
- feerate
- sffo (true/false)
- preset inputs (true/false)
- resulting input count (per type?)
- change created (true/false)
- algo that lead to solution
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419002221)
I think @josibake is making a good point here. Algorithm and waste score by itself would probably still make it difficult to learn something about what went wrong (although they would be useful if the issue filer also provided the corresponding transaction). It might be better to also log the parameters for the coin selection attempt:
- feerate
- sffo (true/false)
- preset inputs (true/false)
- resulting input count (per type?)
- change created (true/false)
- algo that lead to solution
...
💬 fanquake commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845386299)
I've pinged a few people about writing up notes. I'll incorporate them here.
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845386299)
I've pinged a few people about writing up notes. I'll incorporate them here.
✅ FacehuggerCZ closed an issue: "Obscure problem with sent transactions and wallet balance"
(https://github.com/bitcoin/bitcoin/issues/29024)
(https://github.com/bitcoin/bitcoin/issues/29024)
💬 FacehuggerCZ commented on issue "Obscure problem with sent transactions and wallet balance":
(https://github.com/bitcoin/bitcoin/issues/29024#issuecomment-1845388697)
Thanks for the replies, I just wanted to make sure that I didn't happen to have my bitcoin core hacked...
(https://github.com/bitcoin/bitcoin/issues/29024#issuecomment-1845388697)
Thanks for the replies, I just wanted to make sure that I didn't happen to have my bitcoin core hacked...
💬 willcl-ark commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845402519)
Here's one for #28507:
```md
Notable changes
===============
Contrib
-------
- Bash completion files have been renamed from `bitcoin*.bash-completion` to
`bitcoin*.bash`. This means completions can be automatically loaded on demand
based on invoked commands' names when they are put into the completion
directory (found with `pkg-config --variable=completionsdir
bash-completion`) without requiring renaming.
```
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845402519)
Here's one for #28507:
```md
Notable changes
===============
Contrib
-------
- Bash completion files have been renamed from `bitcoin*.bash-completion` to
`bitcoin*.bash`. This means completions can be automatically loaded on demand
based on invoked commands' names when they are put into the completion
directory (found with `pkg-config --variable=completionsdir
bash-completion`) without requiring renaming.
```
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419054827)
> it would be better to log information related to what options the user selected, such as logging that SFFO was selected (I did a quick grep and didn't see that this was being logged, but might have missed it). In general, I think it's always better to log the parameters that were used to start a process, rather than log the resulting algorithm internals.
That's not correct.
The wallet stores its own state linked to the inner node's state. Logging users' RPC command inputs only provides li
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419054827)
> it would be better to log information related to what options the user selected, such as logging that SFFO was selected (I did a quick grep and didn't see that this was being logged, but might have missed it). In general, I think it's always better to log the parameters that were used to start a process, rather than log the resulting algorithm internals.
That's not correct.
The wallet stores its own state linked to the inner node's state. Logging users' RPC command inputs only provides li
...
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419057454)
ea7a61ca94cc0d59cfe25307cd60fcda2aedec5f nit, keeping it closer to 80 characters wide, and drops "Any other candidates will be added as HD keys that have been rotated out.":
```
/**
* UpgradeToGlobalHDKey searches through the descriptors in a descriptor wallet
* and tries to find a CExtPubKey which is likely to be the most recent one used
* to generate all of the automatically generated descriptors. The automatically
* generated descriptors are guaranteed to be 2 p2pkh, 2 p2sh-segwit
...
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419057454)
ea7a61ca94cc0d59cfe25307cd60fcda2aedec5f nit, keeping it closer to 80 characters wide, and drops "Any other candidates will be added as HD keys that have been rotated out.":
```
/**
* UpgradeToGlobalHDKey searches through the descriptors in a descriptor wallet
* and tries to find a CExtPubKey which is likely to be the most recent one used
* to generate all of the automatically generated descriptors. The automatically
* generated descriptors are guaranteed to be 2 p2pkh, 2 p2sh-segwit
...
💬 fanquake commented on pull request "contrib/bash-completions: use package naming conventions":
(https://github.com/bitcoin/bitcoin/pull/28507#issuecomment-1845466985)
Release note to be added in #29023.
(https://github.com/bitcoin/bitcoin/pull/28507#issuecomment-1845466985)
Release note to be added in #29023.
💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1845469595)
To be clear, OP_Return outputs are not added to the UTXO set. That's why we standardized OP_Return in the first place.
On December 7, 2023 2:00:19 PM GMT+01:00, "Léo Haf" ***@***.***> wrote:
>The problem is that they use a diverted means to include data instead of using `OP_RETURN` which has been more or less adapted for this, in particular by not bloating the UTXOs set, that they have an unfair fee reduction compared to `OP_RETURN` and that they unbalance the incentives of miners.
>
>Mine
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1845469595)
To be clear, OP_Return outputs are not added to the UTXO set. That's why we standardized OP_Return in the first place.
On December 7, 2023 2:00:19 PM GMT+01:00, "Léo Haf" ***@***.***> wrote:
>The problem is that they use a diverted means to include data instead of using `OP_RETURN` which has been more or less adapted for this, in particular by not bloating the UTXOs set, that they have an unfair fee reduction compared to `OP_RETURN` and that they unbalance the incentives of miners.
>
>Mine
...
💬 fanquake commented on pull request "descriptors: disallow hybrid public keys":
(https://github.com/bitcoin/bitcoin/pull/28587#issuecomment-1845470214)
Release note to be added in #29023.
(https://github.com/bitcoin/bitcoin/pull/28587#issuecomment-1845470214)
Release note to be added in #29023.
💬 darosior commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845473884)
#28587 rules out usage of hybrid public keys in output descriptors. Hybrid public keys are an [exotic public key encoding](https://bitcoin.stackexchange.com/a/57865/101498) not supported by output descriptors (as specified in BIP380 and documented in doc/descriptors.md). Bitcoin Core would previously incorrectly accept descriptors containing such hybrid keys. This PR fixed that.
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845473884)
#28587 rules out usage of hybrid public keys in output descriptors. Hybrid public keys are an [exotic public key encoding](https://bitcoin.stackexchange.com/a/57865/101498) not supported by output descriptors (as specified in BIP380 and documented in doc/descriptors.md). Bitcoin Core would previously incorrectly accept descriptors containing such hybrid keys. This PR fixed that.
💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1845475729)
> > Next move?
>
> Standardize the new spam method.
I assume you meant "make the new spam method non-standard". That's exactly the whack-a-mole game I'd like to avoid in this part of the code base. Bitcoin Core release cycles are also much too slow to keep with this.
The current situation is very different from when `OP_RETURN` with 80 bytes was standardized. At the time that was a way to _discourage_ the alternative approach of bare multisig, by creating an incentive to use this slight
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1845475729)
> > Next move?
>
> Standardize the new spam method.
I assume you meant "make the new spam method non-standard". That's exactly the whack-a-mole game I'd like to avoid in this part of the code base. Bitcoin Core release cycles are also much too slow to keep with this.
The current situation is very different from when `OP_RETURN` with 80 bytes was standardized. At the time that was a way to _discourage_ the alternative approach of bare multisig, by creating an incentive to use this slight
...
💬 maflcko commented on pull request "depends: Allow PATH with spaces in directory names.":
(https://github.com/bitcoin/bitcoin/pull/28733#issuecomment-1845485680)
Marked as up-for-grabs. Should be trivial to fixup and bring over the finish line.
(https://github.com/bitcoin/bitcoin/pull/28733#issuecomment-1845485680)
Marked as up-for-grabs. Should be trivial to fixup and bring over the finish line.
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419095997)
> What you are suggesting is ok for a post-mortem comment.
I think you're misunderstanding my point. I am saying in the context of this PR (which is a bug fix), I don't think the log line you are adding here adds value and that a better thing to add in this PR would be a check that BnB is never returning change, which either errors or logs a warning.
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419095997)
> What you are suggesting is ok for a post-mortem comment.
I think you're misunderstanding my point. I am saying in the context of this PR (which is a bug fix), I don't think the log line you are adding here adds value and that a better thing to add in this PR would be a check that BnB is never returning change, which either errors or logs a warning.
💬 stickies-v commented on pull request "mempool: Don't sort in entryAll":
(https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845497136)
> Current call sites of entryAll do not require the entries to be sorted
Will this PR not cause issues [here](https://github.com/TheCharlatan/bitcoin/blob/87fa444a04854a5d3a9ff283a8b6c34587e8430f/src/wallet/wallet.cpp#L1938)? In [`CWallet::transactionAddedToMempool`](https://github.com/TheCharlatan/bitcoin/blob/87fa444a04854a5d3a9ff283a8b6c34587e8430f/src/wallet/wallet.cpp#L1406), we eventually call `CWallet::AddToWalletIfInvolvingMe` where we have a [dependency](https://github.com/TheCharlat
...
(https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845497136)
> Current call sites of entryAll do not require the entries to be sorted
Will this PR not cause issues [here](https://github.com/TheCharlatan/bitcoin/blob/87fa444a04854a5d3a9ff283a8b6c34587e8430f/src/wallet/wallet.cpp#L1938)? In [`CWallet::transactionAddedToMempool`](https://github.com/TheCharlatan/bitcoin/blob/87fa444a04854a5d3a9ff283a8b6c34587e8430f/src/wallet/wallet.cpp#L1406), we eventually call `CWallet::AddToWalletIfInvolvingMe` where we have a [dependency](https://github.com/TheCharlat
...