💬 waketraindev commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3552754903)
For reference, this is what I get building master `x86_64-w64-mingw32-gcc (GCC)` 13-posix WSL/Depends build
**without** this patch (issue: https://github.com/bitcoin-core/gui/issues/906)

<img width="776" height="158" alt="image" src="https://github.com/user-attachments/assets/8ad1ffec-4331-4439-b22e-df008eabc57e" />

The amount inputbox/spinner is too small and inputed amount is not visible.

💬 sipa commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3552776457)
I don't think we should have any commits which are known to not compile on a reasonable system.

You'll need re-ACKs anyway for the PR, I don't think there is a material difference for ease of reviewing for doing it in a separate commit vs. just fixing the commit that introduces the problem.

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542077377)
This name=value syntax is inherently ambiguous. There was already a heuristic being used to interpret it before this pull, and now a better heuristic is implemented. I think the new parsing code is simpler and less confusing than the previous code, and it is definitely better documented and tested. This PR is fixing a real issue with bitcoin-cli and base64 which makes PBST methods difficult to use, reported https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2357586680 and other places. I
...

💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3552812910)
> just fixing the commit that introduces the problem

Thanks, did that, should be no difference compared to last head, just a fixup of the last commit.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3552825200)
`b499edb414...07650a49f9`: address https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2539959279

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2542129954)
Yes, stopping the SOCKS5 proxy fixes this. I applied this diff:

```diff
--- i/test/functional/test_framework/socks5.py
+++ w/test/functional/test_framework/socks5.py
@@ -9,6 +9,7 @@ import socket
import threading
import queue
import logging
+import os

from .netutil import (
format_addr_port
@@ -191,6 +192,7 @@ class Socks5Connection():
except Exception as e:
logger.exception("socks5 request handling failed.")
self.serv.queue.put(e)

...

💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3552877873)
> Even though @ryanofsky didn't explicitly review this PR here yet my understanding is that he didn't intend #33770 as a replacement of this either, but please correct me if I am wrong.

I did intend to #33770 to be an alternative this PR and as far I I know there wouldn't be any benefits to this PR if #33770 is merged, unless I'm missing something. Both this PR and #33770 are addressing the problem of `-asmap` default value not matching its default behavior. #33770 addresses that by making th
...

🤔 ryanofsky reviewed a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770#pullrequestreview-3482971511)
Thanks for the reviews!

<!-- begin push-5 -->
Updated deac31dd779d28a8c01f1c3153650f5827cfe75d -> 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4 ([`pr/asmapd.4`](https://github.com/ryanofsky/bitcoin/commits/pr/asmapd.4) -> [`pr/asmapd.5`](https://github.com/ryanofsky/bitcoin/commits/pr/asmapd.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/asmapd.4..pr/asmapd.5))<!-- end --> making suggested documentation tweaks (since previous ACKs were stale anyway)

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2542201608)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529117675

> I also don't like the inconsistent `(default: none)`. I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for `-i2psam` should be fixed.

Thanks, I still think it is nice to be explicit about the default behavior, but I get that if you think about it, any other default behavior would be surprising, so it's not really necessary to mention. Dropped `(default: none)` from bo
...

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2542202272)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529118333

> nit: `strprintf` isn't needed anymore.

Thanks, removed

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3552979597)
> > Even though @ryanofsky didn't explicitly review this PR here yet my understanding is that he didn't intend #33770 as a replacement of this either, but please correct me if I am wrong.
>
> I did intend to #33770 to be an alternative this PR and as far I I know there wouldn't be any benefits to this PR if #33770 is merged, unless I'm missing something. Both this PR and #33770 are addressing the problem of `-asmap` default value not matching its default behavior. #33770 addresses that by mak
...

💬 brunoerg commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3552990665)
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4

Verified it removed the `strprintf` and `(default: none)`.

💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3552996786)
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4

📝 fanquake converted_to_draft a pull request: "subprocess: Let shell parse command on non-Windows systems"
(https://github.com/bitcoin/bitcoin/pull/32577)
Fixes https://github.com/bitcoin/bitcoin/issues/32574.

The `subprocess::Popen` constructor has two overloads: https://github.com/bitcoin/bitcoin/blob/7763e86afa045910a14ac9b2cd644927a447370b/src/util/subprocess.h#L938-L941

During the migration from Boost.Process in https://github.com/bitcoin/bitcoin/pull/28981, the second was chosen for two reasons: (1) it minimized changes at the call sites, and (2) it [addressed](https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837238921) quot
...

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542278811)
> For example, suppose someone created a wallet where -named is enabled by default using: `./bitcoin-cli createwallet "my=wallet"` internally this will be passed as a -named argument and the user will get the error about "Unknown named parameter my" even though the user passed the argument by position. That's why I think your suggested approach might not work here, but you can clarify that if you think so or if I'm missing any point.

I don't understand why my suggestion does not work. `my=` i
...

💬 brunoerg commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542333645)
8a40e10c1ef4cfdd5fe19796131af2b5a34531ce: nit: `i` is not used.
```suggestion
for _ in range(7): # invalid chain must be at least 6 blocks longer to trigger warning
```

💬 exp3rimenter commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3553135639)
Concept ACK on refactoring bool to return state.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542387292)
Ah, rebase fail I guess, done.

💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3553263582)
Thanks for explaining. Now I see why you still want this PR. In the short term with #33770 there are no problems, and in the long terms with embedded asmap turned on by default there are no problems. But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that `-asmap=1` would load a file called `1` if we don't take additional steps to prevent this.

To clarify:

- In the short term, with [#33770](https://github.com/bitcoin/bitcoin/pull/3377
...

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542466280)
Ok, I hope I got them all now.