💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965146687)
`value.isTrue` is indeed `false` when I set `upnp=1` in `bitcoin.conf`, very strange.

Another strange thing is that this code appears to be run twice, so I'm seeing the log entries twice.

💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965166840)
Aha, `!value.IsBool()` for `upnp=1`

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965173605)
Yup. That's intentional. The first warning can happen as many times as needed, the json manipulation should ideally only happen once.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965177521)
Probably not a good place to have this discussion.

But i also don't think TODOs in comments make much sense in general, planning should happen on github and not in the code repository, so will remove it.

💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1965182822)
Add back the merged https://github.com/llvm/llvm-project/pull/127811/files#diff-bf90ae5bcd280af8b1305512d76ab1b81d0521bbc5d251f60bc0d44e790f16c0 (last commit makes sure it's exactly the same as the upstream version)

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2674076713)
> ... I'm confused about why it actually doing anything and agree with stickies concern [#30861 (review)](https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2586373527) that forcing CCACHE_NOHASHDIR=1 might not be ideal, especially because according to documentation for [hash_dir](https://ccache.dev/manual/4.0.html#config_hash_dir) it sounds like if CWD is not hashed and `-fdebug-prefix-map` is not used, objects could be built with the wrong debug information.

I'm wondering in wh
...

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965186614)
The real problem here is that GetPersistentSetting also looks into the configuration file. i'll add a flag to skip that and this should work as intended.

💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674085237)
The following works correctly for me on macOS when either setting `upnp=1` in `bitcoin.conf` or `"upnp": true,` in `settings.json`.

```
diff --git a/src/init.cpp b/src/init.cpp
index ca708085d8..5ecbd6bc20 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -807,10 +807,10 @@ void InitParameterInteraction(ArgsManager& args)
if (args.GetSettingsPath(&path, /* temp= */ false)) {
common::SettingsValue value = args.GetPersistentSetting("upnp");
if (!value.isNull()) {
-
...

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2674102561)
i'll stop repeating it now, but that second `if()` for updating the JSON is not supposed to trigger *at all* if the value is in `bitcoin.conf` only and not in the json.

💬 HenryOssy2707 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/958ee5db63dff84f463048405b07425fea60dfce#r152835080)
Line 560

💬 janb84 commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2674124980)
reACK [dedc2d9](https://github.com/bitcoin/bitcoin/commit/dedc2d9cecccb866230c07112d00288237d12eaa)

💬 HenryOssy2707 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/958ee5db63dff84f463048405b07425fea60dfce#r152835216)
# This is a placeholder file. Please follow the instructions in `contrib/devtools/README.md` to generate a bitcoin.conf file.

📝 hebasto converted_to_draft a pull request: "ci, iwyu: Treat warnings as errors for specific targets"
(https://github.com/bitcoin/bitcoin/pull/31308)
This PR is the first step towards treating IWYU warnings as errors. It utilises CMake's native support via the [`CMAKE_CXX_INCLUDE_WHAT_YOU_USE`](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_INCLUDE_WHAT_YOU_USE.html)) variable. While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the `iwyu_tool.py` script or CMake's built-in functionality.

At this stage, only the h
...

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965231203)
Alternatively it could directly check the value and if necessary manipulate `settings.rw_settings`, inside the `LockSettings` lock. This also keeps it atomic (not that we care here). But it also gives the clearest code and doesn't need API changes.

💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2674176699)
To make sure this doesn't affect normal usage (as suggested by @glozow in https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-02-20#1091000), I ran an IBD with and without this change.

I have rebased the PR to latest master yesterday and ran it until 884000 with real peers, comparing it against master.
It passed and didn't introduce any speed regression.

ACK 301017c6217378ca868e17c7cb8ffe3447abb11d

<details>
<summary>Details</summary>

* 7db860c610451cf1c39b84746b61d4da3388c452
...

💬 moonsettler commented on pull request "Implement OP_CHECKSIGFROMSTACK(VERIFY)":
(https://github.com/bitcoin/bitcoin/pull/29270#discussion_r1965267972)
There is nothing wrong with named constants when a value is reused a lot and can change in the future. But they don't really make the code more readeable in most cases.

🤔 rkrux reviewed a pull request: "qa: clarify and document assumeutxo tests"
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2632680234)
Concept ACK dedc2d9cecccb866230c07112d00288237d12eaa

💬 rkrux commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1965270838)
The current implementation certainly makes it easier to parse but for someone new to this piece of code it still feels like magic value as the amount compression logic and the varint serialisation logic are quite custom. To eliminate the magical-ness completely, I would support pulling in the said routines in the test framework. Some of them are already present in the `utxo-to-sqlite.py` tool.

💬 rkrux commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1965273546)
Comment about the commit message:
Curious to know the reason behind using `UTx0` instead of `UTXO`.

📝 l0rinc opened a pull request: "log,optimization: use original log string when no suspicious chars found"
(https://github.com/bitcoin/bitcoin/pull/31923)
We have to make sure that logged messages don't contain non-printable characters.
Until now we've sanitized these messages by recreating them every time, even though we expect most lines to be already normalized.
This PR splits out the happy path by identifying the suspicious characters, and if none were found return the original string, otherwise use that count to preallocate and construct the new string with escaped characters.

For normal messages this results in a ~6.5x speedup.

> bui
...