💬 vasild commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1648588599)
Good catch! The implicit cast to `bool` of `CThreadInterrupt` makes it easy to get such mistakes if the variable is a pointer to `CThreadInterrupt` or `std::optional<CThreadInterrupt>`. If `CThreadInterrupt` had an explicit method like `IsInterrupted()` then it would be more clear to call that one, but unfortunately it does not have such a method.

To be explicit we can do `if (m_interrupt->operator bool()) {` :laughing: :robot: :rofl:

Or, more seriously, because `m_interrupt` can never be
...

💬 maflcko commented on pull request "refactor: remove extraneous lock annotations from function definitions":
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2182247885)
ACK 5729dbbb7424d02c5e5bc4f2eb340fdc1c0100b4 🦋

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 5729dbbb7424d02c5e5bc4f2eb
...

💬 fjahr commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2182266101)
> I would just like to note that @fjahr is counted as NACK by mistake (read fully his [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978) which is just mentioning that the other he is replying to can "give a NACK").

I didn't notice, I gave @DrahtBot a 👎 so it should be removed.

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1648625983)
fixed

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2182290089)
Just fixed typo noticed by @mzumsande : https://github.com/bitcoin/bitcoin/compare/750f8b50a749e577fc11f2f9f79e06cdd29e66f5..2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28

💬 vasild commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-2182297379)
I re-reviewed this again and retain my ACK because IMO it is an improvement to `master` even without further changes because it reduces the scope where the mutex is held. I don't see the fact that it can go further as a blocker for this.

I would be happy to review changing `StdMutex m_cs` to `Mutex m_cs` here or in another PR.

> The PR description sounded like this was going to be the ultimate change

well, fwiw, the description contains "not introducing that change here".

💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2182303482)
Rebased to fix conflicts.

💬 TheCharlatan commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2182315865)
Guix builds (aarch64):
```
7f69ccde68d336bee6141d2cbcec8046332dedc908ba428ac08299daeb6a5d41 guix-build-545bb6c96080/output/aarch64-linux-gnu/SHA256SUMS.part
e843d7b6003da9f6259da41019f328af337858a45e4197ee9c5050984dc9e255 guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu-debug.tar.gz
1fdc9afbf83b2e8ce19d8a2c5cd080d87172ad02c4ce41036962f1bf675a415a guix-build-545bb6c96080/output/aarch64-linux-gnu/bitcoin-545bb6c96080-aarch64-linux-gnu.tar.gz
400f2e70a
...

👍 MarnixCroes approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2132212545)
tested ACK 4a028cf54c0502bc9ba0804bf1ae413b20a007cb

💬 dergoegge commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182380008)
There are a few things that `--enable-fuzz` does that we definitely want it to do:

* As you said, this option disables all other binaries besides the `fuzz` binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (`--with-sanitizers=fuzzer` i.e. `-fsanitize=fuzzer` with clang), because the other binaries provide a main function.
* libFuzzer is not the only fuzzing engine. We would like to support as many engines as
...

💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182390844)
> > > Maybe wait for the CI to pass, then address the nits to test the created cache?
> >
> >
> > Oh, I guess only master creates the cache?
> > ```
> > if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> > ```
> >
> > So I guess this can be merged after addressing the nits.
>
> Maybe comment that line out in a temporary "NO MERGE" commit?

I've pushed a modified branch to my personal account. Here is a CI job: https://github.com/he
...

👍 vasild approved a pull request: "[PoC] ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164#pullrequestreview-2132233632)
ACK a622fbe70a1842396e2670050f8dd7717d764d3c More testing on different environments is better.

I have no opinion or enough information to judge the security implications for the repository (I am not sure what github actions is exactly) or the limited CI resources or how often a given task finds problems. For any new task we don't know what problems would it find regularly and how would that compare to other tasks.

Maybe, if CI resources are really scarce, an existent CI task can be flipped
...

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182399338)
> I've pushed a modified branch to my personal account.

Me also a few comments up :)

💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182401964)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)

Oh sorry, I missed it.

💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182406521)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)

Could you please to rerun that job one more time to ensure that `ccache --show-stats` will print nearly 100% hit rate?

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182409729)
Although I can see the cache get restored the job is no faster so I'm not sure it's working.

> Could you please rerun that job one more time to ensure that ccache --show-stats will print nearly 100% hit rate?

Will do this

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182430012)
Re-running but the cache hit rate at the moment is low:

```shell
ccache version 4.9.1
Cacheable calls: 770 / 782 (98.47%)
Hits: 59 / 770 ( 7.66%)
Direct: 7 / 59 (11.86%)
Preprocessed: 52 / 59 (88.14%)
Misses: 711 / 770 (92.34%)
Uncacheable calls: 12 / 782 ( 1.53%)
Local storage:
Cache size (GB): 0.1 / 0.1 (102.4%)
Cleanups: 604
Hits: 59 / 770 ( 7.66%)
Misses: 711 / 770 (92.34%)
```

💬 fanquake commented on pull request "WIP Optimize SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182433397)
> The changes make it ~10% faster

I tested this change on an aarch64 machine with GCC 14.1.1, and the benchmark was ~ the same, or slower:
Master
```bash
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 27.32 | 36,601,
...

💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182440051)
Ok, a few takeaways:

* Don't enable `-fsanitize=fuzzer` automatically by e.g. `--enable-fuzz` because that would conflict with other fuzzers.

* A better name for `--enable-fuzz` would be `--disable-all-build-targets-except-fuzz` or `--build-only-fuzz` or `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.

💬 paplorinc commented on pull request "WIP Optimize SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182442479)
Thanks for checking @fanquake, I noticed that the [coverage report](https://corecheck.dev/bitcoin/bitcoin/pulls/30317) also reported the same - seems the M1 processor is doing something differently.