💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182144557)
utACK 4ecbbd9b7fa6f30e1d297cd26b112d3148744f58

💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182161395)
Feel free to propose better solutions, the one in "Describe the solution you'd like" in OP is just an idea.

`--with-sanitizers` can be used to enable additional sanitizers. Why would that be confusing? Because when the fuzzing is enabled, then `-fsanitize=fuzzer` would be used for compilation and then it is redundant if one does `--with-sanitizers=fuzzer` in addition? That would be harmless and produce the expected results. Much like if one uses `--enable-werror` and in addition sets `CXXFLAG
...

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2182172611)
I think this can be closed for now. It seems too controversial for a simple test change?

💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182182086)
> That would be harmless and produce the expected results.

No objection if a duplicate `-fsanitize=fuzzer` is harmless and works in all build settings.

👍 vasild approved a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2132036625)
ACK 7d3662fbe35032178c5a5e27e73c592268f6e41b

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