Skip to content

Commit 497f95c

Browse files
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
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) ![Observed Data](https://github.com/user-attachments/assets/12fdd894-0f2c-4cba-9945-5e9d084c9b24) ![Pasted Graphic 6](https://github.com/user-attachments/assets/97706786-73b1-417d-9036-caaa3add1290) ### 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) ![Observed Data](https://github.com/user-attachments/assets/d50e5b5b-1a67-47c9-980c-564adab083ba) ![Observed Data](https://github.com/user-attachments/assets/cecc0394-f484-410a-ad21-1a9bbd5a1dc7) ## 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) ![Pasted Graphic 10](https://github.com/user-attachments/assets/1f0f6389-dc8f-48a5-80b7-9b5ccb3869dc) ### 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) ![Observed Data](https://github.com/user-attachments/assets/91e97214-6afb-4c3a-a98a-cd7e5704e724) ## 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
1 parent 0f62391 commit 497f95c

File tree

3 files changed

+136
-4
lines changed

3 files changed

+136
-4
lines changed

src/bls/bls.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,7 @@ class CBLSLazyWrapper
391391
mutable std::mutex mutex;
392392

393393
mutable std::array<uint8_t, BLSObject::SerSize> vecBytes;
394+
// Indicates if the value contained in vecBytes is valid
394395
mutable bool bufValid{false};
395396
mutable bool bufLegacyScheme{true};
396397

@@ -462,7 +463,7 @@ class CBLSLazyWrapper
462463
{
463464
std::unique_lock<std::mutex> l(mutex);
464465
s.read(AsWritableBytes(Span{vecBytes.data(), BLSObject::SerSize}));
465-
bufValid = true;
466+
bufValid = std::any_of(vecBytes.begin(), vecBytes.end(), [](uint8_t c) { return c != 0; });
466467
bufLegacyScheme = specificLegacyScheme;
467468
objInitialized = false;
468469
hash.SetNull();
@@ -507,6 +508,14 @@ class CBLSLazyWrapper
507508

508509
bool operator==(const CBLSLazyWrapper& r) const
509510
{
511+
// If neither bufValid or objInitialized are set, then the object is the default object.
512+
const bool is_default{!bufValid && !objInitialized};
513+
const bool r_is_default{!r.bufValid && !r.objInitialized};
514+
// If both are default; they are equal.
515+
if (is_default && r_is_default) return true;
516+
// If one is default and the other isn't, we are not equal
517+
if (is_default != r_is_default) return false;
518+
510519
if (bufValid && r.bufValid && bufLegacyScheme == r.bufLegacyScheme) {
511520
return vecBytes == r.vecBytes;
512521
}

src/evo/deterministicmns.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
477477
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate keyIDOwner=%s", __func__,
478478
dmn->proTxHash.ToString(), EncodeDestination(PKHash(dmn->pdmnState->keyIDOwner)))));
479479
}
480-
if (dmn->pdmnState->pubKeyOperator.Get().IsValid() && !AddUniqueProperty(*dmn, dmn->pdmnState->pubKeyOperator)) {
480+
if (dmn->pdmnState->pubKeyOperator != CBLSLazyPublicKey() && !AddUniqueProperty(*dmn, dmn->pdmnState->pubKeyOperator)) {
481481
mnUniquePropertyMap = mnUniquePropertyMapSaved;
482482
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate pubKeyOperator=%s", __func__,
483483
dmn->proTxHash.ToString(), dmn->pdmnState->pubKeyOperator.ToString())));
@@ -578,7 +578,8 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
578578
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a keyIDOwner=%s", __func__,
579579
proTxHash.ToString(), EncodeDestination(PKHash(dmn->pdmnState->keyIDOwner)))));
580580
}
581-
if (dmn->pdmnState->pubKeyOperator.Get().IsValid() && !DeleteUniqueProperty(*dmn, dmn->pdmnState->pubKeyOperator)) {
581+
if (dmn->pdmnState->pubKeyOperator != CBLSLazyPublicKey() &&
582+
!DeleteUniqueProperty(*dmn, dmn->pdmnState->pubKeyOperator)) {
582583
mnUniquePropertyMap = mnUniquePropertyMapSaved;
583584
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a pubKeyOperator=%s", __func__,
584585
proTxHash.ToString(), dmn->pdmnState->pubKeyOperator.ToString())));
@@ -840,7 +841,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
840841
}
841842
if (newState->IsBanned()) {
842843
// only revive when all keys are set
843-
if (newState->pubKeyOperator.Get().IsValid() && !newState->keyIDVoting.IsNull() && !newState->keyIDOwner.IsNull()) {
844+
if (newState->pubKeyOperator != CBLSLazyPublicKey() && !newState->keyIDVoting.IsNull() &&
845+
!newState->keyIDOwner.IsNull()) {
844846
newState->Revive(nHeight);
845847
if (debugLogs) {
846848
LogPrintf("CDeterministicMNManager::%s -- MN %s revived at height %d\n",

src/test/bls_tests.cpp

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <random.h>
99
#include <streams.h>
1010
#include <util/irange.h>
11+
#include <util/strencodings.h>
1112

1213
#include <boost/test/unit_test.hpp>
1314

@@ -472,4 +473,124 @@ BOOST_AUTO_TEST_CASE(bls_threshold_signature_tests)
472473
FuncThresholdSignature(false);
473474
}
474475

476+
// A dummy BLS object that satisfies the minimal interface expected by CBLSLazyWrapper.
477+
class DummyBLS
478+
{
479+
public:
480+
// Define a fixed serialization size (for testing purposes).
481+
static const size_t SerSize = 4;
482+
std::array<uint8_t, SerSize> data{};
483+
484+
DummyBLS() { data.fill(0); }
485+
486+
// A dummy validity check: valid if any byte is non-zero.
487+
bool IsValid() const
488+
{
489+
return std::any_of(data.begin(), data.end(), [](uint8_t c) { return c != 0; });
490+
}
491+
492+
// Convert to bytes; ignore the legacy flag for simplicity.
493+
std::array<uint8_t, SerSize> ToBytes(bool /*legacy*/) const { return data; }
494+
495+
// Set from bytes; again, ignore the legacy flag.
496+
void SetBytes(const std::array<uint8_t, SerSize>& bytes, bool /*legacy*/) { data = bytes; }
497+
498+
// A dummy malleability check: simply compares the stored data to the given bytes.
499+
bool CheckMalleable(const std::array<uint8_t, SerSize>& bytes, bool /*legacy*/) const { return data == bytes; }
500+
501+
// Reset the object to an "empty" state.
502+
void Reset() { data.fill(0); }
503+
504+
// Produce a string representation.
505+
std::string ToString(bool /*legacy*/) const { return HexStr(data); }
506+
507+
// Equality operator.
508+
bool operator==(const DummyBLS& other) const { return data == other.data; }
509+
};
510+
511+
// Define a type alias for our lazy wrapper instantiated with DummyBLS.
512+
using LazyDummyBLS = CBLSLazyWrapper<DummyBLS>;
513+
514+
// Test 1: Two default (unset) wrappers should compare equal.
515+
BOOST_AUTO_TEST_CASE(test_default_equality)
516+
{
517+
LazyDummyBLS lazy1;
518+
LazyDummyBLS lazy2;
519+
// Neither instance has been set, so they represent the default/null object.
520+
BOOST_CHECK(lazy1 == lazy2);
521+
}
522+
523+
// Test 2: A default wrapper and one initialized with a nonzero DummyBLS should compare unequal.
524+
BOOST_AUTO_TEST_CASE(test_non_default_vs_default)
525+
{
526+
LazyDummyBLS lazy_default;
527+
LazyDummyBLS lazy_set;
528+
DummyBLS obj;
529+
obj.data = {1, 0, 0, 0}; // nonzero data makes the object valid
530+
lazy_set.Set(obj, false);
531+
BOOST_CHECK(!(lazy_default == lazy_set));
532+
BOOST_CHECK(lazy_default != lazy_set);
533+
}
534+
535+
// Test 2: A default wrapper and one initialized with a nonzero DummyBLS should compare unequal.
536+
BOOST_AUTO_TEST_CASE(test_non_default_vs_different)
537+
{
538+
LazyDummyBLS lazy_a;
539+
LazyDummyBLS lazy_b;
540+
DummyBLS obj;
541+
obj.data = {1, 2, 3, 4}; // nonzero data makes the object valid
542+
lazy_a.Set(obj, false);
543+
obj.data = {2, 2, 3, 4}; // nonzero data makes the object valid
544+
lazy_b.Set(obj, false);
545+
BOOST_CHECK(lazy_a != lazy_b);
546+
}
547+
548+
// Test 3: Two wrappers set with the same underlying DummyBLS value compare equal.
549+
BOOST_AUTO_TEST_CASE(test_equality_same_value)
550+
{
551+
LazyDummyBLS lazy1;
552+
LazyDummyBLS lazy2;
553+
BOOST_CHECK(lazy1 == lazy2);
554+
DummyBLS obj;
555+
obj.data = {5, 6, 7, 8};
556+
lazy1.Set(obj, false);
557+
BOOST_CHECK(lazy1 != lazy2);
558+
lazy2.Set(obj, false);
559+
BOOST_CHECK(lazy1 == lazy2);
560+
}
561+
562+
// Test 4: Serialization and unserialization preserve the wrapped value.
563+
BOOST_AUTO_TEST_CASE(test_serialization_unserialization)
564+
{
565+
LazyDummyBLS lazy1;
566+
DummyBLS obj;
567+
obj.data = {9, 10, 11, 12};
568+
// Set with a specific legacy flag (true in this case)
569+
lazy1.Set(obj, true);
570+
571+
// Serialize the lazy object into a data stream.
572+
CDataStream ds(SER_DISK, CLIENT_VERSION);
573+
lazy1.Serialize(ds, true);
574+
575+
// Create a new instance and unserialize the data into it.
576+
LazyDummyBLS lazy2;
577+
lazy2.Unserialize(ds, true);
578+
BOOST_CHECK(lazy1 == lazy2);
579+
BOOST_CHECK(lazy2.Get() == obj);
580+
}
581+
582+
// Test 5: Two wrappers wrapping the same object should have the same hash.
583+
BOOST_AUTO_TEST_CASE(test_get_hash_consistency)
584+
{
585+
LazyDummyBLS lazy1;
586+
LazyDummyBLS lazy2;
587+
DummyBLS obj;
588+
obj.data = {13, 14, 15, 16};
589+
lazy1.Set(obj, false);
590+
lazy2.Set(obj, false);
591+
uint256 hash1 = lazy1.GetHash();
592+
uint256 hash2 = lazy2.GetHash();
593+
BOOST_CHECK(hash1 == hash2);
594+
}
595+
475596
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)