script_number: add impl/tests #516
Conversation
| @@ -0,0 +1,225 @@ | |||
| [ | |||
There was a problem hiding this comment.
These fixtures are generated from bitcoin core.
@jprichardson any ideas on how we could increase transparency over this?
A link to a gist that will likely disappear soon isn't great.
|
@jprichardson thoughts on |
| } | ||
|
|
||
| var result | ||
|
|
There was a problem hiding this comment.
I couldn't go with the basic for loop construction (as done by bitcoin/bitcoin) here simply because we only have 32-bit integers.
There was a problem hiding this comment.
Not sure on 100%, but should work
var result
for (var i = 0; i < length; ++i) {
result += buffers[i] * Math.pow(2, 8 * i)
}There was a problem hiding this comment.
Suffers rounding errors, I tried that initially
There was a problem hiding this comment.
How it's possible? Number have 53 bit precision, there only 40 bits.
Can you paste example that give rounding errors?
There was a problem hiding this comment.
@fanatid never mind, works fine, will have a look at it again tomorrow.
To avoid the 40-bit special case entirely, I'd need a non-integer/no-bitshifts etc equivalent of the negative handling code (aka, -result on 0x80).
There was a problem hiding this comment.
@fanatid I'm thinking when I was testing it, there was other issues that gave me the wrong impression. You are certainly correct that there should not have been issues.
|
I'll merge this tomorrow after I add the missing tests for exceptional cases and run a full 1:1 value matching over the |
| result |= buffer[i] << (8 * i) | ||
| } | ||
|
|
||
| if (buffer[length - 1] & 0x80) return -(result & ~(0x80 << (8 * (length - 1)))) |
There was a problem hiding this comment.
@fanatid if you have any ideas on how to apply this to the 40-bit case, then I'll go ahead and generalize the entire thing (yay).
I can add you as a contributor if you want to push to this branch 👍
For me, its sleep time. Will check back in the morning.
There was a problem hiding this comment.
I think it should be fine:
if (buffer[length - 1] & 0x80) return -((result / Math.pow(2, 8 * (length - 1) - 1)) | 0)There was a problem hiding this comment.
| 0 will force it to 32-bits again, won't work with 40-bit.
Also, this doesn't work at all anyway, was it meant to be just a drop-in replacement?
There was a problem hiding this comment.
@fanatid got the last statement with:
var b = buffer[length - 1]
if (b & 0x80) {
result -= b * Math.pow(2, 8 * (length - 1))
return -((b & ~0x80) * Math.pow(2, 8 * (length - 1)) + result)
}Not really worth it though, the readability is marginal and the performance is almost 50% worse.
With the current code, the difference between when bit-wise operators are OK and the 40-bit special case is explicit.
There was a problem hiding this comment.
Agree, bitwise is more readable.
| @@ -372,6 +372,8 @@ module.exports = { | |||
| fromASM: fromASM, | |||
| toASM: toASM, | |||
|
|
|||
| number: require('./script_number'), | |||
There was a problem hiding this comment.
@jprichardson not going to release this publicly till I get an ACK from you on this (or at least one more contributor)
|
ACK. Nicely done! Any reasons why you want this in the core repo and not a separate package? |
|
@jprichardson not specifically, however it does coincide with the idea of being a core-primitive. |
@jprichardson in working on the spend example for #511 (which I now regret merging without it), I've realised we didn't have a
CScriptNumequivalent for script numbers encoded in a stack.