💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533615466)
I don't think miners use the full `scriptSig` so if we slightly go over it, it shouldn't immediately result in invalid blocks. So it seems overkill to abort the template generation entirely.
I'll log a warning.
The main purpose of this `Assume` is to remind ourselves to inform the mining ecosystem if we ever expand the scriptSig prefix substantially.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533615466)
I don't think miners use the full `scriptSig` so if we slightly go over it, it shouldn't immediately result in invalid blocks. So it seems overkill to abort the template generation entirely.
I'll log a warning.
The main purpose of this `Assume` is to remind ourselves to inform the mining ecosystem if we ever expand the scriptSig prefix substantially.
🚀 fanquake merged a pull request: "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247)
(https://github.com/bitcoin/bitcoin/pull/33247)
👋 fanquake's pull request is ready for review: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)
(https://github.com/bitcoin/bitcoin/pull/33851)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533642433)
I also clarified the comment that it might be a _silently_ breaking change. Although the spec requires `NewTemplate` to keep it at max 8 bytes, the serialisation supports up to 256 and a Template Provider implementation might just not check / enforce this.
https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533642433)
I also clarified the comment that it might be a _silently_ breaking change. Although the spec requires `NewTemplate` to keep it at max 8 bytes, the serialisation supports up to 256 and a Template Provider implementation might just not check / enforce this.
https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533657576)
I'll go for the latter approach.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533657576)
I'll go for the latter approach.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075)
The BIP says:
> For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.
So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075)
The BIP says:
> For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.
So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".
💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533700190)
Went with separate counters and also added the selected network to the log output for clarity.
Example log entry:
```
[addrman] GetAddr returned 62823 random addresses from ipv4; 1104 filtered (0 network, 0 quality, 1104 policy)
[addrman] GetAddr returned 0 random addresses from ipv6; 63927 filtered (63927 network, 0 quality, 0 policy)
[addrman] GetAddr returned 0 random addresses from onion; 63927 filtered (66927 network, 0 quality, 0 policy)
```
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533700190)
Went with separate counters and also added the selected network to the log output for clarity.
Example log entry:
```
[addrman] GetAddr returned 62823 random addresses from ipv4; 1104 filtered (0 network, 0 quality, 1104 policy)
[addrman] GetAddr returned 0 random addresses from ipv6; 63927 filtered (63927 network, 0 quality, 0 policy)
[addrman] GetAddr returned 0 random addresses from onion; 63927 filtered (66927 network, 0 quality, 0 policy)
```
💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533706305)
Updated comments for more clarity
in addrman.h:
```cpp
/** Predicate used to exclude addresses during selection.
* Return true to skip the given address.
*
* Runs while holding AddrMan::cs, so it must be non-blocking, must not
* attempt to reacquire AddrMan::cs, and must preserve lock ordering to
* avoid deadlocks.
*/
```
in net.cpp -> GetAddressesUnsafe()
```cpp
// This runs under AddrMan::cs, and BanMan checks take m_banned_mutex.
...
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533706305)
Updated comments for more clarity
in addrman.h:
```cpp
/** Predicate used to exclude addresses during selection.
* Return true to skip the given address.
*
* Runs while holding AddrMan::cs, so it must be non-blocking, must not
* attempt to reacquire AddrMan::cs, and must preserve lock ordering to
* avoid deadlocks.
*/
```
in net.cpp -> GetAddressesUnsafe()
```cpp
// This runs under AddrMan::cs, and BanMan checks take m_banned_mutex.
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533708792)
I'll drop `input_`
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533708792)
I'll drop `input_`
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2533714543)
Yeah, I don't think we can completely prevent UB here, just do the best and least surprising thing we can.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2533714543)
Yeah, I don't think we can completely prevent UB here, just do the best and least surprising thing we can.
💬 fanquake commented on pull request "refactor: Avoid -W*-whitespace in git archive":
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3541325190)
ACK fa95353902b7a6f73f094e78106088ab3c16ce14
```cpp
// git will expand the next line to "#define GIT_COMMIT_ID ..." inside archives:
//
#define GIT_COMMIT_ID "84f39b282de0836ca5ad6166daafecf15e4c813d"
```
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3541325190)
ACK fa95353902b7a6f73f094e78106088ab3c16ce14
```cpp
// git will expand the next line to "#define GIT_COMMIT_ID ..." inside archives:
//
#define GIT_COMMIT_ID "84f39b282de0836ca5ad6166daafecf15e4c813d"
```
🚀 fanquake merged a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869)
(https://github.com/bitcoin/bitcoin/pull/33869)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177)
That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177)
That seems incorrect, because a pool (like Ocean or P2pool) might let you take part of the reward directly in an output.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877)
I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.
That heuristic won't work anymore if we add a way for clients to specify custom outputs when constructing a block, so it's important to document.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533740877)
I added documentation there, but we also need it here. The `output` field is not limited to `OP_RETURN`, that's just a heuristic used by `ExtractCoinbaseTemplate`.
That heuristic won't work anymore if we add a way for clients to specify custom outputs when constructing a block, so it's important to document.
🤔 stickies-v reviewed a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877#pullrequestreview-3472331847)
Hmm, I think I'm ~0 on this, leaning towards Concept NACK. Removing unnecessary parameters is good, but I personally find an interface more ergonomic when you can call `object.set_bool(value)` instead of `if (value) object.set_bool()`. It generalizes better, and makes using the object more flexible. For example, if `value` depends on multiple factors, with the last one taking priority, the value can be updated multiple times with `object.set_bool(value)` without needing extra state.
In the ca
...
(https://github.com/bitcoin/bitcoin/pull/33877#pullrequestreview-3472331847)
Hmm, I think I'm ~0 on this, leaning towards Concept NACK. Removing unnecessary parameters is good, but I personally find an interface more ergonomic when you can call `object.set_bool(value)` instead of `if (value) object.set_bool()`. It generalizes better, and makes using the object more flexible. For example, if `value` depends on multiple factors, with the last one taking priority, the value can be updated multiple times with `object.set_bool(value)` without needing extra state.
In the ca
...
💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533769750)
It wouldn't lose the witness section @frankomosh it would just be rolled up. It would still print, it would just print in the txIn instead. However i can breaking such a fundamental log would be a problem so I'm okay with making the change less intrusive
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533769750)
It wouldn't lose the witness section @frankomosh it would just be rolled up. It would still print, it would just print in the txIn instead. However i can breaking such a fundamental log would be a problem so I'm okay with making the change less intrusive
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625)
Changed to "minus any non-zero required outputs" and a comment to point out that those don't exist atm.
See https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177 above, I think having non-zero outputs is quite conceivable and it would be a shame if that requires another breaking change.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533772625)
Changed to "minus any non-zero required outputs" and a comment to point out that those don't exist atm.
See https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533732177 above, I think having non-zero outputs is quite conceivable and it would be a shame if that requires another breaking change.
👍 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
...