perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient #6581
Conversation
WalkthroughThis pull request introduces changes in three main areas. In the BLS module, the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Reindex finished successfully |
…default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient
PastaPastaPasta
force-pushed
the
perf-build-simplified-mn-list-diff-bls-compare-to-null
branch
from
40909d7 to
ada6f2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/bls_tests.cpp (2)
475-524: Enhance class documentation and exception safety.Consider the following improvements to the
DummyBLSclass:
- Add method documentation explaining the purpose and contract of each public method
- Add
noexceptspecifications where applicable (e.g., constructors, Reset, IsValid)- Mark methods that don't modify state as
constclass DummyBLS { public: static const size_t SerSize = 4; std::array<uint8_t, SerSize> data{}; - DummyBLS() { + /// Default constructor initializing data to zeros + DummyBLS() noexcept { data.fill(0); } + /// @returns true if any byte in data is non-zero bool IsValid() const { return std::any_of(data.begin(), data.end(), [](uint8_t c){ return c != 0; }); } + /// @returns data array ignoring legacy flag std::array<uint8_t, SerSize> ToBytes(bool /*legacy*/) const { return data; } + /// Sets data from bytes array ignoring legacy flag void SetBytes(const std::array<uint8_t, SerSize>& bytes, bool /*legacy*/) { data = bytes; } + /// @returns true if data matches given bytes ignoring legacy flag bool CheckMalleable(const std::array<uint8_t, SerSize>& bytes, bool /*legacy*/) const { return data == bytes; } + /// Resets data to zeros void Reset() noexcept { data.fill(0); }
529-609: Add test coverage for edge cases and error conditions.While the current test suite is good, consider adding the following test cases:
- Test behavior with invalid data
- Test error conditions in serialization/deserialization
- Test the Reset() functionality
- Test behavior with zero-filled but non-default objects
Would you like me to generate the additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/bls/bls.h(3 hunks)src/evo/deterministicmns.cpp(3 hunks)src/test/bls_tests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bls/bls.h
- src/evo/deterministicmns.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/test/bls_tests.cpp
[error] 473-473: Clang format differences found. Please run 'clang-format' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (1)
src/test/bls_tests.cpp (1)
526-527: LGTM!The type alias is well-defined and appropriately named.
| { | ||
| FuncThresholdSignature(true); | ||
| FuncThresholdSignature(false); | ||
| } |
There was a problem hiding this comment.
Fix clang-format issue.
The CI pipeline indicates a formatting issue. Please run clang-format on the file to resolve this.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 473-473: Clang format differences found. Please run 'clang-format' to format the code.
src/test/bls_tests.cpp
Outdated
| DummyBLS obj; | ||
| obj.data = {1, 2, 3, 4}; // nonzero data makes the object valid | ||
| lazy_a.Set(obj, false); | ||
| obj.data = {4, 3, 2, 1}; // nonzero data makes the object valid |
There was a problem hiding this comment.
nit: maybe make only 1 byte difference here, to catch error "any_of" vs "all_of" in operator ==
obj.data = {0, 2, 3, 4}; // nonzero data makes the object valid
| LazyDummyBLS lazy2; | ||
| BOOST_CHECK(lazy1 == lazy2); | ||
| DummyBLS obj; | ||
| obj.data = {5, 6, 7, 8}; |
There was a problem hiding this comment.
nit: maybe make only 1 byte difference here, to catch error "any_of" vs "all_of".
such as:
obj.data = {1, 2, 3, 5};
src/test/bls_tests.cpp
Outdated
|
|
||
| // Produce a string representation. | ||
| std::string ToString(bool /*legacy*/) const { | ||
| std::ostringstream oss; |
There was a problem hiding this comment.
nit: refactor by using HexStr from src/util/strencodings.h
|
Looks good but clang-format complains a lot. Pls consider formatting new tests accordingly (and also pls add a new line at eof). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/bls_tests.cpp (2)
475-515: LGTM! Clean and efficient implementation of the DummyBLS class.The class provides a good minimal interface for testing CBLSLazyWrapper with efficient implementations. The use of std::array and std::any_of shows good C++ practices.
Consider simplifying the ToString method using HexStr from src/util/strencodings.h as suggested in the past review:
- std::string ToString(bool /*legacy*/) const - { - std::ostringstream oss; - for (auto b : data) { - oss << std::setfill('0') << std::setw(2) << std::hex << static_cast<int>(b); - } - return oss.str(); - } + std::string ToString(bool /*legacy*/) const + { + return HexStr(data); + }
520-600: LGTM! Comprehensive test coverage for LazyDummyBLS.The test suite thoroughly covers equality comparisons, serialization, and hash consistency.
Consider modifying the test data to better catch potential edge cases. As suggested in past reviews, use minimal differences:
- obj.data = {1, 2, 3, 4}; // nonzero data makes the object valid + obj.data = {1, 0, 0, 0}; // single nonzero byte to catch any_of vs all_of errorsThis change would help catch potential issues with the IsValid() implementation if it were mistakenly using all_of instead of any_of.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/evo/deterministicmns.cpp(3 hunks)src/test/bls_tests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/deterministicmns.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (2)
src/test/bls_tests.cpp (2)
517-518: LGTM! Clear type alias definition.The type alias improves readability in the test cases.
473-473: Fix clang-format issue.The CI pipeline indicates a formatting issue. Please run clang-format on the file to resolve this.
|
pls consider fe5250a |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/test/bls_tests.cpp (5)
489-489: Consider using std::ranges::any_of for modern C++.For better readability and maintainability, consider using the more modern std::ranges::any_of algorithm.
- return std::any_of(data.begin(), data.end(), [](uint8_t c) { return c != 0; }); + return std::ranges::any_of(data, [](uint8_t c) { return c != 0; });
529-529: Consider using more diverse test data.The test data could be more diverse to catch edge cases. For example:
- Test with all zeros except the last byte
- Test with alternating patterns
- Test with all maximum values (0xFF)
- obj.data = {1, 0, 0, 0}; // nonzero data makes the object valid + obj.data = {0, 0, 0, 1}; // Test last byte non-zero- obj.data = {1, 2, 3, 4}; // nonzero data makes the object valid + obj.data = {0xAA, 0x55, 0xAA, 0x55}; // Test alternating pattern- obj.data = {5, 6, 7, 8}; + obj.data = {0xFF, 0xFF, 0xFF, 0xFF}; // Test maximum valuesAlso applies to: 541-541, 555-555
535-535: Fix duplicate test case comment.The comment for this test case is identical to the previous one (Test 2), but the test case is different.
-// Test 2: A default wrapper and one initialized with a nonzero DummyBLS should compare unequal. +// Test 3: Two different non-default wrappers should compare unequal.
572-573: Consider testing different serialization versions.The serialization tests only verify with a single legacy flag value. Consider testing both legacy and non-legacy serialization.
CDataStream ds(SER_DISK, CLIENT_VERSION); - lazy1.Serialize(ds, true); + // Test both legacy and non-legacy serialization + lazy1.Serialize(ds, true); + lazy1.Serialize(ds, false); LazyDummyBLS lazy2; - lazy2.Unserialize(ds, true); - BOOST_CHECK(lazy1 == lazy2); + lazy2.Unserialize(ds, true); + BOOST_CHECK(lazy1 == lazy2); + lazy2.Unserialize(ds, false); + BOOST_CHECK(lazy1 == lazy2);Also applies to: 577-578
582-594: Consider adding hash collision test.The hash consistency test is good, but consider adding a test to verify that different objects produce different hashes.
uint256 hash1 = lazy1.GetHash(); uint256 hash2 = lazy2.GetHash(); BOOST_CHECK(hash1 == hash2); + + // Test that different objects have different hashes + DummyBLS different_obj; + different_obj.data = {16, 15, 14, 13}; // Different from original + lazy2.Set(different_obj, false); + uint256 hash3 = lazy2.GetHash(); + BOOST_CHECK(hash1 != hash3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/bls_tests.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (2)
src/test/bls_tests.cpp (2)
476-509: LGTM! Well-structured mock implementation.The DummyBLS class provides a clean and minimal interface required by CBLSLazyWrapper. The implementation includes all necessary methods with clear documentation.
505-505: Good use of HexStr utility.Excellent use of the HexStr utility from strencodings.h for string representation.
…en comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient 0cf8a46 suggestions (UdjinM6) 56ac184 fmt: apply clang-format suggestions (pasta) e18f621 fix: improper default check; add tests; use in more dml places (pasta) ada6f2b perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) Pull request description: ## Profiling Analysis before <img width="1926" alt="Pasted Graphic 11" src="https://github.com/user-attachments/assets/3d333419-eea3-4ba3-a0e1-daa1670e4d52" /> after <img width="1909" alt="Pasted Graphic" src="https://github.com/user-attachments/assets/b019b62d-5033-4ed9-9fdb-34e4e1d9e76a" /> ## Methods Below, is some analysis on the results of running the `protx diff` RPC 500 times. The diffs had a start block between MIN and MAX as defined below; and an end block no more than MAX_DIFF from the selected start block. We then perform some statistical analysis on the data. MIN_VALUE = 1500050 MAX_VALUE = 2000050 MAX_DIFF = 50000  ## Statistical Analysis, outliers included ### Before Five-Number Summary of Execution Times: Min: 0.024492 sec Q1: 0.124626 sec Median: 0.243000 sec Q3: 0.358459 sec Max: 15.583948 sec Mean Execution Time: 0.428296 sec Standard Deviation: 0.933486 sec Linear Regression Results: y = 0.000001 * x + 0.308662 R-squared: 0.008160 (Goodness of Fit)   ### After Five-Number Summary of Execution Times: Min: 0.038174 sec Q1: 0.121363 sec Median: 0.158175 sec Q3: 0.215866 sec Max: 16.587903 sec Mean Execution Time: 0.239239 sec Standard Deviation: 0.762387 sec Linear Regression Results: y = 0.000001 * x + 0.151169 R-squared: 0.006918 (Goodness of Fit) P-value: 0.063105 (Significance)   ## Statistical Analysis, outliers excluded ### Before removed 76 data points Five-Number Summary of Execution Times (After Outlier Removal): Min: 0.035916 sec Q1: 0.211060 sec Median: 0.319278 sec Q3: 0.357963 sec Max: 0.572785 sec Mean Execution Time: 0.289764 sec Standard Deviation: 0.101140 sec Linear Regression Results (After Outlier Removal): y = 0.0000000199 * x + 0.286447 R-squared: 0.000496 (Goodness of Fit)  ### After removed 32 data points Five-Number Summary of Execution Times (After Outlier Removal): Min: 0.038174 sec Q1: 0.119880 sec Median: 0.151724 sec Q3: 0.205017 sec Max: 0.355078 sec Mean Execution Time: 0.164165 sec Standard Deviation: 0.060919 sec Linear Regression Results (After Outlier Removal): y = 0.0000003119 * x + 0.111002 R-squared: 0.399298 (Goodness of Fit)  ## How Has This Been Tested? Ran unit tests locally; reindexing currently, going to let CI run functional tests ## Breaking Changes Should be none; but please think through the diff specifically related to https://github.com/dashpay/dash/compare/develop...PastaPastaPasta:dash:perf-build-simplified-mn-list-diff-bls-compare-to-null?expand=1#diff-0998f8dfc4c1089e90cbaafe9607b361035b904cd103df31e3c2339a3cbf790dR480 ## 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: LGTM, utACK 0cf8a46 Tree-SHA512: 14eb1dd3bb85c271c1d8a381554b1d774c573336123e97cffbf3222efbbe78e201ef9f331046f8f3b9147fda13fc2ea70250a46e87adcc7da5ae3301c555eddd
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for a new version, v22.1.2 ## What was done? See release notes ## How Has This Been Tested? ## Breaking Changes None ## 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 8b4ab03 knst: utACK 8b4ab03 Tree-SHA512: 9f2f65e315940197cc2b75a6b0a858d624256cbe668272ff6dfa216eceda1ba9338484d47afc569202b3b5cc75b4dcb825209efe3a3d3ccec57c741f75c40577
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) 4298d73 chore: bump to 22.1.1 (pasta) fc65a16 chore: release notes for 22.1.1 (pasta) 38762f7 Merge #6574: fix: ReconnectionInfo should also store Dash-specific flags (pasta) 580b74c Merge #6566: fix(qt): avoid leaking balance and CJ info in GUI when in discreet mode (pasta) Pull request description: ## Issue being fixed or feature implemented ## What was done? ff-ed `master` 21.1.0 -> 21.1.2, merging it back into `develop` now ## 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 73fa780; gonna merge as this changes docs only Tree-SHA512: f04fe461edc05c38771d684bd5d60327076251b8004723027276b104989ea6d84f7f77cce31f310f058e81f2c25411ab5a15b563cd3db66091ccaf33da459e3c
3 participants
Profiling Analysis
before

after
Methods
Below, is some analysis on the results of running the
protx diffRPC 500 times. The diffs had a start block between MIN and MAX as defined below; and an end block no more than MAX_DIFF from the selected start block. We then perform some statistical analysis on the data.MIN_VALUE = 1500050
MAX_VALUE = 2000050
MAX_DIFF = 50000

Statistical Analysis, outliers included
Before
Five-Number Summary of Execution Times:
Min: 0.024492 sec
Q1: 0.124626 sec
Median: 0.243000 sec
Q3: 0.358459 sec
Max: 15.583948 sec
Mean Execution Time: 0.428296 sec
Standard Deviation: 0.933486 sec
Linear Regression Results:
y = 0.000001 * x + 0.308662
R-squared: 0.008160 (Goodness of Fit)
After
Five-Number Summary of Execution Times:
Min: 0.038174 sec
Q1: 0.121363 sec
Median: 0.158175 sec
Q3: 0.215866 sec
Max: 16.587903 sec
Mean Execution Time: 0.239239 sec
Standard Deviation: 0.762387 sec
Linear Regression Results:
y = 0.000001 * x + 0.151169
R-squared: 0.006918 (Goodness of Fit)
P-value: 0.063105 (Significance)
Statistical Analysis, outliers excluded
Before
removed 76 data points
Five-Number Summary of Execution Times (After Outlier Removal):
Min: 0.035916 sec
Q1: 0.211060 sec
Median: 0.319278 sec
Q3: 0.357963 sec
Max: 0.572785 sec
Mean Execution Time: 0.289764 sec
Standard Deviation: 0.101140 sec
Linear Regression Results (After Outlier Removal):
y = 0.0000000199 * x + 0.286447
R-squared: 0.000496 (Goodness of Fit)
After
removed 32 data points
Five-Number Summary of Execution Times (After Outlier Removal):
Min: 0.038174 sec
Q1: 0.119880 sec
Median: 0.151724 sec
Q3: 0.205017 sec
Max: 0.355078 sec
Mean Execution Time: 0.164165 sec
Standard Deviation: 0.060919 sec
Linear Regression Results (After Outlier Removal):
y = 0.0000003119 * x + 0.111002
R-squared: 0.399298 (Goodness of Fit)
How Has This Been Tested?
Ran unit tests locally; reindexing currently, going to let CI run functional tests
Breaking Changes
Should be none; but please think through the diff specifically related to https://github.com/dashpay/dash/compare/develop...PastaPastaPasta:dash:perf-build-simplified-mn-list-diff-bls-compare-to-null?expand=1#diff-0998f8dfc4c1089e90cbaafe9607b361035b904cd103df31e3c2339a3cbf790dR480
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.