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

💬 hebasto commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1855651903)
> For anyone testing with HWI, consider also testing #24313.

Here is the combined [`231130-replace-bp+pr24313`](https://github.com/hebasto/bitcoin/tree/231130-replace-bp%2Bpr24313) branch.

💬 maflcko commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-1855661692)
I wonder if this should instead be put in the GUI window and RPC help for the encrypt action, otherwise it seems easy to miss?

The GUI spreads the information over three pop-up windows, which doesn't seem great, when it can be put into just one Window.