strkey: check key length validation methods #1769
Comments
ire-and-curses
added
the
Hacktoberfest
leighmcculloch
changed the title
leighmcculloch
removed
the
Hacktoberfest
leighmcculloch
added a commit
that referenced
this issue
Dec 1, 2021
Replace bytes.Buffer with working with a byte array directly. Also, contains the crc16 improvements by @2opremio that were in #4105. Reduce the allocations in strkey.Encode from 6 to 1, and reduce the heap use from 244 B/op to 64 B/op. In the past I have seen impact to performance of Starlight caused by the strkey.Encode functions. @2opremio also highlighted in #4105 there is an impact there. The use of a bytes.Buffer in this code is rather unnecessary given we know the size of everything, and given that the size of strkeys are small encouraging their data onto the stack is fine. It is worth noting that there is a functional impact by this change. Attempts to strkey.Encode a data value greater than the max payload size as defined by SEP-23 will now error. We've been wanting to add length checks (#1769) for sometime, so I think this is fine, and we can continue to improve those length checks in the future. Before: BenchmarkDecode_accountID-8 2576450 414.2 ns/op 130 B/op 3 allocs/op BenchmarkEncode_accountID-8 3657649 319.6 ns/op 244 B/op 6 allocs/op After: BenchmarkDecode_accountID-8 3563737 334.9 ns/op 128 B/op 2 allocs/op BenchmarkEncode_accountID-8 7365306 165.4 ns/op 64 B/op 1 allocs/op Co-authored-by: Alfonso Acosta <2362916+2opremio@users.noreply.github.com>
tamirms
pushed a commit
that referenced
this issue
Dec 1, 2021
Replace bytes.Buffer with working with a byte array directly. Also, contains the crc16 improvements by @2opremio that were in #4105. Reduce the allocations in strkey.Encode from 6 to 1, and reduce the heap use from 244 B/op to 64 B/op. In the past I have seen impact to performance of Starlight caused by the strkey.Encode functions. @2opremio also highlighted in #4105 there is an impact there. The use of a bytes.Buffer in this code is rather unnecessary given we know the size of everything, and given that the size of strkeys are small encouraging their data onto the stack is fine. It is worth noting that there is a functional impact by this change. Attempts to strkey.Encode a data value greater than the max payload size as defined by SEP-23 will now error. We've been wanting to add length checks (#1769) for sometime, so I think this is fine, and we can continue to improve those length checks in the future. Before: BenchmarkDecode_accountID-8 2576450 414.2 ns/op 130 B/op 3 allocs/op BenchmarkEncode_accountID-8 3657649 319.6 ns/op 244 B/op 6 allocs/op After: BenchmarkDecode_accountID-8 3563737 334.9 ns/op 128 B/op 2 allocs/op BenchmarkEncode_accountID-8 7365306 165.4 ns/op 64 B/op 1 allocs/op Co-authored-by: Alfonso Acosta <2362916+2opremio@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While checking Horizon diff I noticed that #1573 is not checking the keys properly. It checks if the values are properly encoded but it doesn't check the length of the keys (should be 32 bytes). Check js-sdk for comparison: https://github.com/stellar/js-stellar-base/blob/f9d03ce4ffe0989851d777abf2e934fdbf36b514/src/strkey.js#L119
The text was updated successfully, but these errors were encountered: