💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2503965416)
I am doing a breaking change here so I deleted it because it is indeed dummy and unused.
But you are right it is unrelated.
I add it in a separate commit.
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2503965416)
I am doing a breaking change here so I deleted it because it is indeed dummy and unused.
But you are right it is unrelated.
I add it in a separate commit.
💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3503007280)
> Just some nits and questions :=)
I force pushed to address the comment, thanks for review @polespinasa
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3503007280)
> Just some nits and questions :=)
I force pushed to address the comment, thanks for review @polespinasa
💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504009667)
Given [sv2-tp](https://github.com/stratum-mining/sv2-tp) and a few others are the users of this interface and they are still in development.
I'd rather have any un-useful methods pruned.
When we release a stable interface then we can start deprecating things to avoid breaking clients (As you mentioned in the tracking issue it is okay to make breaking changes so all these methods should be pruned and clients should update).
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504009667)
Given [sv2-tp](https://github.com/stratum-mining/sv2-tp) and a few others are the users of this interface and they are still in development.
I'd rather have any un-useful methods pruned.
When we release a stable interface then we can start deprecating things to avoid breaking clients (As you mentioned in the tracking issue it is okay to make breaking changes so all these methods should be pruned and clients should update).
💬 maflcko commented on pull request "ci: Extend tidy job to cover kernel code":
(https://github.com/bitcoin/bitcoin/pull/33818#issuecomment-3503088429)
lgtm ACK 5d0a40d607d4d2902cc0372c2c9c6569dfe2d20c
(https://github.com/bitcoin/bitcoin/pull/33818#issuecomment-3503088429)
lgtm ACK 5d0a40d607d4d2902cc0372c2c9c6569dfe2d20c
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504094533)
There's a note in the tracking issue #33777 that we can drop these deprecated methods as soon as there's an important breaking change.
Even for testing it's nice that current clients works against both v30 and master.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504094533)
There's a note in the tracking issue #33777 that we can drop these deprecated methods as soon as there's an important breaking change.
Even for testing it's nice that current clients works against both v30 and master.
💬 ismaelsadeeq commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503164270)
Why is this important?
Just because of shorhorning is not convincing imo, copying the txs is the most work which has to be done and then copy the header again.
Previously, they could access it with a single call, but after what you propose it will requires two calls.
Another downside is that they now have to calculate the block subsidy themselves, whereas previously it was included in the vout value of the first output.
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503164270)
Why is this important?
Just because of shorhorning is not convincing imo, copying the txs is the most work which has to be done and then copy the header again.
Previously, they could access it with a single call, but after what you propose it will requires two calls.
Another downside is that they now have to calculate the block subsidy themselves, whereas previously it was included in the vout value of the first output.
🚀 fanquake merged a pull request: "ci: Extend tidy job to cover kernel code"
(https://github.com/bitcoin/bitcoin/pull/33818)
(https://github.com/bitcoin/bitcoin/pull/33818)
💬 maflcko commented on pull request "validation: reduce persisted UTXO set size by prioritizing positive lookups (RFC)":
(https://github.com/bitcoin/bitcoin/pull/33817#issuecomment-3503205706)
> Skipping BIP30 for those deeply buried blocks
Not sure about this. Wouldn't this mean someone can feed a `-nominimumchainwork` node a bogus chain, so that the node crashes or is stuck irrecoverably on the bogus chain?
Even if it wasn't, I am not sure if touching validation.cpp is worth it for basically a rounding error on overall IBD speed?
(https://github.com/bitcoin/bitcoin/pull/33817#issuecomment-3503205706)
> Skipping BIP30 for those deeply buried blocks
Not sure about this. Wouldn't this mean someone can feed a `-nominimumchainwork` node a bogus chain, so that the node crashes or is stuck irrecoverably on the bogus chain?
Even if it wasn't, I am not sure if touching validation.cpp is worth it for basically a rounding error on overall IBD speed?
💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504171090)
Hmm, do you plan on deprecating it first and then removing it in a later release? On the contrary, I think it should be dropped as soon as we deem it unnecessary. Otherwise, we’ll end up with interface methods that conflict with each other. It would also be easier to review the changes together, rather than keeping them all here with deprecation comments and later deleting them it will save us review cycle.
> Even for testing it's nice that current clients works against both v30 and master.
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504171090)
Hmm, do you plan on deprecating it first and then removing it in a later release? On the contrary, I think it should be dropped as soon as we deem it unnecessary. Otherwise, we’ll end up with interface methods that conflict with each other. It would also be easier to review the changes together, rather than keeping them all here with deprecation comments and later deleting them it will save us review cycle.
> Even for testing it's nice that current clients works against both v30 and master.
...
💬 l0rinc commented on pull request "validation: reduce persisted UTXO set size by prioritizing positive lookups (RFC)":
(https://github.com/bitcoin/bitcoin/pull/33817#issuecomment-3503228588)
> I am not sure if touching validation.cpp is worth it
It's not about IBD necessarily, but reduced disk footprint and adjusting the database to resemble the usage more closely:
> Removing the LevelDB bloom filters slightly speeds up present-key workloads (~11% faster AssumeUTXO load) and reduces the on-disk chainstate size by ~2% because filter blocks are not stored.
(https://github.com/bitcoin/bitcoin/pull/33817#issuecomment-3503228588)
> I am not sure if touching validation.cpp is worth it
It's not about IBD necessarily, but reduced disk footprint and adjusting the database to resemble the usage more closely:
> Removing the LevelDB bloom filters slightly speeds up present-key workloads (~11% faster AssumeUTXO load) and reduces the on-disk chainstate size by ~2% because filter blocks are not stored.
💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3503238765)
Rebased on top of the merged bitcoin/bitcoin#33818.
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3503238765)
Rebased on top of the merged bitcoin/bitcoin#33818.
💬 l0rinc commented on issue "malloc: Failed to allocate segment from range group - out of space":
(https://github.com/bitcoin/bitcoin/issues/33806#issuecomment-3503242687)
Yeah, doing a `git bisect` to figure out when this was introduced:
<details>
<summary>Details</summary>
```
git checkout da6f041e39efaf64a84b748556e321021ec1f756^
...
rm -rf build && cmake -B build -DBUILD_BENCH=OFF -DENABLE_IPC=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo && \
cmake --build build -j$(nproc)
...
2025-11-07T15:33:59Z UpdateTip: new best=0000000000000000002ad1de1011dd699820b20a24411d92cf801c6b5012395d height=497674 version=0x20000000 log2_work=87.590254 tx=278248302 date='2017-12-05T06
...
(https://github.com/bitcoin/bitcoin/issues/33806#issuecomment-3503242687)
Yeah, doing a `git bisect` to figure out when this was introduced:
<details>
<summary>Details</summary>
```
git checkout da6f041e39efaf64a84b748556e321021ec1f756^
...
rm -rf build && cmake -B build -DBUILD_BENCH=OFF -DENABLE_IPC=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo && \
cmake --build build -j$(nproc)
...
2025-11-07T15:33:59Z UpdateTip: new best=0000000000000000002ad1de1011dd699820b20a24411d92cf801c6b5012395d height=497674 version=0x20000000 log2_work=87.590254 tx=278248302 date='2017-12-05T06
...
🤔 dergoegge reviewed a pull request: "doc: reference fuzz coverage steps in quick-start"
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3434708080)
Concept ACK - Seems reasonable to mention this in the actual fuzzing docs.
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3434708080)
Concept ACK - Seems reasonable to mention this in the actual fuzzing docs.
💬 dergoegge commented on pull request "doc: reference fuzz coverage steps in quick-start":
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2504177836)
nit:
```suggestion
For source-based `coverage reports`, see [developer notes](/doc/developer-notes.md#compiling-for-fuzz-coverage).
```
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2504177836)
nit:
```suggestion
For source-based `coverage reports`, see [developer notes](/doc/developer-notes.md#compiling-for-fuzz-coverage).
```
✅ dergoegge closed an issue: "fuzz: Use-of-uninitialized-value in evutil_inet_pton"
(https://github.com/bitcoin/bitcoin/issues/27975)
(https://github.com/bitcoin/bitcoin/issues/27975)
💬 dergoegge commented on issue "fuzz: Use-of-uninitialized-value in evutil_inet_pton":
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503271376)
Closing since
* I'm not sure if this is still a thing
* I don't really care to fix it and clearly nobody else does either
* In any case, it's an upstream issue and the oss fuzz issue exists as well
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503271376)
Closing since
* I'm not sure if this is still a thing
* I don't really care to fix it and clearly nobody else does either
* In any case, it's an upstream issue and the oss fuzz issue exists as well
💬 Sjors commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503279705)
> Previously, they could access it with a single call, but after what you propose it will requires two calls.
Clients are already expect to call `getHeader()` to get the header. Combined with `getCoinbaseMerklePath()` and `getCoinbase()` lets them construct a template without having to download the full block.
In Stratum v2 clients only call `getBlock()` if the pool wants to inspect their proposed template (see `RequestTransactionData.Success` in sv2-tp). This happens later and currently invol
...
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503279705)
> Previously, they could access it with a single call, but after what you propose it will requires two calls.
Clients are already expect to call `getHeader()` to get the header. Combined with `getCoinbaseMerklePath()` and `getCoinbase()` lets them construct a template without having to download the full block.
In Stratum v2 clients only call `getBlock()` if the pool wants to inspect their proposed template (see `RequestTransactionData.Success` in sv2-tp). This happens later and currently invol
...
💬 maflcko commented on issue "fuzz: Use-of-uninitialized-value in evutil_inet_pton":
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503304087)
It was reproducible in our CI config once: https://issues.oss-fuzz.com/issues/42521668#comment5
But yeah, not sure if anyone cares.
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503304087)
It was reproducible in our CI config once: https://issues.oss-fuzz.com/issues/42521668#comment5
But yeah, not sure if anyone cares.
💬 purpleKarrot commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503306025)
My position is that code like this is **not** something that belongs in a `CMakeLists.txt` file.
Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-2999638262).
As long as fun
...
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503306025)
My position is that code like this is **not** something that belongs in a `CMakeLists.txt` file.
Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-2999638262).
As long as fun
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504267008)
> do you plan on deprecating it first and then removing it in a later release?
No, because the interface is marked experimental I think it's fine to drop it in any release we want. That might be as soon as v31, but we can also do it later.
> your master should be in sync with master
`sv2-tp` is used by people who only run v30, so it has to support both master and v30.
It can already handle _new_ methods by just trying them and falling back to old methods when they're missing.
Rec
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504267008)
> do you plan on deprecating it first and then removing it in a later release?
No, because the interface is marked experimental I think it's fine to drop it in any release we want. That might be as soon as v31, but we can also do it later.
> your master should be in sync with master
`sv2-tp` is used by people who only run v30, so it has to support both master and v30.
It can already handle _new_ methods by just trying them and falling back to old methods when they're missing.
Rec
...