-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Improved performance of block parsing #1279
Conversation
LGTM |
var bn = BN.fromBuffer(reversebuf); | ||
var second = this.buf.readUInt32LE(this.pos); | ||
var first = this.buf.readUInt32LE(this.pos + 4); | ||
var combined = (first * 0x100000000) + second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this lose precision? See:
> 0x100000000 * 0x10000000
1152921504606847000
> 0x100000000 * 0x10000000 + 1
1152921504606847000
Both answers give the same value. Does this mean that if second
is 0
or 1
and first
is 0x10000000
, this would return the same value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can lose precision with numbers equal or above 2 ^ 53
.
> 0x1fffffffffffff === Math.pow(2, 53) - 1
// true
> 1152921504606847000 < 0x1fffffffffffff
// false
And in that case the number will be read directly into BN and not a JavaScript Number first: https://github.com/bitpay/bitcore/pull/1279/files#diff-17000cc0d07e4a49ab9cbafe33ca7231R101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, gotcha. I didn't understand the < 0x1fffffffffffff
part. LGTM
Rebased on master and added comment to code with info about number/array BN instantiation. |
Improved performance of block parsing
readReverse
readUInt64LEBN
readUInt64LEBN