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

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

Maybe add a sentence like "See \ref RPCConvertNamedValues for more information on how named and positional arguments are distinguished with -named." since this comment is only describing what entries need to be added to the table, not exactly how the entries are used.

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

Could change "==label==" to "my=wallet" to be consistent with example below. The "my=wallet" example is probably clearer anyway since there is only one `=` and it's in the middle of the string, and wallet names are probably more familiar than labels.