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

💬 instagibbs commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2132037362)
did that in a7cbaca0af38c183f009a7c8e2cfb54c17a1dc5a

💬 instagibbs commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2132037528)
will do the next time I touch it

💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2132074962)
Hmm, is there a reason why blockundo takes only 1.6ms in total end-to-end when spenttxouts takes 4.6ms in total end-to-end, where about 2.6ms happen on the server (https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001). So with spenttxouts, the client spends about 2ms with 18% CPU and with blockundo the client spends about .8ms with 58% CPU.

Just wondering out of curiosity.

Personally I'd just go with `spenttxouts`, but no strong opinion.

💬 willcl-ark commented on pull request "[28.x] 28.2rc2":
(https://github.com/bitcoin/bitcoin/pull/32684#issuecomment-2949119684)
```
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
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-bui
...