💬 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

💬 fjahr commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3329887782)
This seems to implement the same logic that I used in #31117 but this turned out to be too fragile to be seriously considered for merging there. In this context it may work well enough for mainnet because there are not a lot of forks but we have been discussing an alternative approach for a while that does not rely on `invalidateblock`/`reconsiderblock` and this would solve the competing forks problem as well. I have worked on this idea for #31117 primarily but I will open a PR with that approac
...

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376540950)
This fails `test_unspent_ephemeral` for me when applied on 00c02b42c61

💬 IdotMaster1 commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3330009658)
Concept ACK, but turn the default OP_RETURN back to normal.

🚀 fanquake merged a pull request: "Backport Cirrus runners to 28.x"
(https://github.com/bitcoin/bitcoin/pull/33406)

💬 enirox001 commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3330024209)
Thanks for the review and context @fjahr . I wasn't aware of that prior work.

You're right that this approach has some fragility concerns. I'd be very interested to see your alternative approach that avoids invalidateblock/reconsiderblock altogether, as that does sound more robust.

Regarding the test failure - let me investigate and fix that. Would you prefer I:
1. Fix the test and keep this PR open for comparison with your upcoming alternative approach, or
2. Close this PR and wait fo
...

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376581403)
ah right, it will fail with in-mempool sibling because package version will simply fail when it encounters a "parent already has child in mempool" condition it's not meant for RBF cases

💬 glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376602411)
Noting that this check is redundant with `ReplacementChecks`, since `CalculateChunksForRBF` also calls it before calculating the feerate diagram. I think checking twice is fine, but that one has a slightly different error message that should be unified imo. See this test:

```
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 3da8b477a2f..8bcc9b2fe65 100755
--- a/test/functional/mempool_cluster.py
+++ b/test/functional/mempool_cluster.py
@@ -4,6 +4
...