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

web3.utils.randomHex does not produce consistent length strings #1490

Closed
nick opened this issue Mar 25, 2018 · 7 comments · Fixed by #2794
Closed

web3.utils.randomHex does not produce consistent length strings #1490

nick opened this issue Mar 25, 2018 · 7 comments · Fixed by #2794
Assignees
Labels
Bug Addressing a bug

Comments

@nick
Copy link

nick commented Mar 25, 2018

On Chrome 65 (OSX) using web3 1.0.0-beta.33, it seems web3.utils.randomHex does not produce consistent length hex strings. Also tested on Firefox.

> web3.utils.randomHex(32).length                
63
> web3.utils.randomHex(32).length                
66
> web3.utils.randomHex(32).length                
65
@nnkken
Copy link

nnkken commented May 3, 2018

Same issue here.
The problem is in the package randomhex.
In the package, the implementation for browser is:

var randomBytes = cryptoLib.getRandomValues(new Uint8Array(size));
var returnValue = '0x'+ Array.from(randomBytes).map(function(arr){ return arr.toString(16); }).join('');

And if the number (ranged 0 - 255) is less than 16, arr.toString(16) returns only one single hex digit.

Given that the number of bytes is large enough, the probability to have missing 0s trends to 100%.
(For size = 32, probability to actually have 64 characters = (15/16)^32 = 12.7%, which matches the statistics got in browser)

This is a serious problem, since it greatly decreases the probability of having digit 0.

I've tested calling randomHex(32) 1 million times, and see the statistics of digits appeared. The result is as follows:

[1997961, 3999880, 4004169, 4002023, 3998594, 3998180, 3999718, 4000678, 4002234, 4000798, 3998786, 3999998, 4001340, 4000397, 3997218, 3999074]

As you can see, the probability of having 0 digit is dropped to about half of normal.

Don't know how to open PR for the randomHex repo, since there is no source file in the Github repo...

@theAngryLoop
Copy link

Work Around

`generateNew(){

return new Promise((resolve, reject)=>{

  let pvtKey = null;
  // Because of faulty randomHex implementation use do-while
  do{
    pvtKey = web3.utils.randomHex(32);
  }while(pvtKey.length != 66);

  let addressBuff = privateToAddress(toBuffer(pvtKey));

  resolve({
    private: pvtKey.substring(2), // removes '0x' from begining
    address: bufferToHex(addressBuff)
  });
  
});

}`

@nnkken
Copy link

nnkken commented May 10, 2018

@theAngryLoop
This is a work around, but the distribution of the bytes you get will still be wrong.
For instance, using the same principle to get a result from web3.utils.randomHex(1) with result.length === 4 (0x included) will never get result 0x00, 0x01, ..., 0x0f, since all these results will be actually 0x0, 0x1, ..., 0xf and filtered by the length constraint.

@nivida nivida self-assigned this Aug 9, 2018
@nivida nivida added the Bug Addressing a bug label Aug 9, 2018
@username1565
Copy link

username1565 commented Nov 9, 2018

@nnkken,
In the package, the implementation for browser is:

var randomBytes = cryptoLib.getRandomValues(new Uint8Array(size));
var returnValue = '0x'+ Array.from(randomBytes).map(function(arr){ return arr.toString(16); }).join('');

I see this code here: https://github.com/pwall-org/pwall/blob/master/js/web3-eth-accounts.js

This not working in old browsers.
Look at this code.

var randomBytes = cryptoLib.getRandomValues(new Uint8Array(size));
//console.log(randomBytes); //[130, 247, 4, 241, 77, 175, 138, 55, ...]
/*
//there was been an error for me:
//when browser trying to call Array.from() function
//Uncaught TypeError: undefined is not a function
    var returnValue = '0x' + Array.from(randomBytes).map(function (arr) {
    return arr.toString(16);
}).join('');
			
//console.log(returnValue); //may be 0xHEXSTRING, but not working and give me a throw error
*/
			
//so I used this code
var returnValue = '0x';
for(var i=0;i<=randomBytes.length-1;i++){
    returnValue += randomBytes[i].toString(16);
}
//console.log(returnValue);//0x + hex string working normally, and no any errors.
//... continue script...

So now you can fix this to ensure backward compatibility.

@rstormsf
Copy link

randomHex(12).substr(2).padStart(24, '0')
I use this one

@username1565
Copy link

username1565 commented Dec 23, 2018

@rstormsf, this need to include largest functions:
String.prototype.padStart() and String.prototype.repeat()

@AntonioJuliano
Copy link

This seems to work correctly:

const cryptoObj = window.crypto || window.msCrypto; // for IE 11

export function getRandomHex(size) {
  const randomBytes = cryptoObj.getRandomValues(new Uint8Array(size));

  let returnValue = '0x';
  for (let i = 0; i <= randomBytes.length - 1; i += 1) {
    let bStr = randomBytes[i].toString(16);
    if (bStr.length === 1) {
      bStr = `0${bStr}`;
    }
    returnValue += bStr;
  }

  return returnValue;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Addressing a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants