Clean up wallet encryption code. #5319

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:btc-crypter changing 2 files +17 −25

domob1812 commented at 8:00 AM on November 20, 2014: contributor

Add a new method DecryptKey in crypter.cpp, that combines the logic for decrypting, initialising and validating a CKey object. This was previously duplicated.

Note that the verification of the public key was not done for GetKey before (only while unlocking). I don't think that it is a problem to check the validity of the key also in GetKey, though.
sipa commented at 8:43 AM on November 20, 2014: member

Looks good to me.

Can you rebase on top of #5224, which uses a more independent way of verifying pubkey-key correspondence?
in src/crypter.cpp:None in 2fadcb2eb2 outdated
```
 101 | @@ -102,7 +102,7 @@ bool CCrypter::Decrypt(const std::vector<unsigned char>& vchCiphertext, CKeyingM
 102 |  }
 103 |  
 104 |  
 105 | -bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector<unsigned char> &vchCiphertext)
 106 | +static bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector<unsigned char> &vchCiphertext)
```
Diapolo commented at 11:36 AM on November 20, 2014:

Just for my personal knowledge, what is the benefit oft his static change?

domob1812 commented at 11:39 AM on November 20, 2014:

It emphasises that these functions are "internal" to crypter.cpp.

sipa commented at 11:42 AM on November 20, 2014:

It means the function is local to the compilation unit (the object file produced from this .cpp file), and can't be accessed from outside. That means that it doesn't have to be unique (there cannot be a collision with a function with the same name elsewhere), it can allow more optimizations (for example if all cases can be inlined, there doesn't need to be a full separate version of it), and it reduces link time (because the symbol doesn't need to be exported).

Diapolo commented at 12:03 PM on November 20, 2014:

Thanks for that explanations guys :).
domob1812 commented at 7:11 AM on November 21, 2014: contributor

Of course. Will #5224 be merged soon or should I base this patch off it already before merging?
laanwj added the label Wallet on Nov 28, 2014
fanquake commented at 2:37 PM on December 3, 2014: member

@domob1812 Now that #5224 is merged do you plan on rebasing this?
domob1812 commented at 6:34 PM on December 3, 2014: contributor

@fanquake: Thanks for the update! Yes, I'll rebase this. (In a few days, since I'm on a conference right now.)

Clean up wallet encryption code.

Add a new method DecryptKey in crypter.cpp, that combines the logic for
decrypting, initialising and validating a CKey object.  This was
previously duplicated.

35f7227a86

domob1812 force-pushed on Dec 4, 2014
domob1812 commented at 8:41 PM on December 4, 2014: contributor

Done, now using CKey::VerifyPubKey to verify the decrypted key.
laanwj commented at 11:35 AM on January 8, 2015: member

@sipa @jgarzik ACK?
laanwj merged this on Feb 4, 2015
laanwj closed this on Feb 4, 2015
laanwj referenced this in commit 93b7544501 on Feb 4, 2015
domob1812 deleted the branch on Feb 4, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels