💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2541893083)
Any reason this line was changed? Seems the prior one was shorter, and clearer.

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3552633339)
> Is this still relevant, given you've ACK'd #33770?

Yes, these are solving two separate reasons people were unhappy with the `-asmap` behavior and I think both changes have support and should ideally go into the same release, so users of the option only have to adapt to a new behavior once.

Even though @ryanofsky didn't explicitly review this PR here yet my understanding is that he didn't intend https://github.com/bitcoin/bitcoin/pull/33770 as a replacement of this either, but please cor
...

💬 fjahr commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3552658278)
> forgot to send the nits, basically just a test nit to check the returned size, but just a nit

Thanks, I will check if I can address these in one of the asmap follow-up PRs.

👍 willcl-ark approved a pull request: "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG"
(https://github.com/bitcoin/bitcoin/pull/33888#pullrequestreview-3482692983)
ACK 55555db055b59dd529526915dfc59e5a13e43160

Nice catch! The move looks good with `dimmed-zebra` and the succeeding updates also look correct.

`LINT_CI_IS_PR` is correctly set in the [job](https://github.com/bitcoin/bitcoin/actions/runs/19478315045/job/55743715979?pr=33888#step:6:104).

As there was a break in this check, I checked the top ~ 500 commits (back to May) locally for good measure, which were all signed.

```
src/core/bitcoin on  pr-33888 [$!?] via △ v4.1.2 via 🐍 v3.13.9
...

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541993795)




> ah, since this is the only one that does a merge with master?

Ah, actually all of them do it, and not task is checking for inverse silent merge conflicts.



> So why are we even skipping HEAD if other jobs aren't doing it?

CI can never catch all issues. So at least for issues that are not essential, like silent inverse merge conflicts, it seems ok to not check them.

Same for testing each commit. I mean, it is always great if all commits can be bisected, but test-each-comm
...

💬 maflcko commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3552729205)
> Side-note: `ruff check` is happy with `.github/ci-lint-exec.py` but `ruff format` suggests some changes. I know we don't enforce this, but would be happy to re-ACK with a final commit `ruff`-formatting the file too :)

I tried formatting with `black` and `yapf`, but they contradicted each other, so I haven't tried `ruff format` and I'll leave this as-is for now. :sweat_smile:

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542032116)
So, with the [PR#31375](https://github.com/bitcoin/bitcoin/pull/31375), the `-named` is enabled by default, and the idea is to pass the arguments in the named way or the positional way, without explicitly specifying for both of them. But the problem was that previously some Base64 or even some potential file paths might contain an `=` char in them. This can falsify the above condition of enabling the `-named` by default because if someone is using positional argument parsing, then the `=` in som
...

💬 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