👍 andrewtoth approved a pull request: "log: always print initial script verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3263072412)
Approach ACK.

The way this PR untangles the assumevalid conditions and explicitly logs them is great for readability and debugging. I'm not sure this should go into v30 though since it does touch a lot of consensus code.

The PR title understates quite a bit what this PR is doing. I assume since it has evolved since being initially proposed. I think it can be updated to better detail what this is doing.

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2375948743)
nit: same, no need for "deterministically" here.

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2375904163)
nit: I think we can remove "deterministically" here. It's implied.

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2375961559)
Shouldn't this be `getblockcount() == 1`?

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2376007280)
nit: for consistency "block not in assumevalid chain"

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2376005833)
nit: "block not in best header chain"

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2376011525)
nit: "block too recent relative to best header"

💬 andrewtoth commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2376038253)
I believe the parent comment means to set a `const bool fScriptChecks` variable here below the closing if bracket, so that we don't have to change any code below on lines 2594 and 2653. That makes sense to me.

🤔 janb84 reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3263298901)
ACK 9c13be9c45cad19d9db78e318b1e8376d56d6ec5

Thanks for squashing your commits!

My Guix build checksums:
**Commit:** `9c13be9c45ca`

```shell
22ad5809ed8d8399b0c7960e8c57cbaae5220b2667650262a7ec98a25eaf671f guix-build-9c13be9c45ca/output/aarch64-linux-gnu/SHA256SUMS.part
60b2048157462a5bd7ae00fa60ec8f1d59d84857d55125b0eafa98fad4aa780e guix-build-9c13be9c45ca/output/aarch64-linux-gnu/bitcoin-9c13be9c45ca-aarch64-linux-gnu-debug.tar.gz
7a7280f6b1221a62e9e8591365431f2cc3a0fe111abe
...

💬 fanquake commented on pull request "Backport Cirrus runners to 28.x":
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329042228)
cc @m3dwards

💬 mstampfer commented on pull request "contrib: Add zsh completion scripts":
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3329042312)
commits squashed to a single commit for this branch

💬 pablomartin4btc commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376072486)
Following your suggestion @ryanofsky... If a new type is added (`NUM_OR_STR` or even `ARR_OR_STR` from #32468), is `also_string` still needed?

🤔 rkrux reviewed a pull request: "doc: update multisig tutorial to use multipath descriptors"
(https://github.com/bitcoin/bitcoin/pull/33286#pullrequestreview-3263305638)
crACK 3d42607fb5966d8e3b79fdb878d59483432625d9

💬 rkrux commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376065785)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"

Nit: By convention, the first index of the two corresponds to the receive address and the other being the change address as quoted in the above tutorial too: `doc/multisig-tutorial.md`.

```diff
- # Multipath descriptor expands to change and receive
+ # Multipath descriptor expands to receive and change
```

💬 rkrux commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376077617)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"

> with <code><0;1></code> as the change index.

I find it confusing to read it as the change index when one of the indices is non-change (receive) when expanded later. Maybe the below one?

```diff
- with <code><0;1></code> as the change index.
+ with <code><0;1></code> as the derivation index.
```

----------------------------------------
Nit: To keep it consistent with how
...

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376117984)
It makes sense to add JSON_OR_STRING and related parsing logic after this is merged.

💬 RandyMcMillan commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-3329161743)
concept ACK

🤔 pablomartin4btc reviewed a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3263399644)
tACK 316a0c513278d53cb25f42ea502d20691962aad6

`master:`
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
{
"success": false
}
```
This branch:
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
error code: -30
error message:
Invalid IP address
```
I agree with @vasild with the [suggested](https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3257726472) alternative, not strong opinion.

Would it be useful for
...

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3263312993)
Code review ACK 6730abb137eed82073814bdb8dbd577e79520b59. Just some comments and indentation changed since last review. Thanks for considering all the suggestions

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376071372)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)

Every place this function is called, it called with a `size_t` value cast to an `int`. It would seem nice to make it take an a `size_t` value directly, since it is a new function. (Could also change the `CRPCConvertParam` struct to use `size_t` instead of `int` but would suggest not doing that to keep the changes minimal.)