💬 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
...

🤔 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.

💬 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)
```

💬 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

💬 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.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(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

💬 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())};
```

💬 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.

💬 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) {
```

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(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

📝 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)

💬 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.

💬 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.

📝 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
...

💬 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.

📝 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.

💬 epiccurious commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922494863)
+1 to @1440000bytes, they can contact the listed people directly instead of `security@`.

🤔 mzumsande reviewed a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1857950026)
ACK c340503b67585b631909584f1acaa327f5af25d3

I ran the functional test suite both with and without `--v2transport` on this branch and both runs succeeded.