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

💬 epiccurious commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922597240)
utACK c340503b67585b631909584f1acaa327f5af25d3

💬 epiccurious commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1922599794)
Tested ACK. Passed the functional test runner.

💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1922687448)
I'm working on it. Plan is to harden the function to not read at end and after. I have some initial small draft changes, and want to add testing. Maybe, we can even highlight the issue via additional fuzz+msan tests.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475452360)
This could be done in a follow-up if people want it, because currently the conflicting transactions are stored in the `CWalletTx`, and not the transaction state, so this function is not aware of the txids of the mempool conflicts.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453805)
Done

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453968)
Done

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454363)
Thanks, that should be fixed now