📝 willcl-ark opened a pull request: "Fix lint runner selection (and docker cache)"
(https://github.com/bitcoin/bitcoin/pull/33744)
Fixes: 33735
Correct runner type selection for the lint job.
This was erroneously left-out during refactor of the runner selection mechanism in #33302 causing the lint job to run on GH hosts (and therefore not be able to acces local cirrus caches).
(https://github.com/bitcoin/bitcoin/pull/33744)
Fixes: 33735
Correct runner type selection for the lint job.
This was erroneously left-out during refactor of the runner selection mechanism in #33302 causing the lint job to run on GH hosts (and therefore not be able to acces local cirrus caches).
💬 fanquake commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3467299965)
> we can't expect them to run them on a different machine and not even try them locally.
They always use Podman or Docker, and run a Linux VM, on their macOS machine.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3467299965)
> we can't expect them to run them on a different machine and not even try them locally.
They always use Podman or Docker, and run a Linux VM, on their macOS machine.
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3467320432)
> I tend to think even if this PR is only used to improve getblocktemplate and remove global state [[1]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/rpc/mining.cpp#L776), [[2]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/rpc/mining.cpp#L859-L861), [[3]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/node/miner.h#L186-L189), the changes might be worth it but I'm not sure if you
...
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3467320432)
> I tend to think even if this PR is only used to improve getblocktemplate and remove global state [[1]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/rpc/mining.cpp#L776), [[2]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/rpc/mining.cpp#L859-L861), [[3]](https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/src/node/miner.h#L186-L189), the changes might be worth it but I'm not sure if you
...
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477570759)
> As for `invalidateblock`:... because they should probably do _something_ to get compatible outbound peers...
I don't think that's necessarily true. In the "happy" path use case of `invalidateblock`, a large number of nodes will have done the same, so compatible nodes should be found automatically. Other approaches to prevent connecting with incompatible nodes would of course be better, but we're talking about an emergency here.
> ... and eventually rate-limiting will kick in.
Yes, but
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477570759)
> As for `invalidateblock`:... because they should probably do _something_ to get compatible outbound peers...
I don't think that's necessarily true. In the "happy" path use case of `invalidateblock`, a large number of nodes will have done the same, so compatible nodes should be found automatically. Other approaches to prevent connecting with incompatible nodes would of course be better, but we're talking about an emergency here.
> ... and eventually rate-limiting will kick in.
Yes, but
...
👍 real-or-random approved a pull request: "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)"
(https://github.com/bitcoin/bitcoin/pull/32924#pullrequestreview-3398953286)
utACK https://github.com/bitcoin/bitcoin/pull/32924/commits/5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 interesting case
(https://github.com/bitcoin/bitcoin/pull/32924#pullrequestreview-3398953286)
utACK https://github.com/bitcoin/bitcoin/pull/32924/commits/5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 interesting case
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477594004)
> Or keep on being careful about log levels just as before when the rate limiting didn't exist?
Imo rate limiting changes nothing about how we should think about carefully using log levels. Logs are meant to be informative to the user, and any potential for uninformative entries to flood the log, potentially burying important information, should be avoided. Rate limiting is just a belt-and-suspenders, in my view.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477594004)
> Or keep on being careful about log levels just as before when the rate limiting didn't exist?
Imo rate limiting changes nothing about how we should think about carefully using log levels. Logs are meant to be informative to the user, and any potential for uninformative entries to flood the log, potentially burying important information, should be avoided. Rate limiting is just a belt-and-suspenders, in my view.
💬 stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3467431638)
986eb77 to 3e7d272 - reversed the order of `-regtest` flag and input `DATADIR` as suggested by @TheCharlatan and added simple validation for the flag as suggested by @frankomosh.
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3467431638)
986eb77 to 3e7d272 - reversed the order of `-regtest` flag and input `DATADIR` as suggested by @TheCharlatan and added simple validation for the flag as suggested by @frankomosh.
👍 stickies-v approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3398970603)
ACK 6d2c8ea9dbd77c71051935b5ab59224487509559
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3398970603)
ACK 6d2c8ea9dbd77c71051935b5ab59224487509559
💬 stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477605536)
I think logging the block hash would be helpful here?
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2477605536)
I think logging the block hash would be helpful here?
📝 hebasto converted_to_draft a pull request: "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`"
(https://github.com/bitcoin/bitcoin/pull/33725)
This PR continues the work from bitcoin/bitcoin#31308 by extending the treatment of IWYU warnings as errors to the `src/init` and `src/policy` directories.
(https://github.com/bitcoin/bitcoin/pull/33725)
This PR continues the work from bitcoin/bitcoin#31308 by extending the treatment of IWYU warnings as errors to the `src/init` and `src/policy` directories.
💬 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.