💬 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.
💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538138138)
Makes sense @TheCharlatan @stickies-v, I will update the PR description for better understanding of the changes here.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2538138138)
Makes sense @TheCharlatan @stickies-v, I will update the PR description for better understanding of the changes here.
💬 hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2538144538)
Taken in latest push (wrapped at 80 chars).
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2538144538)
Taken in latest push (wrapped at 80 chars).
💬 janb84 commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3547578829)
> while moving from cirrus-ci to GHA.
Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ? see :
https://github.com/bitcoin/bitcoin/actions/runs/19437220743/job/55611173427?pr=33888#step:1:2
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3547578829)
> while moving from cirrus-ci to GHA.
Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ? see :
https://github.com/bitcoin/bitcoin/actions/runs/19437220743/job/55611173427?pr=33888#step:1:2
💬 maflcko commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3547591517)
> > while moving from cirrus-ci to GHA.
>
> Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ?
No, it is about the platform switch from `cirrus-ci.com` to the GHA platform, which goes through `github.com`. The GHA runner type should be unrelated.
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3547591517)
> > while moving from cirrus-ci to GHA.
>
> Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ?
No, it is about the platform switch from `cirrus-ci.com` to the GHA platform, which goes through `github.com`. The GHA runner type should be unrelated.
🤔 janb84 reviewed a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3477801343)
ACK 552eb90071fd246ba40037f74329403b72453047
changes since last ACK:
- grammar changes.
Thanks for incorporating my NIT.
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3477801343)
ACK 552eb90071fd246ba40037f74329403b72453047
changes since last ACK:
- grammar changes.
Thanks for incorporating my NIT.
💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3547611060)
Thanks for the review @stickies-v
- Address usage of `LogError` instead of `LogDebug` [comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534865290)
- Addressed some nits and removed docstrings.
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3547611060)
Thanks for the review @stickies-v
- Address usage of `LogError` instead of `LogDebug` [comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2534865290)
- Addressed some nits and removed docstrings.
💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#issuecomment-3547619341)
lgtm ACK 552eb90071fd246ba40037f74329403b72453047
(https://github.com/bitcoin/bitcoin/pull/33887#issuecomment-3547619341)
lgtm ACK 552eb90071fd246ba40037f74329403b72453047
💬 laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3547626871)
> Linux 5.10.113-scw1 #1 SMP PREEMPT Mon Jul 15 10:10:04 UTC 2024
It looks like the kernel is fixed and provided externally. It doesn't actually use one from the distro.
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3547626871)
> Linux 5.10.113-scw1 #1 SMP PREEMPT Mon Jul 15 10:10:04 UTC 2024
It looks like the kernel is fixed and provided externally. It doesn't actually use one from the distro.
💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3547653722)
Thanks for restarting the build @maflcko.
@achow101, I can reproduce the same locally, even on Clang 22 using libstdc++ 15.
<details>
<summary>Details</summary>
```
[183/612] Building CXX object src/CMakeFiles/bitcoin_node.dir/node/txorphanage.cpp.o 10:44:13 [1552/3036]
FAILED: src/CMakeFiles/bitcoin_node.di
...
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3547653722)
Thanks for restarting the build @maflcko.
@achow101, I can reproduce the same locally, even on Clang 22 using libstdc++ 15.
<details>
<summary>Details</summary>
```
[183/612] Building CXX object src/CMakeFiles/bitcoin_node.dir/node/txorphanage.cpp.o 10:44:13 [1552/3036]
FAILED: src/CMakeFiles/bitcoin_node.di
...
💬 Sjors commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3547680115)
Ok, keep us posted on what you learn.
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3547680115)
Ok, keep us posted on what you learn.
📝 waketraindev opened a pull request: "net: Decouple `CConnman::GetAddresses` from `CNode`"
(https://github.com/bitcoin/bitcoin/pull/33900)
Decouple `CConnman::GetAddresses` from `CNode` which was used only for getting network_key
(https://github.com/bitcoin/bitcoin/pull/33900)
Decouple `CConnman::GetAddresses` from `CNode` which was used only for getting network_key