💬 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?

🤔 hebasto reviewed a pull request: "depends: static libxcb-cursor"
(https://github.com/bitcoin/bitcoin/pull/33434#pullrequestreview-3263686528)
My Guix builds for Linux hosts on both `aarch64` and `x86_64` platforms:
```
cf7f86d91288477c11afdc767969d6b19b0ba1e62c73924e72b22a2527374653 guix-build-eca50854e1cb/output/aarch64-linux-gnu/SHA256SUMS.part
5afe50eb76079f96a5129b5e51e2ede657f10c5031abe24a875410f006c55511 guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoin-eca50854e1cb-aarch64-linux-gnu-debug.tar.gz
ba6f7937c5d333e64560abcd95dbb3856ceaa43fbcde766ebcaed1f8e23f2a7e guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoi
...

🤔 glozow reviewed a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263812697)
nice, utACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8

🤔 furszy reviewed a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263830391)
Code ACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8