💬 Sjors commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2537840004)
Most of the action is here:
- https://github.com/bitcoin/bitcoin/blob/master/src/external_signer.cpp
- https://github.com/bitcoin/bitcoin/blob/master/src/common/run_command.cpp
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2537840004)
Most of the action is here:
- https://github.com/bitcoin/bitcoin/blob/master/src/external_signer.cpp
- https://github.com/bitcoin/bitcoin/blob/master/src/common/run_command.cpp
🤔 janb84 reviewed a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3477305156)
Concept ACK f8d83ee4d631c95ef6e222d7868bfa9e28b66e05
This PR improves the CI documentation, to clarify certain aspects of the CI which raise questions time to time. (see #31199). I would say that the additional information in this PR would stop some questions from being asked. The information provided is (the docker part) is non-trivial to me, and the importance of `env-i` can be overlooked so the additional information is welcome.
NIT: Small grammar suggestion.
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3477305156)
Concept ACK f8d83ee4d631c95ef6e222d7868bfa9e28b66e05
This PR improves the CI documentation, to clarify certain aspects of the CI which raise questions time to time. (see #31199). I would say that the additional information in this PR would stop some questions from being asked. The information provided is (the docker part) is non-trivial to me, and the importance of `env-i` can be overlooked so the additional information is welcome.
NIT: Small grammar suggestion.
💬 janb84 commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2537769758)
NIT: small grammar suggestion
```suggestion
It is recommended to run the CI system in a clean environment. The `env -i` command
below ensures that *only* specified environment variables are propagated into the local CI.
To run the test stage with a specific configuration:
```
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2537769758)
NIT: small grammar suggestion
```suggestion
It is recommended to run the CI system in a clean environment. The `env -i` command
below ensures that *only* specified environment variables are propagated into the local CI.
To run the test stage with a specific configuration:
```
💬 Sjors commented on issue "External signer: in case of error shows only "External signer failure"":
(https://github.com/bitcoin/bitcoin/issues/31004#issuecomment-3547268112)
Unless you're going to open a PR to improve it yourself, probably nobody else will :-)
I can't close it though, only you or the maintainers can.
(https://github.com/bitcoin/bitcoin/issues/31004#issuecomment-3547268112)
Unless you're going to open a PR to improve it yourself, probably nobody else will :-)
I can't close it though, only you or the maintainers can.
✅ pinheadmz closed an issue: "External signer: in case of error shows only "External signer failure""
(https://github.com/bitcoin/bitcoin/issues/31004)
(https://github.com/bitcoin/bitcoin/issues/31004)
💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3547313774)
I think this could actually be a fine solution, and the additional changes needed are minimal, so I've pushed that up. The hash of the tarball using this branch is `95b00dc41fa090747dc0a7907a5031a2fcb2d7f95c9584ba6bccdb99b6e3d498`.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3547313774)
I think this could actually be a fine solution, and the additional changes needed are minimal, so I've pushed that up. The hash of the tarball using this branch is `95b00dc41fa090747dc0a7907a5031a2fcb2d7f95c9584ba6bccdb99b6e3d498`.
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2537904473)
> We should not make changes to the API if we don't know how completing simple tasks will look like in the future.
I agree with that, but I think I'm misunderstanding the purpose of this PR, then. Adding block headers to the API seems like a pretty straightforward addition to the existing API for which imo just the additional introspection ability seems worthwhile, and that's how I was reviewing it. From your messages, it seems like the actual goal is to implement a specific use case, but tha
...
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2537904473)
> We should not make changes to the API if we don't know how completing simple tasks will look like in the future.
I agree with that, but I think I'm misunderstanding the purpose of this PR, then. Adding block headers to the API seems like a pretty straightforward addition to the existing API for which imo just the additional introspection ability seems worthwhile, and that's how I was reviewing it. From your messages, it seems like the actual goal is to implement a specific use case, but tha
...
💬 Sjors commented on pull request "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3547327475)
Rebased due after #33745 due to import conflict in the test. Also fixed comment typo.
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3547327475)
Rebased due after #33745 due to import conflict in the test. Also fixed comment typo.
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3547353597)
re-ACK 2cb5e6472c8c0109390258c298915dd4011f0292
Trivial rebase after #30595 added `libbitcoinkernel (experimental)` to the CMake summary.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3547353597)
re-ACK 2cb5e6472c8c0109390258c298915dd4011f0292
Trivial rebase after #30595 added `libbitcoinkernel (experimental)` to the CMake summary.
💬 plebhash commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3547392323)
apparently @lucasbalieiro was able to ship https://github.com/stratum-mining/sv2-apps/pull/85 without running into the Docker challenges I originally described as the motivation for IPC via TCP here... I'm yet to dive into it and figure out whether I had any blindspots when opened this issue here
when I asked about encryption I didn't really have the Docker setup in mind... I was just curious about potential use-cases of IPC over TCP
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3547392323)
apparently @lucasbalieiro was able to ship https://github.com/stratum-mining/sv2-apps/pull/85 without running into the Docker challenges I originally described as the motivation for IPC via TCP here... I'm yet to dive into it and figure out whether I had any blindspots when opened this issue here
when I asked about encryption I didn't really have the Docker setup in mind... I was just curious about potential use-cases of IPC over TCP
💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3547440518)
Guix Build (aarch64):
```bash
3bd9c90945da34a1398beceb2474401a486a6f3436bb4a12a573bdfa38df6d62 guix-build-5513cd0941a2/output/arm64-apple-darwin/SHA256SUMS.part
e78e73b65107c95fd4199f5a354471e8f61165c51ee7602b775a59093cf345f5 guix-build-5513cd0941a2/output/arm64-apple-darwin/bitcoin-5513cd0941a2-arm64-apple-darwin-codesigning.tar.gz
3fee841b5625963dda994f68035e2d87decbbce4df666485e76f42fbe2b7f61b guix-build-5513cd0941a2/output/arm64-apple-darwin/bitcoin-5513cd0941a2-arm64-apple-darwin-uns
...
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3547440518)
Guix Build (aarch64):
```bash
3bd9c90945da34a1398beceb2474401a486a6f3436bb4a12a573bdfa38df6d62 guix-build-5513cd0941a2/output/arm64-apple-darwin/SHA256SUMS.part
e78e73b65107c95fd4199f5a354471e8f61165c51ee7602b775a59093cf345f5 guix-build-5513cd0941a2/output/arm64-apple-darwin/bitcoin-5513cd0941a2-arm64-apple-darwin-codesigning.tar.gz
3fee841b5625963dda994f68035e2d87decbbce4df666485e76f42fbe2b7f61b guix-build-5513cd0941a2/output/arm64-apple-darwin/bitcoin-5513cd0941a2-arm64-apple-darwin-uns
...
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538011805)
> From your messages, it seems like the actual goal is to implement a specific use case, but that doesn't seem to be mentioned anywhere?
The PR title says add block header support and validation. To make validation worthwhile, knowing what the best current header is, seems useful. Similarly, we added logic to get the tip for block validation, so you can request more blocks extending your best from your peers. Are you looking for a complete header sync implementation first?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538011805)
> From your messages, it seems like the actual goal is to implement a specific use case, but that doesn't seem to be mentioned anywhere?
The PR title says add block header support and validation. To make validation worthwhile, knowing what the best current header is, seems useful. Similarly, we added logic to get the tip for block validation, so you can request more blocks extending your best from your peers. Are you looking for a complete header sync implementation first?
💬 l0rinc commented on pull request "clang-format: Set InsertNewlineAtEOF: true":
(https://github.com/bitcoin/bitcoin/pull/33896#discussion_r2538032899)
Thanks for checking
(https://github.com/bitcoin/bitcoin/pull/33896#discussion_r2538032899)
Thanks for checking
💬 l0rinc commented on pull request "clang-format: Set InsertNewlineAtEOF: true":
(https://github.com/bitcoin/bitcoin/pull/33896#issuecomment-3547477029)
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
(https://github.com/bitcoin/bitcoin/pull/33896#issuecomment-3547477029)
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
⚠️ Sjors opened an issue: "Block template memory management (for IPC clients)"
(https://github.com/bitcoin/bitcoin/issues/33899)
## Background
Each time we send an IPC client a new `BlockTemplate`, via `createNewBlock` or `waitNext` on the Mining interface, libmultiprocess ensures that we hold on to its encapsulated `CBlockTemplate`. It's just as if our own process were to have shared pointer to it.
We let go of it once either the client calls the `destroy` method or they disconnect (cc @ryanofsky ??).
The `CBlockTemplate` in turn contains a `CBlock` which contains pointers to individual transactions. Those transaction
...
(https://github.com/bitcoin/bitcoin/issues/33899)
## Background
Each time we send an IPC client a new `BlockTemplate`, via `createNewBlock` or `waitNext` on the Mining interface, libmultiprocess ensures that we hold on to its encapsulated `CBlockTemplate`. It's just as if our own process were to have shared pointer to it.
We let go of it once either the client calls the `destroy` method or they disconnect (cc @ryanofsky ??).
The `CBlockTemplate` in turn contains a `CBlock` which contains pointers to individual transactions. Those transaction
...
💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3547497692)
cc @ryanofsky, @plebhash
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3547497692)
cc @ryanofsky, @plebhash
🤔 janb84 reviewed a pull request: "clang-format: Set InsertNewlineAtEOF: true"
(https://github.com/bitcoin/bitcoin/pull/33896#pullrequestreview-3477661462)
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
PR unifies the enforcement of the new line at end of file to also include the rule in the clang-format rules (is already in the linter and in the .editorconfig)
Tested and works as advertised + learned of the existence a new script in contrib :)
(https://github.com/bitcoin/bitcoin/pull/33896#pullrequestreview-3477661462)
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
PR unifies the enforcement of the new line at end of file to also include the rule in the clang-format rules (is already in the linter and in the .editorconfig)
Tested and works as advertised + learned of the existence a new script in contrib :)
💬 m3dwards commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3547524681)
As I understand it, this will produce the cross-compile toolchain using GCC 13. Should we wait on this PR until we have GCC 14] (https://github.com/bitcoin/bitcoin/pull/33775) being as CI for UCRT (https://github.com/bitcoin/bitcoin/pull/33764) will be based on Trixie and GCC 14?
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3547524681)
As I understand it, this will produce the cross-compile toolchain using GCC 13. Should we wait on this PR until we have GCC 14] (https://github.com/bitcoin/bitcoin/pull/33775) being as CI for UCRT (https://github.com/bitcoin/bitcoin/pull/33764) will be based on Trixie and GCC 14?
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538107778)
> Are you looking for a complete header sync implementation first?
No, that's not necessary. I'm just highlighting that in my view there's a gap between what we're expecting of API changes (having clarity on how the new APIs will be used to achieve goals) and then not document (in PR or commits) how we're expecting the API to be used. Just "validation" seems concise.
Anyway, it seems like I'm bringing stop energy and that was not the goal, so I'll take a break here and perhaps revisit late
...
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538107778)
> Are you looking for a complete header sync implementation first?
No, that's not necessary. I'm just highlighting that in my view there's a gap between what we're expecting of API changes (having clarity on how the new APIs will be used to achieve goals) and then not document (in PR or commits) how we're expecting the API to be used. Just "validation" seems concise.
Anyway, it seems like I'm bringing stop energy and that was not the goal, so I'll take a break here and perhaps revisit late
...
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538120529)
> No, that's not necessary
I think that would be perfectly reasonable as a prerequisite for these kind of deep changes actually. The commit and PR description should be updated too to better explain what is going on with the validation changes here in my view.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538120529)
> No, that's not necessary
I think that would be perfectly reasonable as a prerequisite for these kind of deep changes actually. The commit and PR description should be updated too to better explain what is going on with the validation changes here in my view.