💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1440150511)
Done

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440237517)
For reference, the error is https://github.com/bitcoin/bitcoin/actions/runs/7387978338/job/20097842474?pr=29165#step:7:2150

```
/usr/local/include/event2/event.h:838:10: error: parameter 'tv' not found in the function declaration [-Werror,-Wdocumentation]
@param tv the amount of time after which the loop should terminate,
^~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
make[2]: *** [bitcoin_cli-bitcoin-cli.o] Error 1
make[1]: ***
...

👋 maflcko's pull request is ready for review: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165)

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1440249695)
why is this moved to the header?

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440250938)
Right, the `event2/event.h` is no longer considered as a system header, which allows a compiler to emit warnings from it, even configured with the `--enable-suppress-external-warnings` option .

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440252735)
We could pass `-nostdinc` to the compiler and specify include ditrectories explicitly. However, such a solution looks a bit ugly to me.

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440253535)
IIUC this should affect everyone on macOS today already on current master, using clang-15, so a separate issue and bugfix can be filed?

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1875107325)
Fixed pull request description and CI, which should be green now

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440273658)
> The underlying issue is that Apple Clang 15 does not support -Xclang -internal-isystem anymore.

I'd be very suprised if this was the case, given it's a core Clang flag? Even more suprised if they'd for some reason only removed it for x86_64?

💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1875122594)
Thanks for your answers @petertodd.

I think we may be talking a bit past each other in some of those comments, because most of this is too vague and ignores important low-level details.
I'll try to highlight the most important high-level points below.

> So if Alice wants to broadcast a commitment transaction, her version of the fee variants all take the funds for the fees from her balance, and Bob has given Alice a set of signatures for each fee variant. Conversely, if Bob wants to broadc
...

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440277787)
> > The underlying issue is that Apple Clang 15 does not support -Xclang -internal-isystem anymore.
>
> I'd be very suprised if this was the case, given it's a core Clang flag? Even more suprised if they'd for some reason only removed it for x86_64?

https://github.com/bitcoin/bitcoin/actions/runs/7395607754/job/20119092833:
```
checking whether C++ preprocessor accepts -Xclang -internal-isystem/usr/local/include... no
```

💬 hebasto commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1875136650)
```
$ env HOSTS=x86_64-apple-darwin ./contrib/guix/guix-build
...
make: Entering directory '/bitcoin/depends'
Extracting native_libtapi...
/bitcoin/depends/sources/eb33a59f2e30ff9724dc1ea8bee8b5229b0557c9.tar.gz: OK
Preprocessing native_libtapi...
patching file build.sh
Configuring native_libtapi...
Building native_libtapi...
-- The C compiler identification is Clang 17.0.6
-- The CXX compiler identification is Clang 17.0.6
-- The ASM compiler identification is Clang with GNU-like c
...

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440288473)
Pretty sure it's still there? i.e: https://github.com/apple/llvm-project/blob/94624bdf65b8ad61f9bf3948a5bf387c6fa0a967/clang/lib/Driver/ToolChain.cpp#L1138 ?

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694)
Does our `-latomic` check no-longer work, because that should be adding this if needed? According to the docs this should no-longer be needed with clang-15?
> Clang prior to version 15, when building for 32-bit,
> and linking against libstdc++, requires linking with
> -latomic if using the C++ atomic library.

We should probably either fixup the check, or fixup the docs, or both.

💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1440301216)
Afaict oss-fuzz already uses a ram disk for its environments: https://github.com/google/clusterfuzz/blob/c461a961d8fb2afe47fb4af5eee3d1434a324a40/docker/base/setup_clusterfuzz.sh#L38 (i.e. `/tmp` is mounted in ram).

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1875164441)
> This kind of thinking is precisely the problem: you've listed a bunch of potential problems, without any discussion of how actual protocols are impacted by those problems.

Peter, please read the [RBF improvements mailing list thread](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html) to get up to date on users' problems with RBF.

Yes, v3 makes no sense if you think RBF is perfect today. I agree that we cannot have a productive discussion about how to solve
...

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1430067474)
Nit: `s/best block not the same/best block are not the same/`

🤔 TheCharlatan reviewed a pull request: "indexes: Stop using node internal types and locking cs_main, improve sync logic"
(https://github.com/bitcoin/bitcoin/pull/24230#pullrequestreview-1786770579)
Approach ACK

This change looks good, albeit its daunting length. The only thing I am still not sure about is the commit "indexes: Initialize indexes without holding cs_main" c8fde60faad36d454cdd1300184a5169499f9e90 and if there is some observable improvement. Maybe this commit could be left for another PR? Or does it achieve something that is strictly needed for this PR?

The amount of churn in the commits is a bit unfortunate, but I don't have suggestions on how to improve this. Some of th
...

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1435025414)
Nit: In commit 8d0cc07587d4acfab3a8b4f636b631467c8e5f19 why is this assertion not moved as well to `blockConnected`? Similarly the assertion in [here](https://github.com/bitcoin/bitcoin/pull/24230/commits/8d0cc07587d4acfab3a8b4f636b631467c8e5f19#diff-5251d93616c116c364d2d502ad2a59e901a3512194462fabacc9756f96fd8dddR394) in `IgnoreChainstateFlushed`.

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1438864128)
Nit: In commit 8a164cf087c480e68e9ec87a97f759b97015efea: I feel like this would be a bit more readable as a `CBlockIndex&`.