👍 hodlinator approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3010008670)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f

Only able to spot improvements since previous ACK (https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2980338061). :)
Re-tested `cmake --install` + running `bitcoin` wrapper and `test_bitcoin`.

Agree with Sjors that codesigning should probably be re-tested since it's been modified (https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3047748032).

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3062068009)
@TheCharlatan better?

💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2200579176)
Do we still need `SetMinVersion()` (`GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `enum WalletFeature`) at all?

💬 TheCharlatan commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3062135707)
I'm still not fully convinced of the new layout, but also would not unequivocally block this. Maybe some more long term contributors should give their approval here though, since it does change how our binaries are shipped and where we are placing things going forward.

Re Russ' previous comment:
> based on what a good command line interface would look like, not based on idiosyncracies of previous releases.

There is a bunch of nuance here, since this also applies to pretty much all the bin
...

📝 Sjors opened a pull request: "refactor: cleanup index logging"
(https://github.com/bitcoin/bitcoin/pull/32948)
This PR removes the use of `__func__` from index logging, since we have `-logsourcelocations`.

It also improves readability by putting `GetName()` in a more logical place.

Before

> coinstatsindex: best block of the index not found. Please rebuild the index.

After:

> best block of coinstatsindex not found. Please rebuild the index.

I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to
...

💬 fanquake commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200655720)
If you're touching these lines, they should be getting changed to `LogInfo`.

💬 maflcko commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200658540)
```suggestion
LogError("Cannot read current %s state; index may be corrupted",
```

nit: Can remove redundant newlines

💬 Sjors commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3062249324)
I missed a few `__func__`. Replaced `LogPrintf` with `LogInfo` and removed trailing `\n`'s

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3062262136)
While we're touching all of these, it might be worthwhile to change to `LogWarn/LogError` where appropriate.

💬 Sjors commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3062269312)
@dergoegge any suggestions? Despite the new guidance, it's still not always obvious what log level to use.

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200688205)
These can all be `LogError`

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200688995)
LogError

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200689580)
```suggestion
LogWarn("previous block header belongs to unexpected block %s; expected %s",
```

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200685338)
```suggestion
LogWarn("Block %s does not connect to an ancestor of "
```

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200690005)
```suggestion
LogWarn("previous block header belongs to unexpected block %s; expected %s",
```

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200685734)
```suggestion
LogWarn("Locator contains block (hash=%s) not on known best "
```

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3062282839)
> What the comment does seem to imply to me is that going forward all new binaries should be put into /libexec and be used through the wrapper binary to avoid idiosyncrasies.

Thanks for the feedback! Just to clarify, I would not say that all new binaries should be put in libexec. I'd just say that unless there is a good reason for a new binary to be installed in `bin/` and present in `PATH` it should be installed in `libexec/` instead. If we developed a new utility that was used by bitcoin co
...

💬 Sjors commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3062312627)
Thanks for the suggestions @dergoegge.

I replaced:
- `LogInfo("WARNING: ` with `LogWarning`,
- `LogInfo("Failed` with `LogError`

💬 dergoegge commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#discussion_r2200728836)
I think this is also worth `LogError` as it'll require user action.

👍 dergoegge approved a pull request: "log: Properly log warnings with warn loglevel in addrdb"
(https://github.com/bitcoin/bitcoin/pull/32933#pullrequestreview-3010299469)
utACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d

👍 fanquake approved a pull request: "cmake: Use newer signature of `qt6_add_lrelease` when available"
(https://github.com/bitcoin/bitcoin/pull/32940#pullrequestreview-3010307685)
ACK 94931656b52fddb7c03b29685844d85c008cf6ca