💬 laanwj commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1950608126)
Not sure if it's a difference in the tool or the magic files used by default, but `file` output seems noticibly different between linux and mac:

mac (`file-5.41`):
```
/bin/ls: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64
- Mach-O 64-bit executable x86_64] [arm64e:Mach-O 64-bit executable arm64e
- Mach-O 64-bit executable arm64e]
/bin/ls (for architecture x86_64): Mach-O 64-bit executable x86_64
/bin/ls (for architecture arm64e): Mach-O 64-bit ex
...

👍 hebasto approved a pull request: "build: set build type and per-build-type flags as early as possible"
(https://github.com/bitcoin/bitcoin/pull/31711#pullrequestreview-2608325691)
ACK 56a9b847bba2b8deb6a9c3f3a7eb95b4c71c2d14.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2650448405)
> > IMO, there is isn't a good reason to break compatibility here at all, if a shim that just adds one new line and one new file to the build [#31161 (comment)](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2623350369) can provide it.
>
> Thanks! Applied.

Reverted back due to the GenerateSymlinks module was not robust enough.


> In its current form, this change seems likely to cause confusion and wasted time because when upgrading or downgrading around the change, **builds
...

👍 laanwj approved a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2608355209)
Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213

💬 laanwj commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950628441)
Would it be possible to use `bitcoin_daemon_status` here somehow, instead of repeating the expression ? (just a small maintainability thing to keep the reporting and usage consistent if it changes in the future)

📝 fanquake opened a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840)
Our CMake toolchain for a Darwin cross build currently contains:
```bash
set(CMAKE_AR "/usr/bin/llvm-ar")
set(CMAKE_RANLIB "/usr/bin/llvm-ranlib")
set(CMAKE_STRIP "/usr/bin/llvm-strip")
set(CMAKE_OBJCOPY "arm64-apple-darwin-objcopy")
set(CMAKE_OBJDUMP "/usr/bin/llvm-objdump")
```

`objcopy` isn't currently used for the Darwin build (only for Linux and splitting the debug symbols), but we shouldn't be producing a toolchain file that refers to nonexistent tools.

💬 danielabrozzoni commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1950645609)
Of course! Thank you, updated, and made last_negated and categories_to_process const

💬 Sjors commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950652514)
It turns out yes, as long as you define `bitcoin_daemon_status` before `add_subdirectory(src)`. Pushing that.

💬 hebasto commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950655717)
TBH, I'd refrain from making `bitcoin_daemon_status` a "global" variable.

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2650508714)
> This PR's script is saying the harness passes the coverage test while the one I linked is showing differences in hit count.

Thanks, good catch! Fixed off-by-one

💬 Sjors commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950668773)
In that case I'm inclined to not use the variable at all.

💬 Sjors commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950674530)
Oh but `message()` can't handle that. Alright, let's go back to the previous version.

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2650541771)
> Nice. We do have something similar, although it could likely be refined more. You can see it [here](https://github.com/marcofleon/coverage/tree/master/deterministic-coverage). It should show all the places where hit count between two coverage reports differs.

Nice. Happy to pull it in here, but I think it is similar to `diff --unified`, so I used that here for now. Not sure if your script handles sub-line regions, but in this pull they can trivally be enabled by adding the `-show-line-count
...

💬 laanwj commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#discussion_r1950689515)
> In that case I'm inclined to not use the variable at all.

Ok, yes, that would have been my other proposal. But a variable seems to be needed then, let's leave it as-is.

💬 fanquake commented on pull request "[27.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/31422#issuecomment-2650590978)
> probably is not necessary for this PR?

I agree, I'm not planning on backporting that here.

🚀 fanquake merged a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/31422)

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2650680523)
That would be `DynSock::Pipe::GetBytes()` and `DynSock::Pipe::PushBytes()`. `Sock::Send()` and `Sock::Recv()` also use raw pointer + length, but they were not introduced in this PR and, more importantly, are supposed to mimic 1:1 the `send(2)` and `recv(2)` syscalls, in return value, arguments and semantic.

For `GetBytes()` and `PushBytes()` the change would be:

```diff
diff --git i/src/test/util/net.h w/src/test/util/net.h
index 3e717341d8..a28c442035 100644
--- i/src/test/util/net.h

...

🚀 ryanofsky merged a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834)

💬 hodlinator commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2650784726)
> > Got a big diff, indicating a lot of test non-determinism:
>
> I haven't looked in detail, but I presume much is due to randomness. On one hand, one could go with #31486 for the unit tests as well. On the other hand, some unit tests are fuzz tests (drawing inputs from a random seed, which is recorded), so they'd be a bit more limited if the randomness was made deterministic. But I think either way is fine.

This gives me an 8.4K line diff instead of 39k, so there is some hope:
```
RAND
...

💬 ryanofsky commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2650797960)
re: What do you think? PR? Leave it alone?

Whichever you prefer, I'd happily review it. I wanted to make suggestion more as an idea to keep in mind when updating this code or writing new code, because of benefits described in https://github.com/bitcoin/bitcoin/issues/31272

Also ideally the change would go further IMO: replace `std::vector<uint8_t>` with `std::vector<std::byte>`, replace `memcpy` with `std::vector::assign`, replace Recv/Send arguments with `span` instead of `void*`. I don't
...