💬 achow101 commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922296387)
The full recipients list of security@bitcoincore.org is intentionally kept private in order to reduce the risk of targeted attacks against those who receive those emails. Only the people who are willing to take that risk have their fingerprints published.
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922296387)
The full recipients list of security@bitcoincore.org is intentionally kept private in order to reduce the risk of targeted attacks against those who receive those emails. Only the people who are willing to take that risk have their fingerprints published.
💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1922316467)
> I definitely appreciate the discussions about LN usage, but it would be best to take it to the Delving thread. Here's the link > for {switching, imbuing} {commitment, HTLC-X} transactions to use {v3, ephemeral anchors}: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
no time for delving - apologies.
concern is on the lack of anti-pinning robustness of v3 policy, see my previous comments.
if correct and leave a substantial pinning loophole, i’m wo
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1922316467)
> I definitely appreciate the discussions about LN usage, but it would be best to take it to the Delving thread. Here's the link > for {switching, imbuing} {commitment, HTLC-X} transactions to use {v3, ephemeral anchors}: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
no time for delving - apologies.
concern is on the lack of anti-pinning robustness of v3 policy, see my previous comments.
if correct and leave a substantial pinning loophole, i’m wo
...
💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399)
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense!
The problem was the removal of this from `crypto/common.h`:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```
Turns out, `sha256.cpp` was relying on that. Adding that include to `sha256.cpp` (where it belongs) fixes my local benchmarks.
I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after t
...
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399)
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense!
The problem was the removal of this from `crypto/common.h`:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```
Turns out, `sha256.cpp` was relying on that. Adding that include to `sha256.cpp` (where it belongs) fixes my local benchmarks.
I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after t
...
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1857527717)
Addressed feedback by @sipa and @sr-gi.
I also improved the clone skipping by skipping any UTXOs with same effective value and heavier weight that follow an unselected UTXO.
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1857527717)
Addressed feedback by @sipa and @sr-gi.
I also improved the clone skipping by skipping any UTXOs with same effective value and heavier weight that follow an unselected UTXO.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475100291)
Changed as follows:
```diff
- // find any solutions. Redirect to exploring Omission branch of the penultimate selected UTXO (i.e. deselect
- // last selected UTXO, then SHIFT)
+ // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e.
+ // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs)
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475100291)
Changed as follows:
```diff
- // find any solutions. Redirect to exploring Omission branch of the penultimate selected UTXO (i.e. deselect
- // last selected UTXO, then SHIFT)
+ // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e.
+ // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs)
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475094275)
Moved `result.SetSelectionsEvaluated(curr_try)` after the loop
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475094275)
Moved `result.SetSelectionsEvaluated(curr_try)` after the loop
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475155882)
Instead of comparing fees, I’m comparing weights here now. I’ll also add a commit to skip larger weights after tie-breaking the sort order with the weight.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475155882)
Instead of comparing fees, I’m comparing weights here now. I’ll also add a commit to skip larger weights after tie-breaking the sort order with the weight.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475095394)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475095394)
Fixed
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183856)
Done
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183856)
Done
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475146550)
I had to make the following change to ensure that there is a positive effective value:
```diff
- const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, MAX_MONEY - max_spendable)};
+ // Ensure that each UTXO has at least an effective value of 1 sat
+ const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475146550)
I had to make the following change to ensure that there is a positive effective value:
```diff
- const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, MAX_MONEY - max_spendable)};
+ // Ensure that each UTXO has at least an effective value of 1 sat
+ const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183696)
Yep, moved declaration down there.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183696)
Yep, moved declaration down there.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475198825)
Sure, replaced with:
```diff
- size_t i = utxo_pool.size();
- while (i > 0) {
- --i;
+ for (int i = utxo_pool.size() - 1 ; i >= 0; --i) {
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475198825)
Sure, replaced with:
```diff
- size_t i = utxo_pool.size();
- while (i > 0) {
- --i;
+ for (int i = utxo_pool.size() - 1 ; i >= 0; --i) {
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475202681)
Absolutely. Fixed.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475202681)
Absolutely. Fixed.
💬 1440000bytes commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922340140)
In case someone is not comfortable notifying all the unknown members of security@bitcoincore.org email list, an alternative could be to directly email one or more developers mentioned here: https://github.com/bitcoin/bitcoin/security
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922340140)
In case someone is not comfortable notifying all the unknown members of security@bitcoincore.org email list, an alternative could be to directly email one or more developers mentioned here: https://github.com/bitcoin/bitcoin/security
📝 0xBEEFCAF3 converted_to_draft a pull request: "Reenable OP_CAT"
(https://github.com/bitcoin/bitcoin/pull/29247)
Renabling OP_CAT, as per the [draft BIP](https://github.com/bitcoin/bips/pull/1525)
This MR involves reinstating the legacy opcode by replacing OP_SUCCESS126. Currently, there are no proposed activation parameters.
### Relevant Links
[Btcd implementation](https://github.com/btcsuite/btcd/pull/2095)
[Signet MR](https://github.com/bitcoin-inquisition/bitcoin/pull/39)
[Mailing list post](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-October/022049.html)
(https://github.com/bitcoin/bitcoin/pull/29247)
Renabling OP_CAT, as per the [draft BIP](https://github.com/bitcoin/bips/pull/1525)
This MR involves reinstating the legacy opcode by replacing OP_SUCCESS126. Currently, there are no proposed activation parameters.
### Relevant Links
[Btcd implementation](https://github.com/btcsuite/btcd/pull/2095)
[Signet MR](https://github.com/bitcoin-inquisition/bitcoin/pull/39)
[Mailing list post](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-October/022049.html)
💬 0xBEEFCAF3 commented on pull request "Reenable OP_CAT":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-1922343699)
Drafting until I get the [inquisition PR](https://github.com/bitcoin-inquisition/bitcoin/pull/39) approved and I can get the builds passing.
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-1922343699)
Drafting until I get the [inquisition PR](https://github.com/bitcoin-inquisition/bitcoin/pull/39) approved and I can get the builds passing.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475234088)
I don't remember exactly what the bug was, but I know that it had something to do with assuming that all transactions that we call `transactionRemovedFromMempool` on are `TxStateInMempool`. That being said, I will change it back to the way it was before because it is not causing any problems right now.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475234088)
I don't remember exactly what the bug was, but I know that it had something to do with assuming that all transactions that we call `transactionRemovedFromMempool` on are `TxStateInMempool`. That being said, I will change it back to the way it was before because it is not causing any problems right now.
📝 theuni converted_to_draft a pull request: "serialization: c++20 endian/byteswap/clz modernization"
(https://github.com/bitcoin/bitcoin/pull/29263)
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking
...
(https://github.com/bitcoin/bitcoin/pull/29263)
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking
...
💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922418119)
Converted to a draft while I'm still messing with this.
I added a new commit to add the `bitcoin-config.h` include where it's necessary. I'm not done investigating that yet, but it's at least needed for `chacha20poly1305.cpp` as well.
The benchmarks still show some regressions, though it's not nearly as bad as it was before. Will continue poking.
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922418119)
Converted to a draft while I'm still messing with this.
I added a new commit to add the `bitcoin-config.h` include where it's necessary. I'm not done investigating that yet, but it's at least needed for `chacha20poly1305.cpp` as well.
The benchmarks still show some regressions, though it's not nearly as bad as it was before. Will continue poking.
📝 achow101 opened a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367)
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.
(https://github.com/bitcoin/bitcoin/pull/29367)
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.