👍 willcl-ark approved a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776#pullrequestreview-3472377000)
ACK fae3618fd6c82dfcea2f296caa16a79182b32059
This all seems fine, and I agree python is superior to bash where possible. It is indeed annoying that GH doesn't persist the annotations, but after also checking myself I can't think of any better way to do it either.
(https://github.com/bitcoin/bitcoin/pull/33776#pullrequestreview-3472377000)
ACK fae3618fd6c82dfcea2f296caa16a79182b32059
This all seems fine, and I agree python is superior to bash where possible. It is indeed annoying that GH doesn't persist the annotations, but after also checking myself I can't think of any better way to do it either.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541552964)
Rebased after #33745.
To keep this PR focussed I removed mention of deprecation. I have a branch [2025/11/coinbase-template-break](https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2025/11/coinbase-template-break) that shows what I have in mind for deprecating these methods and clearing the dummy coinbase. The first commit is non-breaking, so I'll PR that as a followup. I plan to integrate the rest of the branch into https://github.com/Sjors/bitcoin/pull/106, which is a collec
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541552964)
Rebased after #33745.
To keep this PR focussed I removed mention of deprecation. I have a branch [2025/11/coinbase-template-break](https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2025/11/coinbase-template-break) that shows what I have in mind for deprecating these methods and clearing the dummy coinbase. The first commit is non-breaking, so I'll PR that as a followup. I plan to integrate the rest of the branch into https://github.com/Sjors/bitcoin/pull/106, which is a collec
...
📝 maflcko opened a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886)
Also, add a test for creating a CScript from an empty byte vector.
To test: `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DBUILD_KERNEL_LIB=ON -DENABLE_WALLET=OFF -DENABLE_IPC=OFF && cmake --build ./bld-cmake --parallel $( nproc ) && valgrind --tool=none ./bld-cmake/bin/test_kernel --catch_system_error=no`
B
...
(https://github.com/bitcoin/bitcoin/pull/33886)
Also, add a test for creating a CScript from an empty byte vector.
To test: `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DBUILD_KERNEL_LIB=ON -DENABLE_WALLET=OFF -DENABLE_IPC=OFF && cmake --build ./bld-cmake --parallel $( nproc ) && valgrind --tool=none ./bld-cmake/bin/test_kernel --catch_system_error=no`
B
...
💬 maflcko commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2533874874)
removed the tests for now in https://github.com/bitcoin/bitcoin/pull/33886
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2533874874)
removed the tests for now in https://github.com/bitcoin/bitcoin/pull/33886
👍 TheCharlatan approved a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472498582)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472498582)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15
👍 laanwj approved a pull request: "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3472503057)
Code review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a
These are some straightforward changes that even make sense outside the context of embedded ASmap.
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3472503057)
Code review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a
These are some straightforward changes that even make sense outside the context of embedded ASmap.
💬 willcl-ark commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3541623380)
> ;; --enable-static-nss isn't used yet, because it has been broken
> ;; since 2.33: https://sourceware.org/bugzilla/show_bug.cgi?id=27959.
What are the exact implications of this? I guix-built this branch and loaded it into a scratch docker container and the dns seeds were connected to and loaded fine. Is this coming from my host system perhaps, even inside a scratch container?
I also tested the binary on alpine and it appeared to "fallback" to using libnss without issue...
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3541623380)
> ;; --enable-static-nss isn't used yet, because it has been broken
> ;; since 2.33: https://sourceware.org/bugzilla/show_bug.cgi?id=27959.
What are the exact implications of this? I guix-built this branch and loaded it into a scratch docker container and the dns seeds were connected to and loaded fine. Is this coming from my host system perhaps, even inside a scratch container?
I also tested the binary on alpine and it appeared to "fallback" to using libnss without issue...
🚀 fanquake merged a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776)
(https://github.com/bitcoin/bitcoin/pull/33776)
💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541646050)
Good point! I understand your rationale, but wouldn't the cases you're describing be mostly for debugging/testing purposes?
It seems to me that for typical use cases, we would know whether to enable in-memory mode at compile time, not something that is derived dynamically, so it feels more natural/clear to simply call a function that does that with no input parameter.
Also, having an `update(bool_value)` function might indicate that we're supporting some meaningful toggleable functionality, w
...
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541646050)
Good point! I understand your rationale, but wouldn't the cases you're describing be mostly for debugging/testing purposes?
It seems to me that for typical use cases, we would know whether to enable in-memory mode at compile time, not something that is derived dynamically, so it feels more natural/clear to simply call a function that does that with no input parameter.
Also, having an `update(bool_value)` function might indicate that we're supporting some meaningful toggleable functionality, w
...
💬 laanwj commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3541671065)
> The first three PRs were already part of https://github.com/bitcoin/bitcoin/pull/28792, the others are new.
i'm sure you mean the first three *commits*? 😄
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3541671065)
> The first three PRs were already part of https://github.com/bitcoin/bitcoin/pull/28792, the others are new.
i'm sure you mean the first three *commits*? 😄
💬 stickies-v commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3541688045)
Concept ACK, but a little more information in the PR description would be helpful, it's hard to parse what expected behaviour is - both from the CI, as well as from developers. Does passing CI become mandatory? Are we duplicating CI runs or moving it to a separate iwyu job? Should developers change anything in their workflow? Linking the previous discussion is helpful, but doesn't necessarily describe what's adopted in this PR.
Also, style preferences wrt includes should be added to `develope
...
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3541688045)
Concept ACK, but a little more information in the PR description would be helpful, it's hard to parse what expected behaviour is - both from the CI, as well as from developers. Does passing CI become mandatory? Are we duplicating CI runs or moving it to a separate iwyu job? Should developers change anything in their workflow? Linking the previous discussion is helpful, but doesn't necessarily describe what's adopted in this PR.
Also, style preferences wrt includes should be added to `develope
...
🤔 yuvicc reviewed a pull request: "kernel: add btck_block_tree_entry_equals"
(https://github.com/bitcoin/bitcoin/pull/33855#pullrequestreview-3472582923)
Code Review ACK 096924d39d644acc826cbffd39bb34038ecee6cd
(https://github.com/bitcoin/bitcoin/pull/33855#pullrequestreview-3472582923)
Code Review ACK 096924d39d644acc826cbffd39bb34038ecee6cd
💬 stickies-v commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541746178)
> wouldn't the cases you're describing be mostly for debugging/testing purposes ... we would know whether to enable in-memory mode at compile time
I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.
> Also, having an update(bool_value) function might indicate that we're supporting some meaningful toggleable functionality, while in reality once we construct the chainman, changing these options has no effect.
The same issue exist with either app
...
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541746178)
> wouldn't the cases you're describing be mostly for debugging/testing purposes ... we would know whether to enable in-memory mode at compile time
I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.
> Also, having an update(bool_value) function might indicate that we're supporting some meaningful toggleable functionality, while in reality once we construct the chainman, changing these options has no effect.
The same issue exist with either app
...
💬 hodlinator commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2534011313)
Will try to come up with something in case I re-touch.
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup, or throws an exception and disconnects us
```
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2534011313)
Will try to come up with something in case I re-touch.
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup, or throws an exception and disconnects us
```
💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2534055408)
But if `state` contained anything interesting, wouldn't we already have returned it before reaching this block?
Callers of the function only see either `bg_state` or `state` as this PR stands anyway.
Thanks for taking my other suggestions!
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2534055408)
But if `state` contained anything interesting, wouldn't we already have returned it before reaching this block?
Callers of the function only see either `bg_state` or `state` as this PR stands anyway.
Thanks for taking my other suggestions!
👍 stickies-v approved a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472700020)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15
It's unfortunate that we can't test the nullptr + non-zero length paths through the c++ wrapper, but removing these cases seems like the best approach for now.
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472700020)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15
It's unfortunate that we can't test the nullptr + non-zero length paths through the c++ wrapper, but removing these cases seems like the best approach for now.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541868920)
Fixed MSan uninitialised `coinbase` complaint, which should also fix the `coinbase.value_remaining == 0' failure.
I also switched `coinbase.version` from using `CURRENT_VERSION` to `version`.
Updated some comments above to make it clear that this PR does deprecate `getCoinbaseTx()` and `getWitnessCommitmentIndex()` and an additional remark about `getCoinbaseCommitment()`.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541868920)
Fixed MSan uninitialised `coinbase` complaint, which should also fix the `coinbase.value_remaining == 0' failure.
I also switched `coinbase.version` from using `CURRENT_VERSION` to `version`.
Updated some comments above to make it clear that this PR does deprecate `getCoinbaseTx()` and `getWitnessCommitmentIndex()` and an additional remark about `getCoinbaseCommitment()`.
👋 Sjors's pull request is ready for review: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819)
(https://github.com/bitcoin/bitcoin/pull/33819)
💬 maflcko commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3541894978)
I presume the same fails upstream, without guix: https://github.com/workhorsy/py-cpuinfo/blame/f3f0fec58335b9699b9b294267c15f516045b1fe/tests/test_actual.py#L74
Though, upsteam is unmaintained, so i am not sure what can be done here.
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3541894978)
I presume the same fails upstream, without guix: https://github.com/workhorsy/py-cpuinfo/blame/f3f0fec58335b9699b9b294267c15f516045b1fe/tests/test_actual.py#L74
Though, upsteam is unmaintained, so i am not sure what can be done here.
💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541912895)
> I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.
Of course. I meant that maybe the case where we would "dynamically" decide to use in-memory mode wouldn't be that common.
I found the approach in this PR to be a slight improvement for the common use cases, making it a bit more straightforward. But I understand if you find the dynamic use cases more common than I do. Thanks for the feedback!
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541912895)
> I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.
Of course. I meant that maybe the case where we would "dynamically" decide to use in-memory mode wouldn't be that common.
I found the approach in this PR to be a slight improvement for the common use cases, making it a bit more straightforward. But I understand if you find the dynamic use cases more common than I do. Thanks for the feedback!