💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1663407636)
Thank you for catching this!

💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2205008738)
> Thanks AJ, it's great to get specific and find out what actual problems you are concerned about,

What I'm concerned about is making logging unnecessarily complicated to use -- it should be easy to use, and it should be easy to understand what's going on without needing any context from the rest of the module where the logging is happening, or from debates about how logging should work.

You asked about specific problems this change could cause; and I'm happy to give examples, but they'r
...

💬 TheCharlatan commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205252653)
Re https://github.com/bitcoin/bitcoin/pull/30371#issue-2384114654

> Locally, I can find the bug with -max_len=84 -use_value_profile=1 on a single thread on a laptop.

I've been running this for a day and did not find the crash. How long was this running for?

💬 maflcko commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205271688)
16kk iterations; N=1. Again, I guess my Laptop just got extremely lucky.

📝 maflcko converted_to_draft a pull request: "fuzz: Mutate -max_len= during generation phase"
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.

Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...

💬 maflcko commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205276132)
Turning into draft for now, to allow for more time to benchmark and evaluate this.

🤔 maflcko reviewed a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2155603386)
post-merge lgtm. Left some nits

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194)
What is the point of translating this message when the translation is discarded? Seems wasteful of translator's time, no?

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973)
It is not possible to pass a raw c-string pointer to transifex for translation. Only string literals can be translated.

I guess it could make sense to mark `_(const char*)` as `consteval` to catch those issues at compile time?

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663647981)
Is the trailing newline needed? This would be the first `JSONRPCError` with a trailing newline.

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663637823)
Also, this commit is not a "refactor", because it changes logging and RPC error strings slightly.

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663666751)
I think the path is transformed to an absolute one, so the string representation may be different from the one passed in by the user.

⚠️ maflcko opened an issue: "Use C++20 consteval to verify translation strings"
(https://github.com/bitcoin/bitcoin/issues/30379)
It is possible to write `_(str_ptr)`, which is wrong, see https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973.

I am not familiar with the translation build system integration, but it would be good to effectively change the signature of `_` from `auto _(const char*)` to `consteval auto _(const char*)`.

My understanding is that this would catch this error at compile time, saving review effort and follow-up fixup changes.

Assumed compiler error message:

```
error: the
...

📝 willcl-ark opened a pull request: "Ignore files ignored by git in the Markdown Link Checker lint"
(https://github.com/bitcoin/bitcoin/pull/30380)
Updating to MLC v0.18.0 includes a new feature which will ignore all files ignored by git: `mlc --gitignore`.

This helps avoid false-positives flagged by this linter in non-project files, such as a developer might expect to have in their working directory (e.g. guix-builds, python venvs, etc.)

💬 maflcko commented on pull request "Ignore files ignored by git in the Markdown Link Checker lint":
(https://github.com/bitcoin/bitcoin/pull/30380#issuecomment-2205498290)
The `lint` suffix in the title should be a prefix :sweat_smile:

lgtm otherwise

💬 maflcko commented on pull request "Ignore files ignored by git in the Markdown Link Checker lint":
(https://github.com/bitcoin/bitcoin/pull/30380#discussion_r1663856433)
nit below: Can the `// Filter out this annoying trailing line` be removed now?

💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2205557724)
> That's interesting, I guess it shows the dangers of reviewing with range-diff, because it's hard to know if a review suggestion was fully implemented or there may be some old code left behind.

I don't think this is related to `range-diff`. It is simply a typo, which can be missed by a human reviewer regardless of how the code or diff is displayed.

🤔 maflcko reviewed a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2156027314)
Left a question. Also, I don't think this is a "refactor"

💬 maflcko commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663947032)
a05f4ca241045e8a4b295760590805f961c2e5e7: This commit is not a refactor either.

💬 maflcko commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663945296)
95b09ed4741b9a15d7d285e248161d5a508f5586: The second commit is not a refactor, but claims to be one. However, it changes the rpc error messages. Also, it is removing the `RPC_VERIFY_ALREADY_IN_CHAIN` from this header. Either this is fine, in which case this whole "backward compat" section can be removed, or it is not fine, in which case the symbol needs to be restored. C.f. d29a2917ff73f7e82b32bd94a87df3ee211a27c2