💬 maflcko commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3467501417)
lgtm ACK 0b3b8a3be1a0db0dfc634acca1d9305dc0fbfae6
This is now selecting the correct provider.
Some follow-up ideas:
I ran `codex -c model=gpt-5-mini --dangerously-bypass-approvals-and-sandbox exec 'Review the top commit'` on ff18b6bbaf322739fe98fd51b0d89d65a5775ab5 for fun, and it printed:
```
**Potential issue I found**
- In `action.yml` the `cache-provider` `options` list contains `gh`:
- `.github/actions/configure-docker/action.yml:4`
- But the workflow sets `provider=gh
...
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3467501417)
lgtm ACK 0b3b8a3be1a0db0dfc634acca1d9305dc0fbfae6
This is now selecting the correct provider.
Some follow-up ideas:
I ran `codex -c model=gpt-5-mini --dangerously-bypass-approvals-and-sandbox exec 'Review the top commit'` on ff18b6bbaf322739fe98fd51b0d89d65a5775ab5 for fun, and it printed:
```
**Potential issue I found**
- In `action.yml` the `cache-provider` `options` list contains `gh`:
- `.github/actions/configure-docker/action.yml:4`
- But the workflow sets `provider=gh
...
💬 fanquake commented on pull request "depends: sqlite 3.50.4; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3467654930)
Guix Build (aarch64)
```bash
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21bf1d78cd8ebe9719b1 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu.tar.gz
7d210ac
...
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3467654930)
Guix Build (aarch64)
```bash
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21bf1d78cd8ebe9719b1 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu.tar.gz
7d210ac
...
📝 Sjors opened a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745)
When an IPC client requests a new block template via the Mining interface, we hold on to its `CBlock`. That way when they call `submitSolution()` we can modify it in place, rather than having to reconstruct the full block like the `submitblock` RPC does.
Before this commit however we forgot to invalidate `m_checked_witness_commitment`, which we should since the client brings a new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now
...
(https://github.com/bitcoin/bitcoin/pull/33745)
When an IPC client requests a new block template via the Mining interface, we hold on to its `CBlock`. That way when they call `submitSolution()` we can modify it in place, rather than having to reconstruct the full block like the `submitblock` RPC does.
Before this commit however we forgot to invalidate `m_checked_witness_commitment`, which we should since the client brings a new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now
...
💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3467671174)
@ryanofsky wrote:
> 3. New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior is undefined? Would be helpful to indicate.
It turns out we accept such an invalid block. The reason for that is a cached `m_checked_witness_commitment`. Fixed in #33745 which also adds documentation from this PR.
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3467671174)
@ryanofsky wrote:
> 3. New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior is undefined? Would be helpful to indicate.
It turns out we accept such an invalid block. The reason for that is a cached `m_checked_witness_commitment`. Fixed in #33745 which also adds documentation from this PR.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2477858904)
My hesitation to include stricter tests on the chunks is that we currently have no guarantee that our linearizations will be anything more than topological, so it feels brittle to add tests that put an expectation on linearization quality.
Perhaps something like this would make more sense in the context of implementing SFL?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2477858904)
My hesitation to include stricter tests on the chunks is that we currently have no guarantee that our linearizations will be anything more than topological, so it feels brittle to add tests that put an expectation on linearization quality.
Perhaps something like this would make more sense in the context of implementing SFL?
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3467683231)
> second peer can endlessly send CMPCTBLOCK's with 2^16-1 shortid's and the victim node will just keep calling InitData and looping the entire mempool until it gets the block from an honest peer.
Kind of a tangent, but do you have any idea how fast/slow this is if the mempool has ~100k txns?
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3467683231)
> second peer can endlessly send CMPCTBLOCK's with 2^16-1 shortid's and the victim node will just keep calling InitData and looping the entire mempool until it gets the block from an honest peer.
Kind of a tangent, but do you have any idea how fast/slow this is if the mempool has ~100k txns?
💬 TheCharlatan commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477883143)
What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477883143)
What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
💬 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
...