junderw
commented
4 years ago @jmacxx Thanks for the shout out. Looks like coinbin is using Arrays and var instead of ES6 syntax, so I modified some of our code and put it in.
Let me know if the code looks ok.
There is a bug with how script numbers are encoded. If you do a CLTV transaction using blockheight = 128, the height parameter should be minimally encoded in script as "8000".
It is currently encoded as "80". This throws a script error in bitcoin core when the redeem transaction is checked, stating "numeric value encoded as 80 is not minimally encoded". Likewise, any number where the most significant bit is set has this problem: the ranges 80-ff, 8000-ffff and 800000-ffffff, 80000000-ffffffff. The last range represents a lock-by-blocktime CLTV of 2038-01-19 03:15 (GMT) and onwards. It should be minimally encoded in script as 3400008000 (5 bytes). currently being encoded as 34000080 (4 bytes, msb set implies negative number).
Also, numbers 1-16 should be minimally encoded as OP_1 to OP_16 (hex numbers 0x51 to 0x60), but they are not.
So currently only an issue if you like making CLTVs for long ago block heights (pointless), or if one were to lock funds to 2038 (not recommended anyway!)
If CHECKSEQUENCEVERIFY were ever to be implemented in coinbin using the same encoding this would be more of a problem due to it being a relative number of blocks. Therefore this can be a note to future implementors.
For reference, bitcoinjs-lib does minimally encode script numbers correctly. (Noted as a good tool for test-comparisons).