txscript: Change makeScriptNum take a length argument #455
Conversation
| // Interpreting data as an integer requires that it is not larger than | ||
| // a 32-bit integer. | ||
| func makeScriptNum(v []byte, requireMinimal bool, maxScriptNumLen int) (scriptNum, error) { | ||
| // Interpreting data requires that it is not larger than |
There was a problem hiding this comment.
the "as an integer" part should still be retained. Regardless of the number of bytes, it's still being interpreted as an integer.
|
I think there should be new tests added to deal with the new parameter as well. I see that the existing tests were changed to use the default max size, however, that's only testing the single value. In particular, I'd add tests for all variants of 5 bytes since that is how it will be used. |
| @@ -152,11 +156,6 @@ func (n scriptNum) Int32() int32 { | |||
| // makeScriptNum interprets the passed serialized bytes as an encoded integer | |||
| // and returns the result as a script number. | |||
| // | |||
| // Since the consensus rules dictate the serialized bytes interpreted as ints | |||
There was a problem hiding this comment.
Rather than removing this important information, I'd prefer it simply be modified slightly to reflect the existence of the parameter. Perhaps:
// Since the consensus rules dictate that serialized bytes interpreted as ints
// are only allowed to be in the range determined by a maximum number of bytes,
// on a per opcode basis, an error will be returned when the provided bytes
// would result in a number outside of that range. In particular, the range for
// the vast majority of opcodes dealing with numeric values are limited to 4
// bytes and therefore will pass that value to this function resulting in an
// allowed range of [-2^31 + 1, 2^31 - 1].
|
As we discussed on IRC, the comments on |
| @@ -164,11 +168,16 @@ func (n scriptNum) Int32() int32 { | |||
| // [0x7f 0x00 0x00 ...], etc. All forms except [0x7f] will return an error with | |||
| // requireMinimal enabled. | |||
| // | |||
| // The scriptNumLen is the size of the data to be encoded. | |||
There was a problem hiding this comment.
This isn't quite right. The number is already encoded when this function is called. Perhaps the following?
// The scriptNumLen is the maximum number of bytes the encoded value can be
// before an ErrStackNumberTooBig is returned. This effectivley limits the
// range of allowed values.
// WARNING: Great care should be taken if passing a value larger than
// defaultScriptNumLen, since it could lead to addition and multiplication
// overflows.
//
While current existing numeric opcodes are limited to 4 bytes, new opcodes may need different limits. This mimics Bitcoin Core commit 99088d60d8a7747c6d1a7fd5d8cd388be1b3e138
|
OK |
While current existing numeric opcodes are limited to 4 bytes, new
opcodes may need different limits.
This mimics Bitcoin Core commit 99088d60d8a7747c6d1a7fd5d8cd388be1b3e138