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

Remove always true cases in wallet/crypter.cpp encrypt and decrypt function #7643

Closed
wants to merge 1 commit into from

Conversation

JeremyRubin
Copy link
Contributor

No description provided.

EVP_CIPHER_CTX_init(&ctx);
if (fOk) fOk = EVP_EncryptInit_ex(&ctx, EVP_aes_256_cbc(), NULL, chKey, chIV) != 0;

bool fOk = EVP_EncryptInit_ex(&ctx, EVP_aes_256_cbc(), NULL, chKey, chIV) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this results in the same (remove code) as above.
The L66 EVP function only gets executed when fOk is true (which is always true at L66). fOk will then be set to true if the EVP function did not return 0.

I think the current code works right.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and looks ugly 8)

ACK

@jonasschnelli
Copy link
Contributor

Yes. This code is cleaner.

utACK.

@theuni
Copy link
Member

theuni commented Mar 4, 2016

Change looks fine, but we're about to nuke it anyway with #5949. Would prefer to avoid the rebase.

@sipa
Copy link
Member

sipa commented Mar 5, 2016

No strong opinion about the code change, but agree with @theuni.

@maflcko
Copy link
Member

maflcko commented Mar 5, 2016

NACK, I like the previous code better because the "formatting" is uniform. Also, I am pretty sure the compiler will remove this case. Apart from that, agree with @theuni

@laanwj
Copy link
Member

laanwj commented Mar 6, 2016

Yes, let's focus on getting #5949 merged instead.

@laanwj laanwj closed this Mar 6, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants