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

👍 hodlinator approved a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2608732876)
re-ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1

Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2599809742):
- Broke out initial commit which tests prior behavior (with minor improvements).
- Terser test for last occurrence of negated arg suggested by self.

💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650860541)
> > I want to check if this helps [#30852 (comment)](https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430) when used with #31545 by not having to keep the base images around anymore.
>
> @0xB10C Are you waiting for me to rebase this once more? I've held back, because there is already one review, but I am happy to rebase.

Thanks for the reminder. I played around with this a bit and it does indeed help:

When using https://github.com/bitcoin/bitcoin/pull/31545, the base
...

📝 maflcko opened a pull request: "test: Clear scheduler after init to increase fuzz stability"
(https://github.com/bitcoin/bitcoin/pull/31841)
Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:

* Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
* Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.

Fix one such issue.
...

💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650867558)
My understanding is that https://github.com/bitcoin/bitcoin/pull/28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2650868943)
I've created https://github.com/bitcoin/bitcoin/pull/31841 to make it easier to test this on a real patch.

💬 maflcko commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650891965)
> My understanding is that #28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?

I do not know if they were ever needed to be set inside the CI pod, given that they fall back to the default anyway (see pull description). This may have just been added out of caution, as on the outside the pod, they are required sometimes to be set, see the docs:

https://github.com/bitcoin/bitcoin/blame/86528937e5c4da2e12c46085fc41e87ed75925
...