💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541732485)
See the explanation above:
> timeout-minutes: 360 # Use maximum time, see https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes. Assuming a worst case time of 1 hour per commit, this leads to a --max-count=6 below.

💬 fanquake commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3552345830)
Should #29977 be closed, if we add these docs? Is 20.04 considered unsupported?

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541763940)
The last commit isn't checked either. Maybe 'test some mid commits'

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541775452)
But that's just an optimization, since that's checked elsewhere, no need to point it out here, it's not part of reducing "false sense of security"

💬 fanquake commented on issue "depends: Freetype and xcbproto version in depends are too new":
(https://github.com/bitcoin/bitcoin/issues/29977#issuecomment-3552413548)
Note that libxkb, libxcb, and xcb-proto are now all statically linked, after #33537.

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541787905)
Someone could introduce an inverse silent merge conflict, so in theory that is a "false sense of security"

💬 fanquake commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3552469788)
> minimum GCC version for Windows cross-compilation to [13.1](https://doc.qt.io/qt-6.8/supported-platforms.html#windows).

Have you checked that it doesn't work with 12? According to the documentation, Qt 6.7 (which we currently use) doesn't support any of our Linux compilers. i.e GCC 12+: https://doc.qt.io/archives/qt-6.7/supported-platforms.html#linux-x11, or macOS cross-compilation using Clang: https://doc.qt.io/archives/qt-6.7/supported-platforms.html#macos. So its not clear why we would
...

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541840124)
Isn't that checked by another CI job already?

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541848050)
Which one CI job tests for inverse silent merge conflicts, where the merged result is passing tests, but the raw commit itself is not?

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541861415)
ah, since this is the only one that does a merge with master? So why are we even skipping HEAD if other jobs aren't doing it?

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