Skip to content

Commit 4f56c16

Browse files
WayonbWayonb
authored
Merge pull request #3 from nemtech/master
Merge the latest fixes
2 parents 014969f + 77d1309 commit 4f56c16

22 files changed

+348
-229
lines changed

‎plugins/coresystem/src/importance/ActivityObserverUtils.cpp

+1-18
Original file line numberDiff line numberDiff line change
@@ -24,34 +24,17 @@
2424

2525
namespace catapult { namespace importance {
2626

27-
namespace {
28-
bool ShouldPopBucket(const state::AccountActivityBuckets::ActivityBucket& bucket) {
29-
return model::ImportanceHeight() != bucket.StartHeight
30-
&& Amount() == bucket.TotalFeesPaid
31-
&& 0u == bucket.BeneficiaryCount
32-
&& 0u == bucket.RawScore;
33-
}
34-
}
35-
3627
void UpdateActivity(
3728
const Key& publicKey,
3829
const observers::ObserverContext& context,
3930
const ActivityBucketConsumer& commitAction,
4031
const ActivityBucketConsumer& rollbackAction) {
4132
auto& accountStateCache = context.Cache.sub<cache::AccountStateCache>();
4233
auto accountStateIter = accountStateCache.find(publicKey);
43-
if (accountStateIter.get().Balances.get(accountStateCache.harvestingMosaicId()) < accountStateCache.minHarvesterBalance())
44-
return;
4534

4635
auto& activityBuckets = accountStateIter.get().ActivityBuckets;
4736
auto importanceHeight = model::ConvertToImportanceHeight(context.Height, accountStateCache.importanceGrouping());
48-
if (observers::NotifyMode::Commit == context.Mode) {
49-
activityBuckets.update(importanceHeight, commitAction);
50-
} else {
51-
activityBuckets.tryUpdate(importanceHeight, rollbackAction);
5237

53-
if (ShouldPopBucket(*activityBuckets.begin()))
54-
activityBuckets.pop();
55-
}
38+
activityBuckets.tryUpdate(importanceHeight, observers::NotifyMode::Commit == context.Mode ? commitAction : rollbackAction);
5639
}
5740
}}

‎plugins/coresystem/tests/importance/ActivityObserverUtilsTests.cpp

+73-65
Original file line numberDiff line numberDiff line change
@@ -137,84 +137,82 @@ namespace catapult { namespace importance {
137137

138138
// region bucket creation
139139

140-
TEST(TEST_CLASS, UpdateActivityCommitCreatesNewBucket) {
141-
// Arrange:
142-
TestContext context(observers::NotifyMode::Commit, Amount(1000));
143-
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
144-
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
140+
namespace {
141+
void AssertUpdateActivityDoesNotCreateNewBucket(observers::NotifyMode notifyMode) {
142+
// Arrange:
143+
TestContext context(notifyMode, Amount(1000));
144+
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
145+
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
145146

146-
// Act:
147-
context.update(signerPublicKey);
147+
// Act:
148+
context.update(signerPublicKey);
149+
150+
// Assert: bucket was not created
151+
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
152+
EXPECT_EQ(model::ImportanceHeight(), activityBucket.StartHeight);
153+
}
154+
}
148155

149-
// Assert: bucket was created
150-
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
151-
EXPECT_EQ(Importance_Height, activityBucket.StartHeight);
152-
EXPECT_EQ(2u, activityBucket.BeneficiaryCount);
156+
TEST(TEST_CLASS, UpdateActivityCommitDoesNotCreateNewBucket) {
157+
AssertUpdateActivityDoesNotCreateNewBucket(observers::NotifyMode::Commit);
153158
}
154159

155160
TEST(TEST_CLASS, UpdateActivityRollbackDoesNotCreateNewBucket) {
156-
// Arrange:
157-
TestContext context(observers::NotifyMode::Rollback, Amount(1000));
158-
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
159-
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
160-
161-
// Act:
162-
context.update(signerPublicKey);
163-
164-
// Assert: bucket was not created
165-
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
166-
EXPECT_EQ(model::ImportanceHeight(), activityBucket.StartHeight);
161+
AssertUpdateActivityDoesNotCreateNewBucket(observers::NotifyMode::Rollback);
167162
}
168163

169164
// endregion
170165

171166
// region bucket removal
172167

173-
TEST(TEST_CLASS, UpdateActivityCommitDoesNotRemoveZeroBucket) {
174-
// Arrange:
175-
TestContext context(observers::NotifyMode::Commit, Amount(1000));
176-
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
177-
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
178-
signerAccountStateIter.get().ActivityBuckets.update(Importance_Height, [](auto& bucket) {
179-
test::SetMaxValue(bucket.BeneficiaryCount);
180-
--bucket.BeneficiaryCount;
181-
});
168+
namespace {
169+
size_t CountNonzeroFields(const state::AccountActivityBuckets::ActivityBucket& activityBucket) {
170+
return (Amount() != activityBucket.TotalFeesPaid ? 1 : 0)
171+
+ (0u != activityBucket.BeneficiaryCount ? 1 : 0)
172+
+ (0u != activityBucket.RawScore ? 1 : 0);
173+
}
174+
175+
void AssertUpdateActivityDoesNotRemoveZeroBucket(observers::NotifyMode notifyMode, uint32_t initialBeneficiaryCount) {
176+
// Arrange:
177+
TestContext context(notifyMode, Amount(1000));
178+
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
179+
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
180+
signerAccountStateIter.get().ActivityBuckets.update(Importance_Height, [initialBeneficiaryCount](auto& bucket) {
181+
bucket.BeneficiaryCount = initialBeneficiaryCount;
182+
});
182183

183-
// Act:
184-
context.update(signerPublicKey);
184+
// Act:
185+
context.update(signerPublicKey);
185186

186-
// Assert: bucket was updated
187-
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
188-
EXPECT_EQ(Importance_Height, activityBucket.StartHeight);
189-
EXPECT_EQ(0u, activityBucket.BeneficiaryCount);
187+
// Assert: bucket was updated
188+
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
189+
EXPECT_EQ(Importance_Height, activityBucket.StartHeight);
190+
EXPECT_EQ(0u, activityBucket.BeneficiaryCount);
191+
EXPECT_EQ(0u, CountNonzeroFields(activityBucket));
192+
}
190193
}
191194

192-
TEST(TEST_CLASS, UpdateActivityRollbackRemovesZeroBucket) {
193-
// Arrange:
194-
TestContext context(observers::NotifyMode::Rollback, Amount(1000));
195-
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
196-
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
197-
signerAccountStateIter.get().ActivityBuckets.update(Importance_Height, [](auto& bucket) {
198-
bucket.BeneficiaryCount = 2;
199-
});
200-
201-
// Act:
202-
context.update(signerPublicKey);
195+
TEST(TEST_CLASS, UpdateActivityCommitDoesNotRemoveZeroBucket) {
196+
auto initialBeneficiaryCount = std::numeric_limits<uint32_t>::max() - 1; // max - 1 + 2 == 0
197+
AssertUpdateActivityDoesNotRemoveZeroBucket(observers::NotifyMode::Commit, initialBeneficiaryCount);
198+
}
203199

204-
// Assert: bucket was removed
205-
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
206-
EXPECT_EQ(model::ImportanceHeight(), activityBucket.StartHeight);
200+
TEST(TEST_CLASS, UpdateActivityRollbackDoesNotRemoveZeroBucket) {
201+
AssertUpdateActivityDoesNotRemoveZeroBucket(observers::NotifyMode::Rollback, 2);
207202
}
208203

209204
namespace {
210-
template<typename TUpdateBucket>
211-
void AssertUpdateActivityRollbackDoesNotRemoveNonzeroBucket(const char* message, TUpdateBucket updateBucket) {
205+
void AssertUpdateActivityDoesNotRemoveNonzeroBucket(
206+
observers::NotifyMode notifyMode,
207+
uint32_t initialBeneficiaryCount,
208+
const char* message,
209+
const ActivityBucketConsumer& updateBucket) {
212210
// Arrange:
213-
TestContext context(observers::NotifyMode::Rollback, Amount(1000));
211+
TestContext context(notifyMode, Amount(1000));
214212
auto signerPublicKey = test::GenerateRandomByteArray<Key>();
215213
auto signerAccountStateIter = context.addAccount(signerPublicKey, Amount(1000));
216-
signerAccountStateIter.get().ActivityBuckets.update(Importance_Height, [updateBucket](auto& bucket) {
217-
bucket.BeneficiaryCount = 2;
214+
signerAccountStateIter.get().ActivityBuckets.update(Importance_Height, [initialBeneficiaryCount, updateBucket](auto& bucket) {
215+
bucket.BeneficiaryCount = initialBeneficiaryCount;
218216
updateBucket(bucket);
219217
});
220218

@@ -224,19 +222,29 @@ namespace catapult { namespace importance {
224222
// Assert: bucket was updated
225223
const auto& activityBucket = signerAccountStateIter.get().ActivityBuckets.get(Importance_Height);
226224
EXPECT_EQ(Importance_Height, activityBucket.StartHeight) << message;
225+
EXPECT_EQ(1u, CountNonzeroFields(activityBucket)) << message;
226+
}
227+
228+
void AssertUpdateActivityDoesNotRemoveNonzeroBucketAll(observers::NotifyMode notifyMode, uint32_t initialBeneficiaryCount) {
229+
AssertUpdateActivityDoesNotRemoveNonzeroBucket(notifyMode, initialBeneficiaryCount, "TotalFeesPaid", [](auto& bucket) {
230+
bucket.TotalFeesPaid = bucket.TotalFeesPaid + Amount(1);
231+
});
232+
AssertUpdateActivityDoesNotRemoveNonzeroBucket(notifyMode, initialBeneficiaryCount, "BeneficiaryCount", [](auto& bucket) {
233+
++bucket.BeneficiaryCount;
234+
});
235+
AssertUpdateActivityDoesNotRemoveNonzeroBucket(notifyMode, initialBeneficiaryCount, "RawScore", [](auto& bucket) {
236+
++bucket.RawScore;
237+
});
227238
}
228239
}
229240

241+
TEST(TEST_CLASS, UpdateActivityCommitDoesNotRemoveNonzeroBucket) {
242+
auto initialBeneficiaryCount = std::numeric_limits<uint32_t>::max() - 1; // max - 1 + 2 == 0
243+
AssertUpdateActivityDoesNotRemoveNonzeroBucketAll(observers::NotifyMode::Commit, initialBeneficiaryCount);
244+
}
245+
230246
TEST(TEST_CLASS, UpdateActivityRollbackDoesNotRemoveNonzeroBucket) {
231-
AssertUpdateActivityRollbackDoesNotRemoveNonzeroBucket("TotalFeesPaid", [](auto& bucket) {
232-
bucket.TotalFeesPaid = bucket.TotalFeesPaid + Amount(1);
233-
});
234-
AssertUpdateActivityRollbackDoesNotRemoveNonzeroBucket("BeneficiaryCount", [](auto& bucket) {
235-
++bucket.BeneficiaryCount;
236-
});
237-
AssertUpdateActivityRollbackDoesNotRemoveNonzeroBucket("RawScore", [](auto& bucket) {
238-
++bucket.RawScore;
239-
});
247+
AssertUpdateActivityDoesNotRemoveNonzeroBucketAll(observers::NotifyMode::Rollback, 2);
240248
}
241249

242250
// endregion

‎plugins/coresystem/tests/importance/PosImportanceCalculatorTests.cpp

+53-37
Original file line numberDiff line numberDiff line change
@@ -481,54 +481,70 @@ namespace catapult { namespace importance {
481481

482482
// endregion
483483

484-
// region removed accounts
484+
// region activity bucket creation / removal
485485

486-
TEST(TEST_CLASS, PosDisablesActivityCollectionAndRemovesBucketWhenPresent) {
487-
// Arrange:
488-
auto config = CreateBlockChainConfiguration(0);
486+
namespace {
487+
void RunActivityBucketCreationRemovalTest(
488+
const consumer<const state::AccountActivityBuckets&, uint8_t>& checkIneligibleBalance,
489+
const consumer<const state::AccountActivityBuckets&, uint8_t>& checkEligibleBalance) {
490+
// Arrange:
491+
auto config = CreateBlockChainConfiguration(0);
489492

490-
std::vector<AccountSeed> accountSeeds;
491-
for (auto i = 1u; i <= Num_Account_States; ++i) {
492-
std::vector<state::AccountActivityBuckets::ActivityBucket> buckets;
493-
buckets.push_back(CreateActivityBucket(Amount(100), 200, Recalculation_Height));
494-
accountSeeds.push_back({ config.MinHarvesterBalance, buckets });
495-
}
493+
std::vector<AccountSeed> accountSeeds;
494+
for (auto i = 1u; i <= Num_Account_States; ++i) {
495+
std::vector<state::AccountActivityBuckets::ActivityBucket> buckets;
496+
buckets.push_back(CreateActivityBucket(Amount(100), 200, Recalculation_Height));
497+
accountSeeds.push_back({ config.MinHarvesterBalance, buckets });
498+
}
496499

497-
// Sanity:
498-
EXPECT_EQ(10u, accountSeeds.size());
500+
// Sanity:
501+
EXPECT_EQ(10u, accountSeeds.size());
499502

500-
CacheHolder holder(config.MinHarvesterBalance);
501-
holder.seedDelta(accountSeeds, Recalculation_Height);
502-
holder.Cache.commit();
503-
for (uint8_t i = 1u; i <= Num_Account_States; ++i) {
504-
auto& accountState = holder.get(Key{ { i } });
505-
if (1 == i % 2) {
506-
accountState.Balances.debit(Harvesting_Mosaic_Id, Amount(1));
507-
if (1 == i % 4)
508-
holder.Delta->queueRemove(accountState.PublicKey, accountState.PublicKeyHeight);
503+
CacheHolder holder(config.MinHarvesterBalance);
504+
holder.seedDelta(accountSeeds, Recalculation_Height);
505+
holder.Cache.commit();
506+
for (uint8_t i = 1u; i <= Num_Account_States; ++i) {
507+
auto& accountState = holder.get(Key{ { i } });
508+
if (1 == i % 2) {
509+
accountState.Balances.debit(Harvesting_Mosaic_Id, Amount(1));
510+
if (1 == i % 4)
511+
holder.Delta->queueRemove(accountState.PublicKey, accountState.PublicKeyHeight);
512+
}
509513
}
510-
}
511514

512-
holder.Delta->commitRemovals();
513-
auto pCalculator = CreateImportanceCalculator(config);
515+
holder.Delta->commitRemovals();
516+
auto pCalculator = CreateImportanceCalculator(config);
514517

515-
// Act:
516-
pCalculator->recalculate(Recalculation_Height, *holder.Delta);
518+
// Act:
519+
pCalculator->recalculate(Recalculation_Height, *holder.Delta);
517520

518-
// Assert:
519-
for (uint8_t i = 1u; i <= Num_Account_States; ++i) {
520-
if (1 == i % 4) {
521-
// note that this will not happen in real scenario
522-
EXPECT_FALSE(holder.Delta->contains(Key{ { i } })) << "account " << i;
523-
} else {
524-
const auto& buckets = holder.Delta->find(Key{ { i } }).get().ActivityBuckets;
525-
if (1 == i % 2)
526-
EXPECT_EQ(model::ImportanceHeight(), buckets.get(Recalculation_Height).StartHeight) << "account " << i;
527-
else
528-
EXPECT_EQ(Recalculation_Height, buckets.get(Recalculation_Height).StartHeight) << "account " << i;
521+
// Assert:
522+
for (uint8_t i = 1u; i <= Num_Account_States; ++i) {
523+
if (1 == i % 4) {
524+
// note that this will not happen in real scenario
525+
EXPECT_FALSE(holder.Delta->contains(Key{ { i } })) << "account " << i;
526+
} else {
527+
const auto& buckets = holder.Delta->find(Key{ { i } }).get().ActivityBuckets;
528+
auto checker = (1 == i % 2) ? checkIneligibleBalance : checkEligibleBalance;
529+
checker(buckets, i);
530+
}
529531
}
530532
}
531533
}
532534

535+
TEST(TEST_CLASS, PosCreatesNewActivityBucketOnlyForAccountsWithMinBalanceAtRecalculationHeight) {
536+
RunActivityBucketCreationRemovalTest(
537+
[](const auto& buckets, auto i) {
538+
const auto& bucket = buckets.get(Recalculation_Height);
539+
EXPECT_EQ(model::ImportanceHeight(), bucket.StartHeight) << "account " << i;
540+
EXPECT_EQ(0u, bucket.RawScore) << "account " << i;
541+
},
542+
[](const auto& buckets, auto i) {
543+
const auto& bucket = buckets.get(Recalculation_Height);
544+
EXPECT_EQ(Recalculation_Height, bucket.StartHeight) << "account " << i;
545+
EXPECT_NE(0u, bucket.RawScore) << "account " << i;
546+
});
547+
}
548+
533549
// endregion
534550
}}

‎scripts/jenkins/runner.sh

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ rm -rf catapult-src/internal
66

77
branchName=$(echo ${GIT_BRANCH} | sed 's/origin\///;')
88

9-
# pick devel as default
10-
internalBranchName="devel"
9+
# pick master as default
10+
internalBranchName="master"
1111

1212
# if there's corresponding internal branch to main repo branch pick it instead
1313
hasInternal=$(GIT_SSH_COMMAND="ssh -i ${CREDS_KEYFILE}" git ls-remote --heads ${INTERNAL_REPO} refs/heads/${branchName} | wc -l)

‎src/catapult/consumers/HashCheckConsumer.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ namespace catapult { namespace consumers {
9292

9393
private:
9494
bool shouldSkip(const model::TransactionElement& element) {
95-
if (!m_recentHashCache.add(element.EntityHash))
95+
if (!m_recentHashCache.add(element.MerkleComponentHash))
9696
return true;
9797

9898
return m_knownHashPredicate(element.Transaction.Deadline, element.EntityHash);

‎src/catapult/crypto/SharedKey.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#elif defined(__GNUC__)
3737
#pragma GCC diagnostic ignored "-Wunused-function"
3838
#pragma GCC diagnostic ignored "-Wstrict-aliasing"
39+
#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
3940
#elif defined(_MSC_VER)
4041
#pragma warning(push)
4142
#pragma warning(disable : 4324) /* ed25519 structs use __declspec(align()) */

0 commit comments

Comments
 (0)