[Merged by Bors] - Update key derivation to latest EIP-2333#1633
[Merged by Bors] - Update key derivation to latest EIP-2333#1633realbigsean wants to merge 6 commits intosigp:v0.3.0-stagingfrom
Conversation
|
Added this to v0.3.0 because it's a breaking change. I tested compatibility with the outstanding eth2.0-deposit-cli pull request and it works. It does not work with the current eth2.0-deposit-cli master |
|
For backwards-compatibility I guess users can always use v0.2.x to restore their old keys from a mnemonic. |
|
I've asked about release timing here: ethereum/staking-deposit-cli#108 (comment) |
CarlBeek
left a comment
There was a problem hiding this comment.
LGTM! 👍
For backwards-compatibility I guess users can always use v0.2.x to restore their old keys from a mnemonic.
Indeed, this is how we plan to handle it with the launchpad too.
Release timing
The launchpad will be using the new EIP2333 key derivation for Spadina. See ethereum/staking-deposit-cli#108 (comment) for reasoning why.
Thanks Carl! Given this info, I think it's safe to merge this into the v0.3.0 branch. |
paulhauner
left a comment
There was a problem hiding this comment.
Nice!
No functional issues, but I raised a couple of minor code-quality ones.
|
|
||
| // info = "" + I2OSP(L, 2) | ||
| let mut info = [0; 2]; | ||
| BigEndian::write_int(&mut info, MOD_R_L as i64, 2); |
There was a problem hiding this comment.
I think we can replicate this with std to avoid the byteorder dep and explicitly handle the integer cast:
use std::convert::TryFrom;
let info = u16::try_from(MOD_R_L).expect("MOD_R_L too large").to_be_bytes();| let zero_hash = ZeroizeHash::zero(); | ||
|
|
||
| let mut salt = b"BLS-SIG-KEYGEN-SALT-".to_vec(); | ||
| let mut hashed_salt = [0; HASH_SIZE]; |
There was a problem hiding this comment.
I think you can do away with the hashed_salt variable:
let mut salt = b"BLS-SIG-KEYGEN-SALT-".to_vec();
while output.as_bytes() == zero_hash.as_bytes() {
let mut hasher = Sha256::new();
hasher.update(salt);
salt = hasher.finalize().to_vec();
let prk = hkdf_extract(&salt, ikm_with_postfix.as_bytes());
let okm = &hkdf_expand(prk, &info, MOD_R_L);
output = mod_r(okm.as_bytes());
}It looks a bit more like the spec this way, too.
|
I tested this again after the PR updates and it works. The CI stepped that failed passed for me locally, not sure how to rerun it without pushing another commit. |
|
Already running a review |
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
|
Pull request successfully merged into v0.3.0-staging. Build succeeded: |
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
## Issue Addressed #1624 ## Proposed Changes Updates to match [EIP-2333](`https://eips.ethereum.org/EIPS/eip-2333`) ## Additional Info In order to have compatibility with the eth2.0-deposit-cli, [this PR](ethereum/staking-deposit-cli#108) must also be merged
Issue Addressed
Proposed Changes
Updates to match EIP-2333
Additional Info
In order to have compatibility with the eth2.0-deposit-cli, this PR must also be merged