🤔 l0rinc requested changes to a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2991145098)
We're getting closer, but the PR is not ready yet. I went over the code multiple times, left nits and comments and questions all over, regardless of the severity. I hope you can untangle the different directions, I know that so many comments can be overwhelming, I hope you take it the way that I meant them.

My main concerns are that the code doesn't seem finished yet, it still contains commented out code, typos, redundant comments, dangerous recursion, prefixes for non-dropped log lines, dupl
...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188156156)
nit:

```suggestion
//! Whether any log locations are suppressed. Cached view on m_source_locations for performance reasons.
```

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188158709)
Given that we had problems with Windows so far, it might not be immediately obvious if this newline is a single char on Windows as well - can we generalize the test so that it doesn't matter?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188160472)
I haven't spent time trying to come up with an alternative yet, so I might be completely wrong, but I'm not sure I fully understand yet why we need these states.
If the role of the extra `NEWLY_SUPPRESSED` state is just to signal that we need to log a warning message now, maybe we can simplify a bit since it seems to me some work might be duplicated here (i.e. that we should be able to get rid of this enum completely).
Let me know if you need me to create a prototype to (dis)prove my hunch.

...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188215958)
Can we document somewhere the expected behavior for when the node is restarted?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188158176)
this way we're not seeing the unequal values, just the message. Why not `BOOST_CHECK_EQUAL`?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188159134)
Is there a reason to copy the large message on each invocation?
```suggestion
void LogFromLocation(int location, const std::string& message)
```

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188161714)
we seem to have a lot of (comment) styles here, now that it's this short, let's simplify and unify this:

```suggestion
//! Fixed window rate limiter for logging.
```

📝 Sazwan96 opened a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/32887)
Here's a `cmake-multi-platform.yml` GitHub Actions workflow that builds and tests a CMake project on multiple platforms (Linux, macOS, Windows) with different compilers:

```yaml
name: CMake Multi-Platform Build

on:
push:
branches: [ main, master ]
pull_request:
branches: [ main, master ]

jobs:
build:
runs-on: ${{ matrix.os }}
strategy:
matrix:
include:
# Linux configurations
- os: ubuntu-22.04
compiler: gcc
...

💬 Sazwan96 commented on pull request "Create cmake-multi-platform.yml":
(https://github.com/bitcoin/bitcoin/pull/32887#issuecomment-3042937690)
Heloo

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3043395366)
Rebased https://github.com/bitcoin/bitcoin/commit/e260e23d88cd6b2aaa03d9d45dae8b174f2cfe1f to https://github.com/bitcoin/bitcoin/commit/a53d43bc0f010513b0922ad48d611f402ec0e511 to address https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185605623

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2188910716)
I modified to calculate chainwork 100 blocks greater than the current `best_header`, and I checked that the test still verifies the expected behaviour. See https://github.com/bitcoin/bitcoin/pull/31615/commits/a53d43bc0f010513b0922ad48d611f402ec0e511

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2189149829)
done

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2189152069)
Great! Thanks

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2189159233)
@optout21 see https://github.com/bitcoin/bitcoin/pull/32876#issuecomment-3036784505

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2189162753)
Great! Thanks :)
Done

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2189174459)
True! Done

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2189175462)
> To be sure, the "show this help message" does not appear when you just run bitcoin with no arguments

That would be great, but it doesn't for me:

```
% build/bin/bitcoin
Usage: bitcoin [OPTIONS] COMMAND...

Options:
...
-h, --help Show this help message
```

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2189201868)
I don't see in which case a 0 or less can be passed. It should never happen.
If it fails on the assume then there's something wrong in another part of the code and we should not treat that as an expected case.

Happy to go back to return -1 if there's a specific case for that.

🤔 hodlinator reviewed a pull request: "headerssync: Preempt unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2992479971)
Thanks for checking this out @danielabrozzoni! Pushed some minor changes based on your feedback.