💬 fanquake commented on pull request "[28.x] 28.2rc2":
(https://github.com/bitcoin/bitcoin/pull/32684#issuecomment-2948529722)
Guix Build:
```bash
be30f094366c71bd7251247319dbb0cf1b5674d13e210685328daff1bf855766 guix-build-3b05c498a08e/output/aarch64-linux-gnu/SHA256SUMS.part
1823e9b3d7c97a533ab77540095223bc48759216b9e9fe757f24575f2323680f guix-build-3b05c498a08e/output/aarch64-linux-gnu/bitcoin-3b05c498a08e-aarch64-linux-gnu-debug.tar.gz
8fd4d3a844e845ae13a95d165ae2cd1fea735295c24bc68b3fd28cb91f59c397 guix-build-3b05c498a08e/output/aarch64-linux-gnu/bitcoin-3b05c498a08e-aarch64-linux-gnu.tar.gz
b86f8537ceb5f364
...

🤔 janb84 reviewed a pull request: "ci: Rewrite test-each-commit as py script"
(https://github.com/bitcoin/bitcoin/pull/32680#pullrequestreview-2904243167)
After some contemplating I think the Python code is the better way to go. The Rust code was written a bit verbose and could have been made smaller, but Python seems like the more suitable tool for this task because:

- Its more readable for CI maintenance, more people have python knowledge.
- Python integrates better with existing CI tooling (e.g. is already used in the ci.yml)
- Lower barrier to entry for contributors who need to debug CI issues.

And Python still gives us the advantages
...

💬 Sjors commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2948584686)
I tested this on top of #31802 on a VM running OpenBSD 7.7 (dropping `NO_IPC` for OpenBSB).

Without the patch I get a bunch of errors like:

```
[ 2%] Building CXX object src/kj/CMakeFiles/kj.dir/cidr.c++.o
/root/src/bitcoin/depends/work/build/aarch64-unknown-openbsd7.7/native_capnp/1.1.0-f5dbdc0b2f0/src/kj/cidr.c++:116:71: error: member access into incomplete type 'const struct sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
```
...

💬 dergoegge commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2131827333)
nit: Same as above, the Assume can be inline

💬 dergoegge commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2131823809)
nit: I think this means we can get rid of `READ_STATUS_CHECKBLOCK_FAILED`? It's unused now.

💬 maflcko commented on issue "depends: OpenBSD needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2948623295)
> I'm not sure. The following seems [working](https://github.com/hebasto/bitcoin-core-nightly/blob/2973f9d010f6581a7663d5cea4246ee7f6dc4070/.github/workflows/openbsd.yml#L104):

This was tested on x86:

```
to: /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/depends/x86_64-unknown-openbsd7.7
```

However this issue happens on aarch64:

```
gmake: *** [funcs.mk:343: /home/sjors/src/bitcoin/depends/aarch64-unknown-openbsd7.7
```

Does UTM allow to spin up an x86_64 openbsd for testing?
...

💬 Sjors commented on issue "depends: OpenBSD needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2948641785)
@maflcko I can choose between Virtualize and Emulate, the latter being a bit slower, but might avoid this issue. I'll try.

🤔 stickies-v reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2904095923)
Approach ACK. I think this PR needs release notes because of the behaviour change.

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131702030)
nit: moving it into the `BCLog` namespace seems fine, but `BCLog::WINDOW_MAX_BYTES` is not a very helpful name. `RATELIMIT_MAX_BYTES` probably better?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131720267)
nit: line length

```suggestion
//! Attempts to reset the logging window if the window interval has passed. This will clear
//! m_source_locations and m_suppressed_locations if a reset occurs.
```

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131883418)
As you pointed out, this changes the logging output by now including the full function signature instead of just the function name. I think that's generally an improvement, but should probably be mentioned in the release notes (which would be good to have for the new rate limiting behaviour in general anyway) because it might break downstream log parsing. Would suggest adding commit at the top with release notes?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131861828)
This will now only be shown for a single source location per time window. I think this should be moved to `MaybeResetWindow()`, where we iterate over `m_suppressed_locations` before clearing it?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131888118)
Since `source_loc` is fairly large and this function is frequently called, I think it might make sense to optimize this a bit further and have `LogPrintStr_` (and its callsites) take a `std::source_location&&` so we can move it here and avoid the copy?

<details>
<summary>git diff on 911ee520c8</summary>

```diff
diff --git a/src/logging.cpp b/src/logging.cpp
index e31e346549..006bac6a98 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -74,8 +74,7 @@ bool BCLog::Logger::StartLoggi
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131856132)
> I think it makes the function names a little longer in the map, but I'm ok with it.

Oh interesting, I didn't realize that. On my machine, `__func__` contains just the function name, whereas `function_name()` returns the entire signature, e.g.:

```
__func__: AppInitParameterInteraction
source_location: bool AppInitParameterInteraction(const ArgsManager &)
```

I don't see how this affects any maps, though? I think it just affects the messages when we stop and restart logging?

> I
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131722722)
Docstring could be beefed up a bit to highlight that this is not a pure check but has side effects. E.g.:

```
//! Consumes `source_loc`'s available bytes corresponding to the size of the (formatted)
//! `str` and returns true if it exceeds the allowance in the current time window.

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2131914811)
Nice. Since af86b55f996cf06e8e9179d5c1fe7ce3d94fa3c6 now uses `LogPrintLevel_` directly, I think the changes to `LogPrintLevel` in ed3c0a592b60a565ec5dade7c3af62e7f4cf75a1 can now be reverted and rate limiting applied consistently to all "public" macros? (would also require an update to the `UpdateTipLog` docstring)

👍 willcl-ark approved a pull request: "[28.x] 28.2rc2"
(https://github.com/bitcoin/bitcoin/pull/32684#pullrequestreview-2904528329)
crACK 3b05c498a08e004a349e768c7bdbd09362673920

will kick off a guix build over lunch, but backport and version changes all appear correct

💬 fanquake commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2132018213)
Can you clarify why the casting was added; with it removed, I don't see any issues in our CI.

💬 hebasto commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#discussion_r2132019062)
I believe the same applies to [FreeBSD](https://man.freebsd.org/cgi/man.cgi?query=sha256) as well.

💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2948958116)
I don't think so. If anything, it's even more of an indication that we should swap what has been implemented for GCC (and still not documented as such, leading to i.e #32571) for LLVM / Clang, which is clearly what project contributors are using, or wanting to use.