📝 l0rinc converted_to_draft a pull request: "optimization: `CheckBlock` input duplicate detection"
(https://github.com/bitcoin/bitcoin/pull/31682)
`CheckBlock`'s latency is critical for efficiently validating correct inputs during transaction validation, including mempool acceptance and new block creation.
This PR improves performance and maintainability by introducing the following changes:
* Benchmarks now measure `CheckBlock` performance for both valid and invalid cases separately (without serialization cost), ensuring accurate insights into critical paths.
* Simplified checks for the most common cases (1 or 2 inputs - [70-90% of t
...
(https://github.com/bitcoin/bitcoin/pull/31682)
`CheckBlock`'s latency is critical for efficiently validating correct inputs during transaction validation, including mempool acceptance and new block creation.
This PR improves performance and maintainability by introducing the following changes:
* Benchmarks now measure `CheckBlock` performance for both valid and invalid cases separately (without serialization cost), ensuring accurate insights into critical paths.
* Simplified checks for the most common cases (1 or 2 inputs - [70-90% of t
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923806250)
They're always skewed, but if we use actual blocks as basis, at least we can't trick ourselves so easily.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923806250)
They're always skewed, but if we use actual blocks as basis, at least we can't trick ourselves so easily.
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923821802)
> They're always skewed,
Not if we use randomization.
> but if we use actual blocks as basis, at least we can't trick ourselves so easily.
We are just tricking ourselves differently.
After a brief discussion with @Eunovo and @josibake elsewhere I think there should be 3 separate benchmarks: No Schnorr Blocks, Random-mix Blocks and only schnorr blocks. This allows to see the impact of changes on each of these scenarios and the random mix blocks should cover what we currently see on c
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923821802)
> They're always skewed,
Not if we use randomization.
> but if we use actual blocks as basis, at least we can't trick ourselves so easily.
We are just tricking ourselves differently.
After a brief discussion with @Eunovo and @josibake elsewhere I think there should be 3 separate benchmarks: No Schnorr Blocks, Random-mix Blocks and only schnorr blocks. This allows to see the impact of changes on each of these scenarios and the random mix blocks should cover what we currently see on c
...
💬 1440000bytes commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2604902068)
> @1440000bytes, this behavior isn't helpful - you're just reiterating what I've already explained, without suggesting workable solutions.
I don't think optimization is worth changing consensus code.
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2604902068)
> @1440000bytes, this behavior isn't helpful - you're just reiterating what I've already explained, without suggesting workable solutions.
I don't think optimization is worth changing consensus code.
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923846544)
> They're always skewed - Not if we use randomization.
We always introduce bias, it's unavoidable. Let's try to come up with something that minimizes our own preference of how we wish the blocks looked like (as in this PR where the block only contained Schnorrs). We can avoid that by checking the actual usages and relying on them.
> I think there should be 3 separate benchmarks: No Schnorr Blocks, Random-mix Blocks and only schnorr blocks.
This will likely minimize the biases that we i
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923846544)
> They're always skewed - Not if we use randomization.
We always introduce bias, it's unavoidable. Let's try to come up with something that minimizes our own preference of how we wish the blocks looked like (as in this PR where the block only contained Schnorrs). We can avoid that by checking the actual usages and relying on them.
> I think there should be 3 separate benchmarks: No Schnorr Blocks, Random-mix Blocks and only schnorr blocks.
This will likely minimize the biases that we i
...
📝 maflcko opened a pull request: " test: Bump sync_mempools timeout in p2p_1p1c_network.py "
(https://github.com/bitcoin/bitcoin/pull/31701)
This should address the two issues that happened in https://github.com/bitcoin/bitcoin/actions/runs/12885576442/job/35924329657?pr=25832#step:6:7601:
* The combined log isn't printed on a test failure.
* The timeout is too strict for the GHA virtual machines.
For reference, the output was:
```
...
149/315 - rpc_blockchain.py --v2transport passed, Duration: 10 s
150/315 - p2p_addrfetch.py passed, Duration: 1 s
151/315 - p2p_1p1c_network.py failed, Duration: 31 s
stdout:
2025-01-
...
(https://github.com/bitcoin/bitcoin/pull/31701)
This should address the two issues that happened in https://github.com/bitcoin/bitcoin/actions/runs/12885576442/job/35924329657?pr=25832#step:6:7601:
* The combined log isn't printed on a test failure.
* The timeout is too strict for the GHA virtual machines.
For reference, the output was:
```
...
149/315 - rpc_blockchain.py --v2transport passed, Duration: 10 s
150/315 - p2p_addrfetch.py passed, Duration: 1 s
151/315 - p2p_1p1c_network.py failed, Duration: 31 s
stdout:
2025-01-
...
💬 fanquake commented on pull request "depends: Override default build type for `libevent`":
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2604967588)
Guix Build (x86_64):
```bash
22e26035bc1c8484369d855033af86a1209103e9f5c013db27c9a5e88f798de4 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/SHA256SUMS.part
161a7f07ec0bbd8471453249319f179b6478f18b506b837404d6fd661c3790cf guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu-debug.tar.gz
6288a7dfb39d4816b8e43666b7690db2a7fbbcc9077a4b1ec72ecac9804dd8a6 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu.tar.gz
428e0bf
...
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2604967588)
Guix Build (x86_64):
```bash
22e26035bc1c8484369d855033af86a1209103e9f5c013db27c9a5e88f798de4 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/SHA256SUMS.part
161a7f07ec0bbd8471453249319f179b6478f18b506b837404d6fd661c3790cf guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu-debug.tar.gz
6288a7dfb39d4816b8e43666b7690db2a7fbbcc9077a4b1ec72ecac9804dd8a6 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu.tar.gz
428e0bf
...
💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1923883932)
Mostly to mock `Wait` iirc. I'll reproduce your fuzz finding now and look into this. Thanks!
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1923883932)
Mostly to mock `Wait` iirc. I'll reproduce your fuzz finding now and look into this. Thanks!
💬 dergoegge commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1923890415)
I see, perhaps you can just override `Wait` of `FuzzedSock` then? Or the time mocking could also just be a part of `FuzzedSock` (seems useful for all harnesses).
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1923890415)
I see, perhaps you can just override `Wait` of `FuzzedSock` then? Or the time mocking could also just be a part of `FuzzedSock` (seems useful for all harnesses).
💬 glozow commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2604985835)
If that's the case, it's best to reopen this only when there is (1) more conceptual support and (2) you've had the time to add tests and fix CI. As per the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#decision-making-process), PRs are expected to have test coverage and not break existing tests.
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2604985835)
If that's the case, it's best to reopen this only when there is (1) more conceptual support and (2) you've had the time to add tests and fix CI. As per the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#decision-making-process), PRs are expected to have test coverage and not break existing tests.
💬 fanquake commented on pull request "depends: Override default build type for `libevent`":
(https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475)
Something that should probably be followed up with, given that most coverage builds are using depends, is the fact that `-ffile-prefix-map` keeps being hardcoded into depends, while CMakelists.txt says:
```bash
# Avoiding the `-ffile-prefix-map` compiler option because it implies
# `-fcoverage-prefix-map` on Clang or `-fprofile-prefix-map` on GCC,
# which can cause issues with coverage builds,
```
Might be less of an issue given these are deps but it's at least inconsistent.
(https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475)
Something that should probably be followed up with, given that most coverage builds are using depends, is the fact that `-ffile-prefix-map` keeps being hardcoded into depends, while CMakelists.txt says:
```bash
# Avoiding the `-ffile-prefix-map` compiler option because it implies
# `-fcoverage-prefix-map` on Clang or `-fprofile-prefix-map` on GCC,
# which can cause issues with coverage builds,
```
Might be less of an issue given these are deps but it's at least inconsistent.
🤔 glozow reviewed a pull request: "test: Bump sync_mempools timeout in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/pull/31701#pullrequestreview-2564986589)
ACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
Makes sense to me. There are 4 nodes on p2p_1p1c_network, so makes sense that it could take longer than 20sec for everything to propagate. Default is 60 iirc?
(https://github.com/bitcoin/bitcoin/pull/31701#pullrequestreview-2564986589)
ACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
Makes sense to me. There are 4 nodes on p2p_1p1c_network, so makes sense that it could take longer than 20sec for everything to propagate. Default is 60 iirc?
💬 l0rinc commented on pull request "test: Bump sync_mempools timeout in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/pull/31701#issuecomment-2605067663)
utACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
Same happened in https://github.com/bitcoin/bitcoin/actions/runs/12888898958/job/35934893317?pr=31699
(https://github.com/bitcoin/bitcoin/pull/31701#issuecomment-2605067663)
utACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
Same happened in https://github.com/bitcoin/bitcoin/actions/runs/12888898958/job/35934893317?pr=31699
💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923955156)
I've updated the PR to use 3 blocks
- all schnorr
- 50% schnorr, 50% ecdsa
- all ecdsa
If we used one block from Mainnet here, we would optimize for that block. This setup of 3 blocks is more balanced. We can always do an actual IBD benchmark or use tools like [Benchcoin](https://github.com/bitcoin-dev-tools/benchcoin) cc @josibake
Connecting random blocks from Mainnet is tricky. At best, I would only be able to use the Mainnet block as a template to create the blocks for the benchmark
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923955156)
I've updated the PR to use 3 blocks
- all schnorr
- 50% schnorr, 50% ecdsa
- all ecdsa
If we used one block from Mainnet here, we would optimize for that block. This setup of 3 blocks is more balanced. We can always do an actual IBD benchmark or use tools like [Benchcoin](https://github.com/bitcoin-dev-tools/benchcoin) cc @josibake
Connecting random blocks from Mainnet is tricky. At best, I would only be able to use the Mainnet block as a template to create the blocks for the benchmark
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923957793)
As mentioned earlier, I would prefer basing the constants on data derived from actual usage. The current approach feels arbitrary and directly impacts the outcome.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923957793)
As mentioned earlier, I would prefer basing the constants on data derived from actual usage. The current approach feels arbitrary and directly impacts the outcome.
💬 Sjors commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2605081704)
> Would have been nice to have tests here but fine for me to keep it for a follow-up
#29675 introduces `wallet_musig.py` which uses PSBT internally. IIUC all new fields are needed for MuSig2 to work at all, so they're tested implicitly. Additionally the test inspects `musig2_participant_pubkeys` and `musig2_partial_sigs`, though not `musig2_pubnonces`.
That said, I think it would be better if this PR introduced at least some basic test coverage for e.g. the new errors that are introduced.
...
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2605081704)
> Would have been nice to have tests here but fine for me to keep it for a follow-up
#29675 introduces `wallet_musig.py` which uses PSBT internally. IIUC all new fields are needed for MuSig2 to work at all, so they're tested implicitly. Additionally the test inspects `musig2_participant_pubkeys` and `musig2_partial_sigs`, though not `musig2_pubnonces`.
That said, I think it would be better if this PR introduced at least some basic test coverage for e.g. the new errors that are introduced.
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923964274)
> we would optimize for that block
So what are we optimizing for now?
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923964274)
> we would optimize for that block
So what are we optimizing for now?
💬 Sjors commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1923997661)
433eca29933447b70feba61fa26d3e6345fb3115: if `s_val` has less than 33 bytes remaining, this throws?
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1923997661)
433eca29933447b70feba61fa26d3e6345fb3115: if `s_val` has less than 33 bytes remaining, this throws?
💬 theuni commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1923995451)
> to allow the compiler to optimize for the case where paths of execution including that statement are less likely than _**any alternative path**_ of execution that does not include such a statement
See here for why this is probably a bad idea: https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/
tl;dr: It's impl-defined whether or not this makes the parent `if(tx.vin.size() == 2)` unlikely as well.
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1923995451)
> to allow the compiler to optimize for the case where paths of execution including that statement are less likely than _**any alternative path**_ of execution that does not include such a statement
See here for why this is probably a bad idea: https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/
tl;dr: It's impl-defined whether or not this makes the parent `if(tx.vin.size() == 2)` unlikely as well.
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924039967)
Picking a random block is just as arbitrary and it would make sense to base the numbers on an outlook for the future rather than looking backwards. I think 50/50 is good as a starting point. I'm sure there has been a block in past that had exactly 50% schnorr sigs, so this argument about using a specific block is pointless. If you want to suggest a different split based on the past I would suggest taking a larger range of blocks and deriving a very specific split from that. Like over the last ye
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924039967)
Picking a random block is just as arbitrary and it would make sense to base the numbers on an outlook for the future rather than looking backwards. I think 50/50 is good as a starting point. I'm sure there has been a block in past that had exactly 50% schnorr sigs, so this argument about using a specific block is pointless. If you want to suggest a different split based on the past I would suggest taking a larger range of blocks and deriving a very specific split from that. Like over the last ye
...