💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855419653)
GHA fail can be ignored:

```
==> Pouring python@3.11--3.11.6_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
rm '/usr/local/bin/2to3'

To force the link and overwrite all conflicting files:
brew link --overwrite python@3.11

To list all files that would be deleted:
brew link --overwrite --d
...

💬 maflcko commented on pull request "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target":
(https://github.com/bitcoin/bitcoin/pull/29076#issuecomment-1855420108)
review ACK e03d6f7ed534f423f58236866f8e83beee1871e1

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426435808)
Yeah... slightly annoying that we can't check this until we're sure a trim happened.

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426447476)
> confusing to have a u8string method that doesn't actually return a u8string

My concern exactly!

💬 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