💬 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

💬 maflcko commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663937723)
I also don't understand why the first commit is needed. The error message is already correct (and unchanged in this pull request): `Untranslated("Inputs missing or spent")`.

I'd say it is fine to drop the commit.

💬 willcl-ark commented on pull request "Ignore files ignored by git in the Markdown Link Checker lint":
(https://github.com/bitcoin/bitcoin/pull/30380#discussion_r1663949766)
Yes, it looks like it can. Added a commit for that

💬 maflcko commented on pull request "psbt: Check non witness utxo outpoint early":
(https://github.com/bitcoin/bitcoin/pull/29855#issuecomment-2205695176)
It would be nice to have this in 28.x

💬 willcl-ark commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1663950622)
Done in 7e36dca657c66bc70b04d5b850e5a335aecfb902

👍 maflcko approved a pull request: "lint: Ignore files ignored by git in the Markdown Link Checker"
(https://github.com/bitcoin/bitcoin/pull/30380#pullrequestreview-2156055054)
lgtm

📝 willcl-ark opened a pull request: "[WIP] net: return result from addnode RPC"
(https://github.com/bitcoin/bitcoin/pull/30381)
Fixes: #20552

Picks up a [branch](https://github.com/bitcoin/bitcoin/compare/master...amitiuttarwar:bitcoin:2020-11-open-conn-improvements) from by @amitiuttarwar which adds a success response to `addnode` RPC.

It adds a new return object to `addnode` RPC indicating that the call was successful:

```cpp
{
{RPCResult::Type::STR, "operation", "The operation called (onetry/add/remove)"},
{RPCResult::Type::STR, "result", "The result of the operation (success/failed)"},
{RPCRe
...

💬 willcl-ark commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2205725737)
I did experiment with not throwing `JSONRPCError`s at all from this call, and always returning a `result` and optionally an `error` if it occurred, but this doesn't align with our general practice of throwing `JSONRPCError`s on failures, so I abandoned this approach.

This does mean that the `result` field here is pretty much redundant; we either throw, or return a result (implicitly indidcating success). Perhaps it could therefore be removed? Unsure if folks would prefer the re-assurance of s
...

💬 willcl-ark commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2205734826)
Before these changes:

```log
$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry

```

After:

```log
$ src/bitcoin-cli -signet addnode i-am-not-an-address onetry
error code: -24
error message:
Unable to open connection
```

💬 willcl-ark commented on issue "addnode RPC call should return success/failure indicator":
(https://github.com/bitcoin/bitcoin/issues/20552#issuecomment-2205735199)
Opened #30381 to address this issue.

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#discussion_r1663965619)
Thanks for checking. I'd say dropping support for macos-13+xcode should be done in a follow-up (probably far into the future).

Or was it meant as a nit to remove the `?` in this line of code? If yes, I'll address it, if I have to retouch or rebase.

💬 ryanofsky commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2205758873)
> 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. (I didn't use range-diff and missed it, fwiw)

I'll often ACK a PR and make a suggestion like you can "replace foo/2 with bar". Then if the PR is updated, I will check the range diff and reACK with "the only thing that changed since last review is replacing foo/2 with bar" without looking at the complete diff again. This is potentially d
...