💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113414578)
Rebased after #32888. I ended up indenting the list of apt installed packages for the `test-each-commit` job, which might reduce future conflicts.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113414578)
Rebased after #32888. I ended up indenting the list of apt installed packages for the `test-each-commit` job, which might reduce future conflicts.
💬 fanquake commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3113467932)
A run of the new job / flags here: https://github.com/bitcoin/bitcoin/actions/runs/16495974354/job/46641652091?pr=33044.
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3113467932)
A run of the new job / flags here: https://github.com/bitcoin/bitcoin/actions/runs/16495974354/job/46641652091?pr=33044.
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228533987)
I can drop this is other reviewers agree that it's confusing. I personally think this is the more intuitive behavior for path args independently of asmap because our bool args all work with both `-foo` and `-foo=1` afaik. If the user wants to use `-asmap` (or any path arg) as a bool then I think they would expect it to work the same as the other bool params. Fwiw, I discovered this because I wrote a functional test that covered `-asmap=1` and I actually expected it work, only after it didn't I a
...
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228533987)
I can drop this is other reviewers agree that it's confusing. I personally think this is the more intuitive behavior for path args independently of asmap because our bool args all work with both `-foo` and `-foo=1` afaik. If the user wants to use `-asmap` (or any path arg) as a bool then I think they would expect it to work the same as the other bool params. Fwiw, I discovered this because I wrote a functional test that covered `-asmap=1` and I actually expected it work, only after it didn't I a
...
💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3113495899)
This PR has several acks, but it would be helpful if someone familiar with the build system could review the first commit here cd97905ebc564b8b095099a28d1d5437951927c4 and provide feedback. The other commits are all documentation commits, so less critical. @hebasto @fanquake @theuni @maflcko @purpleKarrot might bit good candidates according to git log (sorry if I forgot anyone).
Related PR #31802 switching on the `ENABLE_IPC` option is a similar state, and could use some build system review.
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3113495899)
This PR has several acks, but it would be helpful if someone familiar with the build system could review the first commit here cd97905ebc564b8b095099a28d1d5437951927c4 and provide feedback. The other commits are all documentation commits, so less critical. @hebasto @fanquake @theuni @maflcko @purpleKarrot might bit good candidates according to git log (sorry if I forgot anyone).
Related PR #31802 switching on the `ENABLE_IPC` option is a similar state, and could use some build system review.
...
📝 darosior opened a pull request: "[29.x] backport #32069"
(https://github.com/bitcoin/bitcoin/pull/33052)
Backport https://github.com/bitcoin/bitcoin/pull/32069 to 29. This is a test flakiness fix for https://github.com/bitcoin/bitcoin/pull/31757, which was backported to 29 in https://github.com/bitcoin/bitcoin/pull/32589.
(https://github.com/bitcoin/bitcoin/pull/33052)
Backport https://github.com/bitcoin/bitcoin/pull/32069 to 29. This is a test flakiness fix for https://github.com/bitcoin/bitcoin/pull/31757, which was backported to 29 in https://github.com/bitcoin/bitcoin/pull/32589.
💬 darosior commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3113500474)
Opened a separate backport PR for the flakiness fix: https://github.com/bitcoin/bitcoin/pull/33052
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3113500474)
Opened a separate backport PR for the flakiness fix: https://github.com/bitcoin/bitcoin/pull/33052
💬 fanquake commented on pull request "test: fix intermittent failure in wallet_reorgsrestore.py":
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-3113519822)
Backported to `29.x` in #33052.
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-3113519822)
Backported to `29.x` in #33052.
👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3051726249)
Code review ACK 241e9d14879abbc75a17dba764ef2ad58179db57. Just rebased since last review
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3051726249)
Code review ACK 241e9d14879abbc75a17dba764ef2ad58179db57. Just rebased since last review
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228555002)
In commit "cmake: enable ENABLE_IPC option by default" (969a9472a43b7e8c0ae1b8d0dd1a71e5d118ea0c)
Maybe sort these while you are splitting them up. libboost-dev libqrencode-dev and systemtap-sdt-dev seem to be the only entries out of order.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228555002)
In commit "cmake: enable ENABLE_IPC option by default" (969a9472a43b7e8c0ae1b8d0dd1a71e5d118ea0c)
Maybe sort these while you are splitting them up. libboost-dev libqrencode-dev and systemtap-sdt-dev seem to be the only entries out of order.
💬 purpleKarrot commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3113533739)
I confirm that the change to the CMake code is OK. I am not very fond of the `install_binary_component` function, but that is orthogonal to the PR.
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3113533739)
I confirm that the change to the CMake code is OK. I am not very fond of the `install_binary_component` function, but that is orthogonal to the PR.
💬 maflcko commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3113534833)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3113534833)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228569351)
done
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228569351)
done
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571714)
I did sort them, but missed a few.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571714)
I did sort them, but missed a few.
💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228571572)
ah doi, restored
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228571572)
ah doi, restored
💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228570611)
🤦 thank you, fixed
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2228570611)
🤦 thank you, fixed
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228578817)
Fixed the sort
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228578817)
Fixed the sort
💬 fanquake commented on pull request "[29.x] backport #32069":
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113551574)
Thanks, this should have been backported when #31757 was done, but likely missed at that happened later. Can you also add the rel note: https://github.com/fanquake/bitcoin/commit/da24253775f9f6cecd8fbcf65e8aa6f40090f72c.
(https://github.com/bitcoin/bitcoin/pull/33052#issuecomment-3113551574)
Thanks, this should have been backported when #31757 was done, but likely missed at that happened later. Can you also add the rel note: https://github.com/fanquake/bitcoin/commit/da24253775f9f6cecd8fbcf65e8aa6f40090f72c.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972)
In 28af4d727b3eeff3da0a3dfab0260c57fb52111c: This should be squashed into the commit that actually changes the Guix build. Ideally CMake would warn/error here, at least in the Guix build. We don't want it to continue silently if binaries don't exist/are missing from these checks.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228510972)
In 28af4d727b3eeff3da0a3dfab0260c57fb52111c: This should be squashed into the commit that actually changes the Guix build. Ideally CMake would warn/error here, at least in the Guix build. We don't want it to continue silently if binaries don't exist/are missing from these checks.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989)
Please don't make random (incomplete codebase-wise) formatting changes, to unrelated parts of the code. I think there's enough happening in this PR.
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228508989)
Please don't make random (incomplete codebase-wise) formatting changes, to unrelated parts of the code. I think there's enough happening in this PR.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627)
Yea, these need fleshing out, because they don't give much useful information to users, and will likely leave them with questions: "What's IPC?" (never defined), "What's `bitcoin-node`; should I use that instead of `bitcoind`?" (same for the GUI), "Why is this enabled by default if it's for some experimental feature I (and basically all other users) don't use?".
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228571627)
Yea, these need fleshing out, because they don't give much useful information to users, and will likely leave them with questions: "What's IPC?" (never defined), "What's `bitcoin-node`; should I use that instead of `bitcoind`?" (same for the GUI), "Why is this enabled by default if it's for some experimental feature I (and basically all other users) don't use?".