💬 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-1854708178)
Could squash this easy change into one commit?

💬 brunoerg 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-1854718328)
> Could squash this easy change into one commit?

Sure, done!

💬 shovas commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1854796784)
> Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue? Could you share the logs? (here or in a new issue is fine, just tag me when you do it).

So far I have been using USB storage (128GB SanDisk Extreme Pro USB and a 1TB ADATA external ssd) whereas I have now successfully downloaded the complete blockchain to internal nvme fine, something that failed many times when trying to use a usb device as storage. Two machines, both Windows 10.

I am running
...

💬 kashifs commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1425998172)
Perhaps. That would make it consistent with const CAmount& selection_target. It's my understanding that the documentation should exactly match the function definition. Is there another way to look at this?

💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855053056)
Just kicked off 10×10h of fuzzing, will take a look tomorrow

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

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