fix: update BuildTestVectors call to adjust batch size based on output flag by PastaPastaPasta · Pull Request #7013 · dashpay/dash

PastaPastaPasta · 2025-11-26T21:49:46Z

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

runs locally; but I didn't reproduce the failure, likely due to different compilation flags. Even with undefined sanitizer, I couldn't reproduce.

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

…t flag

github-actions · 2025-11-26T21:50:21Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-11-26T21:54:14Z

Walkthrough

Two benchmark function calls in src/bench/bls.cpp have been modified to conditionally adjust the invalidCount parameter passed to BuildTestVectors. In both BLS_Verify_Batched and BLS_Verify_BatchedParallel, the invalidCount is now set to 10 when benchmark output is enabled (via bench.output()), and 1 when disabled. All other behavior remains unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Single file modified with two identical, homogeneous conditional parameter adjustments
No logic changes, control flow modifications, or public API alterations
Simple ternary operator applied consistently across both call sites

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description check	❓ Inconclusive	The description references an issue but lacks substantive details about what was done, making it difficult to assess completeness.	Provide implementation details explaining why the batch size adjustment was necessary and how it addresses the referenced issue.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: updating BuildTestVectors calls to adjust batch size based on an output flag, which directly corresponds to the changeset.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b9c29d and b220dec.

📒 Files selected for processing (1)

src/bench/bls.cpp (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,hpp,cc}

📄 CodeRabbit inference engine (CLAUDE.md)

Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1

Files:

src/bench/bls.cpp

src/bench/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

Performance benchmarks in src/bench/ must use the nanobench library

Files:

src/bench/bls.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

📚 Learning: 2025-11-24T16:41:22.457Z

Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features

Applied to files:

src/bench/bls.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: linux64_tsan-build / Build source
GitHub Check: Lint / Run linters
GitHub Check: x86_64-w64-mingw32 / Build depends
GitHub Check: x86_64-apple-darwin / Build depends
GitHub Check: x86_64-pc-linux-gnu / Build depends
GitHub Check: arm-linux-gnueabihf / Build depends
GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends

🔇 Additional comments (2)

src/bench/bls.cpp (2)

251-259: Fixes invalidCount > count UB in BLS_Verify_Batched.

Tying both count and invalidCount to bench.output() ensures that when bench.output() is false you no longer pass (1, 10) into BuildTestVectors, which previously caused out-of-bounds writes to invalid[i]. With (1, 1) you still exercise an invalid vector path without invoking UB, and behavior for the verbose case (1000, 10) is preserved.

307-315: Parallel batched benchmark receives the same UB fix and stays consistent.

Applying the same (bench.output() ? 1000 : 1, bench.output() ? 10 : 1) pattern here removes the same potential invalidCount > count issue in the parallel benchmark and keeps the test-vector mix consistent with the non‑parallel version. The duplication between the two call sites looks fine given the small expression.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

UdjinM6

utACK b220dec

… size based on output flag b220dec fix: update BuildTestVectors call to adjust batch size based on output flag (pasta) Pull request description: ## Issue being fixed or feature implemented see dashpay#7011 ## What was done? ## How Has This Been Tested? runs locally; but I didn't reproduce the failure, likely due to different compilation flags. Even with undefined sanitizer, I couldn't reproduce. ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b220dec Tree-SHA512: 5cfb630c576abf7f019348fd4acd878ab44485bd0b658521660259ed1ea0570b46eafbbd783bc58f2b707d549f6daf74e1c8f16a636b597d1c3e9aab386f5722

736bb26 chore: bump manpages for 23.0.1 (pasta) d5c7d25 chore: bump nMinimumChainWork and defaultAssumeValid (pasta) 4f8aa71 chore: bump version to 23.0.1 (pasta) 0865b7c docs: add release notes for 23.0.1 (pasta) 2048b42 Merge #6986: test: new commandline argument -tinyblk to use blk size just 64kb instead 16Mb (pasta) 1a9b20c Merge #7013: fix: update BuildTestVectors call to adjust batch size based on output flag (pasta) 36e4679 Merge #7009: fix: include QDebug directly (pasta) 69d0c9c Merge #6999: feat: verify and repair evodb diffs automatically at node startup (pasta) ca16437 Merge #6996: perf: reduce cs_main lock scope in evodb verify/repair operations (pasta) 207526e Merge #6977: fix: bls benchmarks crash when ran independently (pasta) 226aaf4 Merge #6969: feat: add evodb verify and repair RPC commands (pasta) 92abe9b Merge #6964: perf: remove duplicated check of same key in the instant send database (pasta) 5a1ec4c Merge #6961: fix: correct BLS scheme setting in `MigrateLegacyDiffs()` when `nVersion` is present (pasta) bf653d3 Merge #6949: depends: Qt 5.15.18 (pasta) faf58cd merge bitcoin#30774: Qt 5.15.16 (Kittywhiskers Van Gogh) 6a995f5 Merge #6944: fix: HD chain encryption check ordering issue (pasta) Pull request description: ## Issue being fixed or feature implemented See commits; release v23.0.1 ## What was done? see commits ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 736bb26 Tree-SHA512: fe02c4c3e520b11af54d09c47bda94112313456f9f1cb6d14e78ef16704b2a8ec8feb80fa914c55e152db2bcac7f278291824aaa36c8079eb9e0c9bff9e554a4

fe1cff3 chore: bump release to 23.0.2 (pasta) a8f15c1 Merge #7032: fix: drop gsl usage from RebuildListFromBlock function wrapper (pasta) 736bb26 chore: bump manpages for 23.0.1 (pasta) d5c7d25 chore: bump nMinimumChainWork and defaultAssumeValid (pasta) 4f8aa71 chore: bump version to 23.0.1 (pasta) 0865b7c docs: add release notes for 23.0.1 (pasta) 2048b42 Merge #6986: test: new commandline argument -tinyblk to use blk size just 64kb instead 16Mb (pasta) 1a9b20c Merge #7013: fix: update BuildTestVectors call to adjust batch size based on output flag (pasta) 36e4679 Merge #7009: fix: include QDebug directly (pasta) 69d0c9c Merge #6999: feat: verify and repair evodb diffs automatically at node startup (pasta) ca16437 Merge #6996: perf: reduce cs_main lock scope in evodb verify/repair operations (pasta) 207526e Merge #6977: fix: bls benchmarks crash when ran independently (pasta) 226aaf4 Merge #6969: feat: add evodb verify and repair RPC commands (pasta) 92abe9b Merge #6964: perf: remove duplicated check of same key in the instant send database (pasta) 5a1ec4c Merge #6961: fix: correct BLS scheme setting in `MigrateLegacyDiffs()` when `nVersion` is present (pasta) bf653d3 Merge #6949: depends: Qt 5.15.18 (pasta) faf58cd merge bitcoin#30774: Qt 5.15.16 (Kittywhiskers Van Gogh) 6a995f5 Merge #6944: fix: HD chain encryption check ordering issue (pasta) 6fd7059 chore: mark v23 as release (pasta) ae08f53 docs: integrate 6946 release notes into final (pasta) 74a222d Merge #6946: feat: show seed on wallet creation (pasta) 877343a Merge #6943: fix: don't treat arrays/objects as string literals for composite methods (pasta) 00368fb Merge #6940: fix: reuse best clsig to avoid potential race condition (pasta) 8eceb98 Merge #6938: fix: logic error in `CheckDecryptionKey` (pasta) 3f30664 Merge #6929: fix: repair `makeseeds.py`, `getblockchaininfo[softforks]` help text, drop extra `generate`s from test, resolve macOS GID issue (pasta) 7ba4f1c Merge #6928: docs: update man pages for 23.0 (pasta) a6c1d6a Merge #6920: chore: update minimum chain work, tx stats, checkpoints, seeds and SECURITY.md for v23 (pasta) 84df1f0 Merge #6909: chore: Translations 2025-10 (pasta) a6449b1 Merge #6885: fix: improve governance/proposal dialog strings (pasta) ebf3a64 docs: typos (pasta) 4ad5533 docs: typos (pasta) f407453 doc: Replace Bitcoin Core PR references with Dash Core backport PRs (pasta) 78d0725 docs: add note on proto bump and platformban p2p (pasta) e0519c3 docs: fix whitespace errors (pasta) bc8db22 docs: minor improvements to release notes (pasta) c338511 docs: reorganize rpc updates to organize extended address changes (thephez) 700c46e style: make heading style consistent (thephez) bd636bd docs: add contributors (pasta) 6d29bc3 docs: revert changes deferred to v24 (pasta) 615f5ff docs: make the downgrade warning more confident (pasta) 567771a Apply suggestions from code review (PastaPastaPasta) 2b3211a docs: add link to 22.1.3 release notes (pasta) 548a38a docs: remove individual release notes files (pasta) e770c25 docs: add v23.0.0 release notes and archive v22.1.3 (pasta) Pull request description: ## Issue being fixed or feature implemented Includes changes from 23.0.0 release too because we never merged it back. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 491db4a kwvg: utACK 491db4a Tree-SHA512: 61895cd1f9d01ac7be1d407afe1ddd24b98e8242cb03229ecd586a4d2d1c43dbc62c98da52a8c715b3a5943fb40e99b23251e691f778779af9d6da94392122a3

fix: update BuildTestVectors call to adjust batch size based on outpu…

b220dec

…t flag

PastaPastaPasta added this to the 23.0.1 milestone Nov 26, 2025

PastaPastaPasta requested review from UdjinM6 and knst November 26, 2025 21:49

PastaPastaPasta added the backport-candidate-23.0.x label Nov 26, 2025

UdjinM6 approved these changes Nov 27, 2025

View reviewed changes

PastaPastaPasta merged commit 199ac79 into dashpay:develop Nov 28, 2025
28 of 30 checks passed

PastaPastaPasta mentioned this pull request Nov 29, 2025

Undefined behaviour in bench_dash #7011

Closed

DashCoreAutoGuix mentioned this pull request Dec 2, 2025

Merge bitcoin/bitcoin#27477: test: add regression tests for #27468 (invalid URI segfaults) DashCoreAutoGuix/dash#1135

Open

2 tasks

UdjinM6 removed the backport-candidate-23.0.x label Dec 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update BuildTestVectors call to adjust batch size based on output flag#7013

fix: update BuildTestVectors call to adjust batch size based on output flag#7013
PastaPastaPasta merged 1 commit intodashpay:developfrom
PastaPastaPasta:fix/bench-undef

PastaPastaPasta commented Nov 26, 2025

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

coderabbitai bot commented Nov 26, 2025

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

PastaPastaPasta commented Nov 26, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Nov 26, 2025

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Nov 26, 2025

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants