💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1855476801)
Before @sipa optimization:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,401,785.00 | 713.38 | 1.6% | 0.02 | `ShouldFanoutTo`
```

After:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 369,620.50 | 2,705.48
...

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426458856)
The warning tells us that the code is _potentially_ unsafe. Not that it has a certain bug now. It is similar to having a variable declared with `GUARDED_BY()`, then accessing it without the mutex in such a way that it is safe.

> Maybe declaring the fs::path& path const, and writing to m_cached_blocks_path directly could make the warning go away?

Would that be like tricking the compiler, going around the warning without actually removing the underlying potential problem?

📝 maflcko opened a pull request: "build: Require libc++-16 or later"
(https://github.com/bitcoin/bitcoin/pull/29077)
I am not familiar with macOS, but given that https://github.com/bitcoin/bitcoin/pull/28622 prescribes a minimum of libc++-16 for macOS, doing the same on Linux could make sense?

This is needed for stuff like `std::ranges::equal`, see https://github.com/bitcoin/bitcoin/pull/29071

I don't think anyone is using `-stdlib=libc++` on Linux, except for the CI and OSS-Fuzz.

The minimum required clang version remains at `clang++-13`.

💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855524913)
> Interesting that the guix macOS build passed, given that it is on clang-15, no?

I'd guess this is because our macOS SDK (libc++) is from Xcode 15, which is based on LLVM ~16, and [includes support for ranges](https://developer.apple.com/xcode/cpp/#c++20). So as long as clang-15 can compile it, which it should given vanilla LLVM supports the use of ranges (experimentally) in 15, it probably works.

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-1855525331)
Draft for now to gather conceptual feedback first.

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489158)
Rewrote the docs in this file, I think it's much more clear now

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489285)
Added the inheritance check, but the second half is redundant with when we check the child, no?

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489422)
Added, thanks

💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855569550)
Oh, I guess clang-15 can use libc++-16, but not libstdc++, according to https://godbolt.org/z/dbKWeE8Te and https://github.com/llvm/llvm-project/commit/babdef27c503c0bbbcc017e9f88affddda90ea4e ?

💬 maflcko commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1855571815)
lgtm. Could squash the simple test fix into one commit?

💬 dergoegge commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1855575173)
Fuzzer now crashes on:

```
$ echo "HwoKCQoKCgoKCgoKCgkKCgoKvQoLCgoKCgkKCgoKvQoLCgoKCgoJCgoKCgpBCgoKCgpBDg==" | base64 --decode > minisketch.crash
$ FUZZ=minisketch ./src/test/fuzz/fuzz minisketch.crash
test/fuzz/minisketch.cpp:73 minisketch_fuzz_target: Assertion `dec.size() == num_diff' failed.
```

I think this stems from the fact that the minisketch implementation now might differ between the sketches in the test.

💬 fanquake commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426530173)
Given https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855569550 and https://godbolt.org/z/n4oTE9sTE the `libstdc++-10` requirement isn't going to work here? Looks like this really needs to be clang-16.

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426531861)
CI is green, though? :sweat_smile:

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426534962)
I understand your point.
`TrimToSize` docstring says
```cpp
/** Remove transactions from the mempool until its dynamic size is <= sizelimit.
```
Now `TrimToSize`removes transaction even if the mempool is not full if their are 0 fee transactions.
the docstring should be updated to reflect the operation and maybe the name?

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426535623)
So as long as you don't need deferred concepts instantiation, it should be fine.

📝 maflcko opened a pull request: "build: Bump guix time-machine to unlock riscv64 metal"
(https://github.com/bitcoin/bitcoin/pull/29078)
Closes https://github.com/bitcoin/bitcoin/issues/29020

💬 maflcko commented on pull request "build: Bump guix time-machine to unlock riscv64 metal":
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1855611758)
Is there an easy way to print all affected/changed/bumped dependencies in the Bitcoin Core build graph?

💬 hebasto commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1855630481)
Rebased and implemented @Sjors's [suggestions](https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1854029824).

Thank you @Sjors for testing!

🤔 Sjors reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1781522971)
059d847810051b52d33b5b16711a031c3fd199a9 looks good, except that you need to call `SetActiveHDKey()` in `CWallet::EncryptWallet` (to avoid needlessly running the upgrade code).

> Note that key rotation on encryption can happen only once in a wallet's lifetime - when the wallet is encrypted the first. We do not do key rotation when the passphrase changes.

TIL you can't turn off encryption once it's turned on (`walletpassphrasechange` does not allow `""` as the password, unlike `createwallet
...

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1426540509)
ce5495a4b27fe9646c9933dc39f0cb4e8510eaff: I still think it's nicer to check

```cpp
// Upgrade the wallet to have a global HD key, or in the special case
// where the user downgraded and then encrypted / decrypted.
if(!pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY) || enc_status != IsCrypted()) {
```

here rather than inside `UpgradeToGlobalHDKey`, so that it's clear to people reading this code when they need to open `wallet.cpp` and look at the function body.

Alternatively, you
...