Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.clone() added to getBlockHash to avoid the chance of writing on sensitive data #553

Merged
merged 3 commits into from
May 21, 2018

Conversation

lsebrie
Copy link
Contributor

@lsebrie lsebrie commented May 11, 2018

When we introduce data on a Dataword it's by reference by default, so to avoid that a blockHash could be written by another class, a change is to introduced to return a copy of the Dataword

colltoaction
colltoaction previously approved these changes May 11, 2018
Copy link
Contributor

@colltoaction colltoaction left a comment

Choose a reason for hiding this comment

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

Makes sense if you don't want to add performance overhead to using DataWord.getData, but in the future we should evaluate the possibility of making it immutable.

@donequis
Copy link
Contributor

DataWord should be a final class now that I think about it

@donequis
Copy link
Contributor

@tinchou Making Dataword immutable will force us to alloc more memory when doing some operations with dispensable data. For example, if we have A and B on the stack and we want to add them, we can override A with A+B instead of creating a new object for A+B

Copy link
Contributor

@adauto82 adauto82 left a comment

Choose a reason for hiding this comment

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

Seems to check out with the scope of the task.

@adauto82
Copy link
Contributor

Check SonarQube

@ajlopezrsk ajlopezrsk merged commit f2ee9c8 into master May 21, 2018
@aeidelman aeidelman added this to the Bamboo v0.4.3 milestone Jun 8, 2018
@bernacodesido bernacodesido deleted the op-hash_missing_clone branch September 5, 2018 20:34
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.

6 participants