💬 optout21 commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2188489973)
Nit: Maybe `options` could be inlined here?

📝 Raimo33 opened a pull request: "Remove redundant else statements"
(https://github.com/bitcoin/bitcoin/pull/32886)

✅ Raimo33 closed a pull request: "Remove redundant else statements"
(https://github.com/bitcoin/bitcoin/pull/32886)

📝 Raimo33 reopened a pull request: "Remove redundant else statements"
(https://github.com/bitcoin/bitcoin/pull/32886)

💬 l0rinc commented on pull request "Remove redundant else statements":
(https://github.com/bitcoin/bitcoin/pull/32886#issuecomment-3042335106)
Please stop spamming, a lot of people are getting email notifications in these cases.

💬 Raimo33 commented on pull request "Remove redundant else statements":
(https://github.com/bitcoin/bitcoin/pull/32886#issuecomment-3042340180)
Got it. But I see a lot of bad code through the whole repo

✅ pinheadmz closed a pull request: "Remove redundant else statements"
(https://github.com/bitcoin/bitcoin/pull/32886)

💬 pinheadmz commented on pull request "Remove redundant else statements":
(https://github.com/bitcoin/bitcoin/pull/32886#issuecomment-3042346466)
Please don't open new pull requests just for tiny typo fixes, they are a drag on our integration testing infrastructure, maintainer time and reviewer time. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring:

> Trivial pull requests or pull requests that refactor the code with no clear benefits may be immediately closed by the maintainers to reduce unnecessary workload on reviewing.

There are many more significant ways to contribute to Bitcoin -- for example, loo
...

🤔 Prabhat1308 reviewed a pull request: "threading: use correct mutex name in reverse_lock fatal error messages"
(https://github.com/bitcoin/bitcoin/pull/32829#pullrequestreview-2991646500)
> So one thing I noticed is that the tests passed, but the previous commit [85c2848](https://github.com/bitcoin/bitcoin/pull/32829/commits/85c2848eb575f4abaa81fdd4e8f3b2048693dd98) had failures but CI / test each commit (pull_request) passed
>
> not sure this is a followup someone can look at to fix?

I could replicate this when I built cmake with
```
cmake -B build -DCMAKE_BUILD_TYPE=Debug
```
This is because the specific piece of code is not enabled until there is the `DEBUG_LOCKORDE
...

💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3042550496)
Rebased 155afe8529a611c3dcb3fb76101abd01020a24ea -> 80810b6011e30ac5ff72c43a2fbfd0e13df0c4cc ([blocktreestore_0](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_0) -> [blocktreestore_1](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_0..blocktreestore_1))

* Fixed silent merge conflict with #29307

🤔 mzumsande reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-2991732098)
I think that this assert could be hit if a reorg was happening while `Sync()` was going on:

1) `Sync()` syncs some blocks but isn't finished (doesn't update m_best_block)
2) The node undergoes a reorg, so that some of the synced blocks need to be rewinded.

3) `Rewind()` is called and the assert fails.

I could trigger this on regtest with an added sleep in `Sync()`, freezing the sync so that I could do the reorg.
I think it's going to be hard / impossible to trigger it in a test witho
...

💬 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_r2188157982)
what's the reason for commented out code? Is this PR still in draft mode or is it ready for review?

💬 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_r2188155450)
nit: so many comment styles used here, most in new code. Now that this is a one-liner, we can unify it with the rest of the comments here:
```suggestion
//! Fixed window rate limiter for logging.
```

If you apply this, please see other similar ones as well.

The problem with these inconsistencies is not just cosmetic, these differences are distracting us from being able to treat inconsistencies as code smells.

🤔 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)
```