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

💬 kevkevinpal commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541695322)
is this the reason for the limitation? or are there other reasons?

if its just this couldn't we just get how many commits are in this branch and use it here?

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2541710794)
i don't think this heuristic makes sense, because it is already incomplete with the merge of this pull, and it is unclear how to even maintain it going forward. I don't think it is a good use of time to think about every single named argument, whether it can be a string, and whether the string may theoretically contain a `=`.

It would be better to just fully list all named args names in the client. Then, just check if the string starts with the named arg name. If yes, it is a named arg. If not,
...

💬 fanquake commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2541711810)
xcb & xkb libs can be dropped.

💬 fanquake commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2541716526)
I'm not sure we need to link to Qt docs here, given it's listing ~ 25 different things, of which only 2-3 are relevant to our binaries.

💬 theStack commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3552330703)
In addition to `feature_bip68_sequence.py`, it seems that the functional tests `feature_coinstatsindex.py`, `feature_pruning.py` and `mempool_reorg.py` hit this code-path as well (tested experimentally by putting an `assert(false)` into the branch to provoke a crash), most likely all of them in the course of an `invalidateblock` call, though I didn't check in detail. However, none of these use the `-alertnotify` option, so the function `AlertNotify` returns early in those cases.

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