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

💬 maflcko commented on issue "Use C++20 consteval to verify translation strings":
(https://github.com/bitcoin/bitcoin/issues/30379#issuecomment-2205761015)
```diff
diff --git a/src/util/translation.h b/src/util/translation.h
index d33fd2d0a0..7d82264a43 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -71,7 +71,7 @@ const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
* Translation function.
* If no translation function is set, simply return the input.
*/
-inline bilingual_str _(const char* psz)
+inline consteval bilingual_str _(const char* psz)
{
return bilingual_str{psz, G_TRANSLATION_
...

🚀 fanquake merged a pull request: "lint: Ignore files ignored by git in the Markdown Link Checker"
(https://github.com/bitcoin/bitcoin/pull/30380)

✅ maflcko closed an issue: "Use C++20 consteval to verify translation strings"
(https://github.com/bitcoin/bitcoin/issues/30379)

💬 maflcko commented on issue "Use C++20 consteval to verify translation strings":
(https://github.com/bitcoin/bitcoin/issues/30379#issuecomment-2205773613)
nvm, I'll open a pull request soon.

⚠️ dg123lzk opened an issue: "I want to set up a node quickly, do you have a snapshot？"
(https://github.com/bitcoin/bitcoin/issues/30382)
I want to set up a node quickly, do you have a snapshot？

✅ willcl-ark closed an issue: "I want to set up a node quickly, do you have a snapshot？"
(https://github.com/bitcoin/bitcoin/issues/30382)

💬 willcl-ark commented on issue "I want to set up a node quickly, do you have a snapshot？":
(https://github.com/bitcoin/bitcoin/issues/30382#issuecomment-2205785013)
Hi @dg123lzk

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com/) or the `#bitcoin` IRC channel on the [Libera Chat](https://libera.chat/) network.

For proposed protocol changes you can post to the [bitcoindev mailing list](https://groups.google.com/g/bitcoindev) or the [Delving Bitcoin](https://delvingbitcoin.org/) Discorse forum.

For general bitcoin discussion you can try [bitcointalk](bitcointalk.org)
...

👍 fanquake approved a pull request: "26.2 final changes"
(https://github.com/bitcoin/bitcoin/pull/30376#pullrequestreview-2156124267)
ACK eef5dbc21b3fd52069f2f0855fb76a13234ebbf3 If you want, you could also backport the changes to get the ASAN job running again, but that isn't blocking here.

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#discussion_r1663999366)
It did not mean any changes to the current branch :)

It was just a note (mostly for myself).

💬 willcl-ark commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-2205804346)
Can we close this and https://github.com/bitcoin/bitcoin/issues/19034 as "unplanned"?

In theory v28.0 will (hopefully) be the final version we will read BDB wallets at all, and we now have our own [BDB parser](https://github.com/bitcoin/bitcoin/pull/26606), and will (soon) be able to [migrate without BDB](https://github.com/bitcoin/bitcoin/pull/26596).

I don't see these issues ever being fixed, and therefore they are not useful to keep open IMO...

💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1664002278)
In commit "refactor: PSBTError::MISSING_INPUTS" (a05f4ca241045e8a4b295760590805f961c2e5e7)

I wonder if "invalid transaction inputs" might be a clearer message for a user than "invalid inputs." (I had suggested "invalid inputs" but in this context it seems vague.)

👍 ryanofsky approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2156118365)
Agree on not calling the commits which change error messages refactoring. I do like first commit making error constant more consistent with error message, but agree it is not essential.

💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663994169)
re: https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663945296

Good catch. It is inconsistent to have added backwards compatible aliases previously when renaming and not add them now when renaming again. But it looks like the backwards compatibility section was added in 2014 when it was might have less clear whether aliases were needed. I'd say now aliases are probably not needed and it would be better to remove this section.

📝 maflcko opened a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383)
The translation helper function `_()` has many problems. For example, the following compiles:

```cpp
auto ptr{"wrong"};
_(ptr);
_(nullptr);
_(0);
_(NULL);
```

However, it is wrong, because none of the arguments passed to the function can be picked up by the translation tooling for transifex.

Fix all issues by enforcing only real string literals can be passed to the function.

💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2205818693)
Can be tested by looking at the CI failure, which should pop up soon, or by manually inserting the C++ code from the pull request description.