💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2477948499)
Oops, yes I think this must be a holdover from a very early draft of this PR, prior to txgraph. I'll remove.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2477948499)
Oops, yes I think this must be a holdover from a very early draft of this PR, prior to txgraph. I'll remove.
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477956663)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
If keeping changes in this function instead of making the fix in AddMerkleRootAndCoinbase, would be good to s/m_block_template->block/block on this line to be consistent
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477956663)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
If keeping changes in this function instead of making the fix in AddMerkleRootAndCoinbase, would be good to s/m_block_template->block/block on this line to be consistent
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477966256)
re: https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477883143
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
This seems like a really good idea. Having AddMerkleRootAndCoinbase update relevant flags seems like a more robust solution that would have prevented this problem.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477966256)
re: https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477883143
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
This seems like a really good idea. Having AddMerkleRootAndCoinbase update relevant flags seems like a more robust solution that would have prevented this problem.
🤔 TheCharlatan reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3399383695)
Made sure this also doesn't slow down normal block retrieval. There is a bit of extra logic in the hot path for `ReadRawBlock`, but I think this should be fine.
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3399383695)
Made sure this also doesn't slow down normal block retrieval. There is a bit of extra logic in the hot path for `ReadRawBlock`, but I think this should be fine.
💬 TheCharlatan commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2477934300)
Can you add a test case for the error conditions? Maybe I am missing it, but corecheck's coverage report also thinks they are not covered.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2477934300)
Can you add a test case for the error conditions? Maybe I am missing it, but corecheck's coverage report also thinks they are not covered.
💬 willcl-ark commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3467895584)
I think it would make sense to include here.
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3467895584)
I think it would make sense to include here.
📝 maflcko opened a pull request: "ci: Add missing python3-dev package for riscv64"
(https://github.com/bitcoin/bitcoin/pull/33746)
This is required to compile the pip wheels on native riscv64.
(https://github.com/bitcoin/bitcoin/pull/33746)
This is required to compile the pip wheels on native riscv64.
💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467952513)
Otherwise the CI output will look like:
```
+ retry -- pip3 install --break-system-packages pycapnp
Collecting pycapnp
Downloading pycapnp-2.2.1.tar.gz (709 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 709.1/709.1 kB 539.2 kB/s eta 0:00:00
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproj
...
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467952513)
Otherwise the CI output will look like:
```
+ retry -- pip3 install --break-system-packages pycapnp
Collecting pycapnp
Downloading pycapnp-2.2.1.tar.gz (709 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 709.1/709.1 kB 539.2 kB/s eta 0:00:00
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproj
...
💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467963658)
This is based on commit 72511fd02e72b74be11273e97bd7911786a82e54 to avoid the clang-16 ICE, so the CI should now pass on `FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh"`.
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467963658)
This is based on commit 72511fd02e72b74be11273e97bd7911786a82e54 to avoid the clang-16 ICE, so the CI should now pass on `FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh"`.
💬 instagibbs commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3467999278)
I'm generally a -0 on this. I'd rather energy be spent making it easier to fetch "real" estimates, as per your other work?
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3467999278)
I'm generally a -0 on this. I'd rather energy be spent making it easier to fetch "real" estimates, as per your other work?
💬 purpleKarrot commented on pull request "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn":
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468011813)
> * It works around an UB-Sanitizer bug, when the source range is empty.
I guess what you want to express is:
* It has defined semantics for an empty source ranges.
Passing a zero length to `memcpy` is UB per the language standard. This is not an "UB-Sanitizer bug".
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468011813)
> * It works around an UB-Sanitizer bug, when the source range is empty.
I guess what you want to express is:
* It has defined semantics for an empty source ranges.
Passing a zero length to `memcpy` is UB per the language standard. This is not an "UB-Sanitizer bug".
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478021355)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It seems this comment is not actually true if this function is returning false now, or the comment could at least be clarified.
It also seems like just returning false here on failure without error messages could make it difficult for clients to debug issues. Will more specific errors be logged here? Could they be easily returned?
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478021355)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It seems this comment is not actually true if this function is returning false now, or the comment could at least be clarified.
It also seems like just returning false here on failure without error messages could make it difficult for clients to debug issues. Will more specific errors be logged here? Could they be easily returned?
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It's good to mention the block is modified now but it would seem helpful to provide a little more detail here about what is expected and what is modified here especially since IPC behavior is different from RPC.
You also [mentioned](https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299129244) that RPC "This is safe for submitted blocks" documentation isn't really true and it wou
...
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It's good to mention the block is modified now but it would seem helpful to provide a little more detail here about what is expected and what is modified here especially since IPC behavior is different from RPC.
You also [mentioned](https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299129244) that RPC "This is safe for submitted blocks" documentation isn't really true and it wou
...
💬 fanquake commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468059025)
https://github.com/bitcoin/bitcoin/actions/runs/18939943748/job/54075967306?pr=33745#step:9:2351:
```bash
test/miner_tests.cpp(70): Entering test suite "miner_tests"
test/miner_tests.cpp(686): Entering test case "CreateNewBlock_validity"
<snip>
2025-10-30T12:08:21.221330Z [test] [validationinterface.cpp:218] [void ValidationSignals::BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *)] [validation] Enqueuing BlockConnected: block hash=000000000019d66
...
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468059025)
https://github.com/bitcoin/bitcoin/actions/runs/18939943748/job/54075967306?pr=33745#step:9:2351:
```bash
test/miner_tests.cpp(70): Entering test suite "miner_tests"
test/miner_tests.cpp(686): Entering test case "CreateNewBlock_validity"
<snip>
2025-10-30T12:08:21.221330Z [test] [validationinterface.cpp:218] [void ValidationSignals::BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *)] [validation] Enqueuing BlockConnected: block hash=000000000019d66
...
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3468078851)
> What is the plan for adding CI, as that blocks everything here?
I'm working on it. I have issues with the `sqlite` package that are not reproducible neither in the [nightly builds](https://github.com/hebasto/bitcoin-core-nightly/actions/workflows/windows-gcc-ucrt.yml?query=event%3Aschedule) nor locally.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3468078851)
> What is the plan for adding CI, as that blocks everything here?
I'm working on it. I have issues with the `sqlite` package that are not reproducible neither in the [nightly builds](https://github.com/hebasto/bitcoin-core-nightly/actions/workflows/windows-gcc-ucrt.yml?query=event%3Aschedule) nor locally.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478177222)
I move the change away from here.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478177222)
I move the change away from here.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468101541)
@TheCharlatan wrote:
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
Done and pushed, but still checking the CI failure.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468101541)
@TheCharlatan wrote:
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
Done and pushed, but still checking the CI failure.
💬 fanquake commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3468116092)
@frankomosh the PR description needs updating to reflect the current change?
cc @dergoegge @marcofleon
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3468116092)
@frankomosh the PR description needs updating to reflect the current change?
cc @dergoegge @marcofleon
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468117727)
I dropped `Assume(pblock->m_checked_witness_commitment);` from `miner.cpp`, since the assumptions seems to be wrong.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468117727)
I dropped `Assume(pblock->m_checked_witness_commitment);` from `miner.cpp`, since the assumptions seems to be wrong.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478202068)
It behaves the same way as `ProcessNewBlock`, which has the following confusing documentation:
> This only returns after the best known valid
> * block is made active. Note that it does not, however, guarantee that the
> * specific block passed to it has been checked for validity!
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478202068)
It behaves the same way as `ProcessNewBlock`, which has the following confusing documentation:
> This only returns after the best known valid
> * block is made active. Note that it does not, however, guarantee that the
> * specific block passed to it has been checked for validity!