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

💬 RandyMcMillan commented on pull request "Updated MacOS icon to more closely fit Apple's design standards":
(https://github.com/bitcoin-core/gui/pull/852#issuecomment-3329236575)
I like the "flat" icon in https://github.com/bitcoin-core/gui/pull/879

maybe cherry-pick the art from #879 and post some variations?

💬 ryanofsky commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376156696)
re: 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?

Yes my suggestion above is just a way of improving this python test, which makes sure the client-side type table in `src/rpc/client.cpp` is consistent with type information in the server RPC code. The other file `src/rpc/util.cpp` is a server-side file while provides informat
...

🤔 furszy reviewed a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3263479028)
ACK 1ff9e929489e625a603e8755b8efe849feda1f16

💬 marcofleon commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3329445838)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3

I think this warning makes sense to remove for v30 if there’s not yet a clear timeline for deprecation. I don’t think the wording matters too much for now, and can be decided on later along with a deprecation plan and better documentation wrt this setting.

Could be good to set various arbitrary values on some monitoring nodes and see what the impacts are (potentially more stale blocks?). Ultimately, if users, miners included, want to set this to
...

📝 ismaelsadeeq opened a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475)
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.

The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.

i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.

Instead of bailing out after going through transactions eq
...

💬 jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3329617679)
> > > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
> >
> >
> > Friendly ping to coordinators for addressing issues:
> >
> > * @sr-gi -- Catalan (ca)
> > * @ostruvek -- Czech (cs)
> > * @pryds -- Danish (da)
> > * @laanwj @sipa -- Dutch (nl)
> > * @Emzy -- German (de)
> > * @cryptomeow -- Greek (el)
> > * @jesterhodl -- Polish (pl)
> >
> > UPD. French (fr) and Spanish (es) coordinators have been notified vi
...

💬 maflcko commented on pull request "Backport Cirrus runners to 28.x":
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329647242)
lgtm ACK ea4e0aa8c4 🥄

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ea4e0aa8c4 🥄
gzSENNVTLprDXOwKFeI2v4to/hZq+ZG
...

💬 john-moffett commented on pull request "rpc: addpeeraddress: throw on invalid IP":
(https://github.com/bitcoin/bitcoin/pull/33430#issuecomment-3329686831)
Thanks, all!

@vasild

> As an alternative, we could use the `error` field in the response instead of throwing an exception.

I tried to use the same pattern as `setban` does, which I think has a clean boundary between an invalid input and a problem that occurred despite valid input. To me, that category separation makes sense.

@pablomartin4btc

> Would it be useful for the user to differentiate from an empty address vs a provided invalid IP (which obviously includes an empty value)
...

💬 john-moffett commented on pull request "rpc: Always return per-wtxid entries in submitpackage tx-results":
(https://github.com/bitcoin/bitcoin/pull/33427#discussion_r2376310092)
Apologies if it was too minor to refactor! Will try to keep it more surgical in the future for these types of touch-up PRs.

💬 katesalazar commented on pull request "Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3329734282)
Ah, nice and small, thanks. I have a couple of questions.

This is in your code:

> This must be done before creating the QApplication

Does this mean window won't change live when theme changes, on Linux?

This is in your post:

> unlike macOS where it works out of the box.

Does this mean window will change live when theme changes, on macOS?