💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340212685)
> Sure. This branch clearly demonstrates false positive results for bitcoind and bitcoin-qt:

Thanks. I built this branch and inspected `bitcoind`, and it contains calls to fortified functions. i.e:
```bash
objdump -D /root/bitcoin/guix-build-423fc912bca9/output/x86_64-linux-gnu/bitcoin-423fc912bca9/bin/bitcoind
<snip>
6f9afe: e8 cd dd 95 ff call 578d0 <__vsnprintf_chk@plt>
6fd0a1: e8 ba b1 95 ff call 58260 <__fprintf_chk@plt>
6fe135: e8 66 a0 95 ff call
...

💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340221792)
> So how is this a false positive?

This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1751658436)
Got the error again, trying to create a godbolt reproducer to understand the apparent pointer arithmetic misunderstanding

<details>
<summary>Error</summary>

```
In file included from /ci_container_base/src/script/script.h:11,
from /ci_container_base/src/primitives/transaction.h:11,
from /ci_container_base/src/primitives/block.h:9,
from /ci_container_base/src/kernel/chainparams.h:11,
from /ci_container_base/src/kernel
...

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340225004)
> This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.

Yes, the check as implemented, which checks for (any) usage of foritfied function calls in the binary, passes, because the binary contains calls to fortified functions.

💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751663364)
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.

How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?

:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30848)

👍 dergoegge approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292038673)
Code review ACK 1243d2ffa30ebc8a148623e3cdabd61f70714932

CI timeout is unrelated

👍 fanquake approved a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791#pullrequestreview-2292069963)
ACK 2d68c3b1c2e4f8fb881efc3569506d426ee5155d

🚀 fanquake merged a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791)

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2340343047)
Addressed comments by @instagibbs. Thanks for the review!

👍 dergoegge approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292103593)
reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751765403)
Thanks for the review!

> is usually not a good idea

Can you please expand on that? I don't mind splitting, as you've recommended, but I have to understand what the current drawbacks are.

🤔 BrandonOdiwuor reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2292156787)
Concept ACK

👋 brunoerg's pull request is ready for review: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)

💬 fanquake commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2340408539)
Yea. Seems like it is also not a difference between PRs and merge commits, given that now the current merge run is slow, and a simultaneously running PR is fast:
ASAN (most recent merge commit): [` Total Test time (real) = 1086.64 sec`](https://github.com/bitcoin/bitcoin/actions/runs/10790934929/job/29927228108#step:7:4710).
ASAN (current PR run): [` Total Test time (real) = 510.99 sec`](https://github.com/bitcoin/bitcoin/actions/runs/10790805641/job/29926810997?pr=30433#step:7:4710).

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1751778401)
Just changed it to use `LegacyDataSPKM`. Note that this target is about the spkm migration only, not the whole process.

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2340420173)
It also doesn't look cmake related, because the latest 28.x build also times out: https://github.com/bitcoin/bitcoin/runs/29755145359

📝 hebasto opened a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861)
This PR makes it possible to reuse the `ccache` cache populated during a build in another build directory:
```
$ cmake -B build_1 -DWITH_CCACHE=ON
$ cmake --build build_1 -t bitcoind
$ cmake -B build_2 -DWITH_CCACHE=ON
$ cmake --build build_2 -t bitcoind # 100% cache hit rate is expected.
```

The first commit has been picked from https://github.com/bitcoin/bitcoin/pull/30803.

Please refer to https://github.com/bitcoin/bitcoin/pull/20353 for historical context.

Addresses this [com
...

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2340474631)
> However, ccache does _not_ hit across two different build dirs, compiling the same commit.

Fixed in https://github.com/bitcoin/bitcoin/pull/30861.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2340474919)
ACK faa32adbcf4c04f0a426eaba4a43b29a293de72b

<details>
<summary>Details</summary>

* brace initialization in base.cpp
* pos*t*itional typo fix
* added ConstevalFormatString overload

</details>

📝 hebasto converted_to_draft a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)
Closes https://github.com/bitcoin/bitcoin/issues/30799:

```
$ ./build/src/bitcoind -logsourcelocations -asmap=/tmp/no_file 2>&1 | head -1
2024-09-04T07:01:03Z [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v28.99.0-b94fe85fbe2f (release build)
```