Skip to content

Don't use boost range adaptors in CDeterministicMNList#2327

Merged
UdjinM6 merged 1 commit intodashpay:developfrom
codablock:pr_noboostadaptors
Oct 2, 2018
Merged

Don't use boost range adaptors in CDeterministicMNList#2327
UdjinM6 merged 1 commit intodashpay:developfrom
codablock:pr_noboostadaptors

Conversation

@codablock
Copy link

These cause crashes when used in for loops. Reason is very likely that
temporary CDeterministicMNList objects are destroyed before entering the
loop while the adaptors rely on these objects.

Discovered in one of our devnets while calling "protx list"

@UdjinM6 UdjinM6 added this to the 12.4 milestone Sep 28, 2018
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The less dependencies the better :)

See inline comments.

Side note: conceptually, it's a bit concerning that we have to loop to build the vector only to pass this vector to some function to loop over it again. Sounds like a good place for some refactoring in the future (but I'm fine with this PR as a drop-in replacement for boost stuff for now).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto& ...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same or even

-    for (auto txout : block.vtx[0]->vout) {
-        for (auto& dmn : dmnList.GetValidMNs()) {
+    for (const auto& txout : block.vtx[0]->vout) {
+        for (const auto& dmn : dmnList.GetValidMNs()) {

while at it ;)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we check for if (IsMNValid(p.second)) here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good you spotted this one

@codablock
Copy link
Author

Agree that this solution is suboptimal, and while reading your concern I realized that this is maybe a good candidate for something like ForEachMN(function). Will implement this now and push in a few minutes.

These cause crashes when used in for loops. Reason is very likely that
temporary CDeterministicMNList objects are destroyed before entering the
loop while the adaptors rely on these objects.

Discovered in one of our devnets while calling "protx list"
@codablock
Copy link
Author

Pushed new version of this fix, based on ForEachMN which avoids unnecessary copies

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍

utACK

@UdjinM6 UdjinM6 merged commit eaef902 into dashpay:develop Oct 2, 2018
@codablock codablock deleted the pr_noboostadaptors branch December 27, 2018 15:24
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request May 20, 2019
These cause crashes when used in for loops. Reason is very likely that
temporary CDeterministicMNList objects are destroyed before entering the
loop while the adaptors rely on these objects.

Discovered in one of our devnets while calling "protx list"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants