💬 zhaocaipeng commented on issue "is a getBlockTemplate transactions's bug?":
(https://github.com/bitcoin/bitcoin/issues/29166#issuecomment-1874774863)
> If you have a general question, consider asking on [stack exchange](https://bitcoin.stackexchange.com/).
>
> If this is a bug report, what was the JSON result returned from `getblocktemplate` when this happened?
I'm not entirely sure if this is a bug. Is the sum of sigops in the transaction details returned by getBlockTemplate not allowed to exceed 2000?
(https://github.com/bitcoin/bitcoin/issues/29166#issuecomment-1874774863)
> If you have a general question, consider asking on [stack exchange](https://bitcoin.stackexchange.com/).
>
> If this is a bug report, what was the JSON result returned from `getblocktemplate` when this happened?
I'm not entirely sure if this is a bug. Is the sum of sigops in the transaction details returned by getBlockTemplate not allowed to exceed 2000?
✅ zhaocaipeng closed an issue: "is a getBlockTemplate transactions's bug?"
(https://github.com/bitcoin/bitcoin/issues/29166)
(https://github.com/bitcoin/bitcoin/issues/29166)
💬 aureleoules commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1440077502)
Seems like a hazard
```suggestion
} else if (!is_trusted && txo.GetWalletTx().InMempool()) {
```
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1440077502)
Seems like a hazard
```suggestion
} else if (!is_trusted && txo.GetWalletTx().InMempool()) {
```
💬 aureleoules commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1440086373)
nit: Could use `contains` (and other places too)
```suggestion
if (!tx_safe_cache.contains(outpoint.hash)) {
```
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1440086373)
nit: Could use `contains` (and other places too)
```suggestion
if (!tx_safe_cache.contains(outpoint.hash)) {
```
💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1440150443)
Done.
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1440150443)
Done.
💬 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
(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]: ***
...
(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)
(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?
(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 .
(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.
(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?
(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
(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?
(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
...
(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
```
(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
...
(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 ?
(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.
(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).
(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).