From 5382940e607abfd498cff6741ddbe46be8ea145d Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 10:58:19 +0200 Subject: [PATCH 01/16] Remove unnecessary host functions allocator usage --- ...0000-remove-unnecessary-allocator-usage.md | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 text/0000-remove-unnecessary-allocator-usage.md diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0000-remove-unnecessary-allocator-usage.md new file mode 100644 index 000000000..826df1394 --- /dev/null +++ b/text/0000-remove-unnecessary-allocator-usage.md @@ -0,0 +1,202 @@ +# RFC-0000: Remove unnecessary host functions allocator usage + +| | | +| --------------- | ------------------------------------------------------------------------------------------- | +| **Start Date** | 2023-07-04 | +| **Description** | Add alternatives to host functions that make use of the allocator when unnecessary | +| **Authors** | Pierre Krieger | + +## Summary + +Add new versions of host functions in order to avoid using the allocator when the buffer being allocated is of a size known at compilation time. + +## Motivation + +The heap allocation of the runtime is currently controlled by the host using a memory allocator on the host side. + +The API of many host functions consists in allocating a buffer. For example, when calling `ext_hashing_twox_256_version_1`, the host allocates a 32 bytes buffer using the host allocator, and returns a pointer to this buffer to the runtime. The runtime later has to call `ext_allocator_free_version_1` on this pointer in order to free the buffer. + +Even though no benchmark has been done, it is pretty obvious that this design is very inefficient. To continue with the example of `ext_hashing_twox_256_version_1`, it would be more efficient to instead write the output hash to a buffer that was allocated by the runtime on its stack and passed by pointer to the function. Allocating a buffer on the stack in the worst case scenario simply consists in decreasing a number, and in the best case scenario is free. Doing so would save many Wasm memory reads and writes by the allocator, and would save a function call to `ext_allocator_free_version_1`. + +After this RFC, only the following functions have no exact equivalent that doesn't use the allocator: + +- `ext_storage_get` +- `ext_default_child_storage_get` +- `ext_storage_next_key` +- `ext_default_child_storage_next_key` +- `ext_crypto_ed25519_public_keys` +- `ext_crypto_sr25519_public_keys` +- `ext_crypto_ecdsa_public_keys` +- `ext_offchain_network_state` +- `ext_offchain_local_storage_get` +- `ext_offchain_http_response_wait` +- `ext_offchain_http_response_headers` +- `ext_offchain_http_response_read_body` + +## Stakeholders + +No attempt was made at convincing stakeholders. The writer of this RFC believes that these changes are pretty non-controversial. + +## Explanation + +This RFC proposes to introduce the following new host functions: + +```wat +(func $ext_storage_read_version_2 + (param $key i64) (param $value_out i64) (param $offset i32) (result i64)) +(func $ext_default_child_storage_read_version_2 + (param $child_storage_key i64) (param $key i64) (param $value_out i64) + (param $offset i32) (result i64)) +``` + +The signature and behaviour of `ext_storage_read_version_2` and `ext_default_child_storage_read_version_2` is identical to their version 1 equivalent, but the return value has a different meaning. +The new functions directly return the number of bytes written in the `value_out` buffer. If the entry doesn't exist, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `value_out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. + +```wat +(func $ext_hashing_keccak_256_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_keccak_512_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_sha2_256_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_blake2_128_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_blake2_256_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_twox_64_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_twox_128_version_2 + (param $data i64) (param $out i32)) +(func $ext_hashing_twox_256_version_2 + (param $data i64) (param $out i32)) +(func $ext_trie_blake2_256_root_version_3 + (param $data i64) (param $version i32) (param $out i32)) +(func $ext_trie_blake2_256_ordered_root_version_3 + (param $data i64) (param $version i32) (param $out i32)) +(func $ext_trie_keccak_256_root_version_3 + (param $data i64) (param $version i32) (param $out i32)) +(func $ext_trie_keccak_256_ordered_root_version_3 + (param $data i64) (param $version i32) (param $out i32)) +(func $ext_default_child_storage_root_version_3 + (param $child_storage_key i64) (param $out i32)) +(func $ext_storage_root_version_3 + (param $out i32)) +(func $ext_crypto_ed25519_generate_version_2 + (param $key_type_id i32) (param $seed i64) (param $out i32)) +(func $ext_crypto_sr25519_generate_version_2 + (param $key_type_id i32) (param $seed i64) (return i32)) +(func $ext_crypto_ecdsa_generate_version_2 + (param $key_type_id i32) (param $seed i64) (return i32)) +``` + +The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. + +```wat +(func $ext_default_child_storage_root_version_3 + (param $child_storage_key i64) (param $out i32)) +(func $ext_storage_root_version_3 + (param $out i32)) +``` + +The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. + +I have taken the liberty to take the version 1 of these functions as a base rather than the version 2, as a PPP deprecating the version 2 of these functions has previously been accepted: . + +```wat +(func $ext_storage_clear_prefix_version_3 + (param $prefix i64) (param $limit i64) (param $removed_count_out i32) + (return i32)) +(func $ext_default_child_storage_clear_prefix_version_3 + (param $child_storage_key i64) (param $prefix i64) + (param $limit i64) (param $removed_count_out i32) (return i32)) +(func $ext_default_child_storage_kill_version_4 + (param $child_storage_key i64) (param $limit i64) + (param $removed_count_out i32) (return i32)) +``` + +The behaviour of these functions is identical to their version 2 and 3 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the version 3 and 4 of these functions accepts a `removed_count_out` parameter containing the memory location to a 8 bytes buffer where the host writes the number of keys that were removed in little endian. The functions return 1 to indicate that there are keys remaining, and 0 to indicate that all keys have been removed. + +Note that there is an alternative proposal to add new host functions with the same names: . This alternative doesn't conflict with this one except for the version number. One proposal or the other will have to use versions 4 and 5 rather than 3 and 4. + +```wat +(func $ext_crypto_ed25519_sign_version_2 + (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i32)) +(func $ext_crypto_sr25519_sign_version_2 + (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i32)) +func $ext_crypto_ecdsa_sign_version_2 + (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i32)) +(func $ext_crypto_ecdsa_sign_prehashed_version_2 + (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i64)) +``` + +The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. + +Note that the return value is 0 on success and 1 on failure, while the previous version of these functions write 1 on success (as it represents a SCALE-encoded `Some`) and 0 on failure (as it represents a SCALE-encoded `None`). Returning 0 on success and non-zero on failure is consistent with common practices in the C programming language and is less surprising than the opposite. + +```wat +(func $ext_crypto_secp256k1_ecdsa_recover_version_3 + (param $sig i32) (param $msg i32) (param $out i32) (return i64)) +(func $ext_crypto_secp256k1_ecdsa_recover_compressed_version_3 + (param $sig i32) (param $msg i32) (param $out i32) (return i64)) +``` + +The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is the same as in version 2 of these functions: . + +```wat +(func $ext_offchain_http_request_start_version_2 + (param $method i64) (param $uri i64) (param $meta i64) (result i32)) +``` + +The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns 0. An identifier of 0 is invalid and is reserved to indicate failure. + +```wat +(func $ext_offchain_http_request_write_body_version_2 + (param $method i64) (param $uri i64) (param $meta i64) (result i32)) +``` + +The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing two bytes in it, and returning a pointer to it, the new version of this function simply indicates what happened: + +- 0 on success. +- 1 if the deadline was reached. +- 2 if there was an I/O error while processing the request. +- 3 if the identifier of the request is invalid. + +These values are equal to the values returned on error by the version 1 (see ), but incremented by 1 in order to reserve 0 for success. + +```wat +(func $ext_offchain_submit_transaction_version_2 + (param $data i64) (return i32)) +(func $ext_offchain_http_request_add_header_version_2 + (param $request_id i32) (param $name i64) (param $value i64) (result i32)) +``` + +The behaviour of these functions is identical to their version 1 equivalent. Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions simply return 1 or 0. + +## Drawbacks + +- This RFC might be difficult to implement in Substrate due to the internal code design. It is not clear to the author of this RFC how difficult it would be. + +- In some situations, the runtime might still have to call `ext_allocator_malloc_version_1` then call one of the new functions, which is slightly less performant than when the two operations are combined into one call. The author of this RFC believes that this negligible, and that the performance saved by not allocating a buffer in most situations is worth the trade-off. + +## Prior Art + +The API of these new functions was heavily inspired by API used by the C programming language. + +## Unresolved Questions + +No unresolved questions. + +## Future Possibilities + +The existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Furthermore, runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to updated to not use it. + +When it comes to removing the allocator usage from the functions that still use it after this RFC, it can in my opinion be done like this: + +- The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions can be updated to no longer use the allocator by splitting them in two: one function to query the number of items and one function to query an individual item. +- The `ext_offchain_local_storage_get` should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. +- The `ext_storage_next_key`, `ext_default_child_storage_next_key` should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. +- The `ext_storage_get` and `ext_default_child_storage_get` should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. + +Furthermore, the input data provided to the runtime is also allocated using the allocator. New host functions that allow reading the input from the runtime would have to be added. + +Because all these changes might be controversial and might require benchmarking, and that the removal of the allocator might also be controversial, I have decided to not include them as part of this RFC. From 86c1ffb781442167a41bd944d192b2a80845f428 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:04:12 +0200 Subject: [PATCH 02/16] Tweaks --- text/0000-remove-unnecessary-allocator-usage.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0000-remove-unnecessary-allocator-usage.md index 826df1394..faea7d81c 100644 --- a/text/0000-remove-unnecessary-allocator-usage.md +++ b/text/0000-remove-unnecessary-allocator-usage.md @@ -50,7 +50,7 @@ This RFC proposes to introduce the following new host functions: ``` The signature and behaviour of `ext_storage_read_version_2` and `ext_default_child_storage_read_version_2` is identical to their version 1 equivalent, but the return value has a different meaning. -The new functions directly return the number of bytes written in the `value_out` buffer. If the entry doesn't exist, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `value_out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. +The new functions directly return the number of bytes that were written in the `value_out` buffer. If the entry doesn't exist, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `value_out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. ```wat (func $ext_hashing_keccak_256_version_2 @@ -89,7 +89,7 @@ The new functions directly return the number of bytes written in the `value_out` (param $key_type_id i32) (param $seed i64) (return i32)) ``` -The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. +The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. ```wat (func $ext_default_child_storage_root_version_3 @@ -98,7 +98,7 @@ The behaviour of these functions is identical to their version 1 or version 2 eq (param $out i32)) ``` -The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. +The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. I have taken the liberty to take the version 1 of these functions as a base rather than the version 2, as a PPP deprecating the version 2 of these functions has previously been accepted: . @@ -129,7 +129,7 @@ func $ext_crypto_ecdsa_sign_version_2 (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. +The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. Note that the return value is 0 on success and 1 on failure, while the previous version of these functions write 1 on success (as it represents a SCALE-encoded `Some`) and 0 on failure (as it represents a SCALE-encoded `None`). Returning 0 on success and non-zero on failure is consistent with common practices in the C programming language and is less surprising than the opposite. @@ -140,7 +140,7 @@ Note that the return value is 0 on success and 1 on failure, while the previous (param $sig i32) (param $msg i32) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is the same as in version 2 of these functions: . +The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is the same as in version 2 of these functions: . ```wat (func $ext_offchain_http_request_start_version_2 @@ -149,6 +149,8 @@ The behaviour of these functions is identical to their version 2 equivalents. Th The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns 0. An identifier of 0 is invalid and is reserved to indicate failure. +Host implementaters should be aware that, because a non-zero identifier value was previously valid, this might require slightly more changes to the host than just adding the new function. + ```wat (func $ext_offchain_http_request_write_body_version_2 (param $method i64) (param $uri i64) (param $meta i64) (result i32)) From f5c6459a53bb7cc1a5f95d7cfdc00002c49bf733 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:11:09 +0200 Subject: [PATCH 03/16] More text --- text/0000-remove-unnecessary-allocator-usage.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0000-remove-unnecessary-allocator-usage.md index faea7d81c..a842291ac 100644 --- a/text/0000-remove-unnecessary-allocator-usage.md +++ b/text/0000-remove-unnecessary-allocator-usage.md @@ -190,14 +190,14 @@ No unresolved questions. ## Future Possibilities -The existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Furthermore, runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to updated to not use it. +The existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Furthermore, runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to be updated to not use it. When it comes to removing the allocator usage from the functions that still use it after this RFC, it can in my opinion be done like this: - The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions can be updated to no longer use the allocator by splitting them in two: one function to query the number of items and one function to query an individual item. - The `ext_offchain_local_storage_get` should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. - The `ext_storage_next_key`, `ext_default_child_storage_next_key` should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. -- The `ext_storage_get` and `ext_default_child_storage_get` should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. +- The `ext_storage_get` and `ext_default_child_storage_get` should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. Most of the time (numbers, cryptographic public keys, etc.), the runtime knows ahead of the time the size of the data that it is going to read. Using `read` instead of `get` can be problematic in situations where the size of the data is not known ahead of time, which is `Vec`s and `String`s, as it might require reallocations and mutiple calls to `read` instead of just one to `get`. While `Vec`s and `String`s are pretty uncommon, it is difficult to know ahead of time whether this is an actual problem, and this would need to be benchmarked. A theoretical alternative approach could be to provide an API similar to `mmap`, where the runtime maps a storage value in its memory, but this theoretical approach might not realistically be implementable. Furthermore, the input data provided to the runtime is also allocated using the allocator. New host functions that allow reading the input from the runtime would have to be added. From e212169de372f8a93d4cc8b70686cee2232656e2 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:12:26 +0200 Subject: [PATCH 04/16] More text, I'm done now --- text/0000-remove-unnecessary-allocator-usage.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0000-remove-unnecessary-allocator-usage.md index a842291ac..1b9cb47b7 100644 --- a/text/0000-remove-unnecessary-allocator-usage.md +++ b/text/0000-remove-unnecessary-allocator-usage.md @@ -195,9 +195,9 @@ The existence of the host-side allocator has become questionable over time. It i When it comes to removing the allocator usage from the functions that still use it after this RFC, it can in my opinion be done like this: - The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions can be updated to no longer use the allocator by splitting them in two: one function to query the number of items and one function to query an individual item. -- The `ext_offchain_local_storage_get` should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. -- The `ext_storage_next_key`, `ext_default_child_storage_next_key` should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. -- The `ext_storage_get` and `ext_default_child_storage_get` should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. Most of the time (numbers, cryptographic public keys, etc.), the runtime knows ahead of the time the size of the data that it is going to read. Using `read` instead of `get` can be problematic in situations where the size of the data is not known ahead of time, which is `Vec`s and `String`s, as it might require reallocations and mutiple calls to `read` instead of just one to `get`. While `Vec`s and `String`s are pretty uncommon, it is difficult to know ahead of time whether this is an actual problem, and this would need to be benchmarked. A theoretical alternative approach could be to provide an API similar to `mmap`, where the runtime maps a storage value in its memory, but this theoretical approach might not realistically be implementable. +- The `ext_offchain_local_storage_get` hody gunvyiond should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. +- The `ext_storage_next_key` and `ext_default_child_storage_next_key` host functions should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. +- The `ext_storage_get` and `ext_default_child_storage_get` host functions should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. Most of the time (numbers, cryptographic public keys, etc.), the runtime knows ahead of the time the size of the data that it is going to read. Using `read` instead of `get` can be problematic in situations where the size of the data is not known ahead of time, which is `Vec`s and `String`s, as it might require reallocations and mutiple calls to `read` instead of just one to `get`. While `Vec`s and `String`s are pretty uncommon, it is difficult to know ahead of time whether this is an actual problem, and this would need to be benchmarked. A theoretical alternative approach could be to provide an API similar to `mmap`, where the runtime maps a storage value in its memory, but this theoretical approach might not realistically be implementable. Furthermore, the input data provided to the runtime is also allocated using the allocator. New host functions that allow reading the input from the runtime would have to be added. From bd45a3f8ea226609ffbf8ac305afd451a2cf0d1d Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:19:53 +0200 Subject: [PATCH 05/16] Fix non-zero value for ECDSA recovery --- text/0000-remove-unnecessary-allocator-usage.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0000-remove-unnecessary-allocator-usage.md index 1b9cb47b7..84fa6ee84 100644 --- a/text/0000-remove-unnecessary-allocator-usage.md +++ b/text/0000-remove-unnecessary-allocator-usage.md @@ -140,7 +140,15 @@ Note that the return value is 0 on success and 1 on failure, while the previous (param $sig i32) (param $msg i32) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is the same as in version 2 of these functions: . +The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. + +The non-zero value written on failure is: + +- 1: incorrect value of R or S +- 2: incorrect value of V +- 3: invalid signature + +These values are equal to the values returned on error by the version 2 (see ), but incremented by 1 in order to reserve 0 for success. ```wat (func $ext_offchain_http_request_start_version_2 From 4bc65f50c8915b05f0fe11a042bfdb5054cd3888 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:51:02 +0200 Subject: [PATCH 06/16] Forgot to set the number --- ...ator-usage.md => 0004-remove-unnecessary-allocator-usage.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename text/{0000-remove-unnecessary-allocator-usage.md => 0004-remove-unnecessary-allocator-usage.md} (99%) diff --git a/text/0000-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md similarity index 99% rename from text/0000-remove-unnecessary-allocator-usage.md rename to text/0004-remove-unnecessary-allocator-usage.md index 84fa6ee84..f1e83a303 100644 --- a/text/0000-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -1,4 +1,4 @@ -# RFC-0000: Remove unnecessary host functions allocator usage +# RFC-0004: Remove unnecessary host functions allocator usage | | | | --------------- | ------------------------------------------------------------------------------------------- | From e34bf2c83a49a839269e5f6c5602880fd319384e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 11:51:13 +0200 Subject: [PATCH 07/16] Typo --- text/0004-remove-unnecessary-allocator-usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index f1e83a303..3417474f6 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -203,7 +203,7 @@ The existence of the host-side allocator has become questionable over time. It i When it comes to removing the allocator usage from the functions that still use it after this RFC, it can in my opinion be done like this: - The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions can be updated to no longer use the allocator by splitting them in two: one function to query the number of items and one function to query an individual item. -- The `ext_offchain_local_storage_get` hody gunvyiond should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. +- The `ext_offchain_local_storage_get` host functions should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. - The `ext_storage_next_key` and `ext_default_child_storage_next_key` host functions should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. - The `ext_storage_get` and `ext_default_child_storage_get` host functions should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. Most of the time (numbers, cryptographic public keys, etc.), the runtime knows ahead of the time the size of the data that it is going to read. Using `read` instead of `get` can be problematic in situations where the size of the data is not known ahead of time, which is `Vec`s and `String`s, as it might require reallocations and mutiple calls to `read` instead of just one to `get`. While `Vec`s and `String`s are pretty uncommon, it is difficult to know ahead of time whether this is an actual problem, and this would need to be benchmarked. A theoretical alternative approach could be to provide an API similar to `mmap`, where the runtime maps a storage value in its memory, but this theoretical approach might not realistically be implementable. From 9cd11a6e7ac77b9bf7ca592d184d300f36cdd619 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Jul 2023 20:34:37 +0200 Subject: [PATCH 08/16] More typos fixed --- text/0004-remove-unnecessary-allocator-usage.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index 3417474f6..d8d82879e 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -84,9 +84,9 @@ The new functions directly return the number of bytes that were written in the ` (func $ext_crypto_ed25519_generate_version_2 (param $key_type_id i32) (param $seed i64) (param $out i32)) (func $ext_crypto_sr25519_generate_version_2 - (param $key_type_id i32) (param $seed i64) (return i32)) + (param $key_type_id i32) (param $seed i64) (param $out i32) (return i32)) (func $ext_crypto_ecdsa_generate_version_2 - (param $key_type_id i32) (param $seed i64) (return i32)) + (param $key_type_id i32) (param $seed i64) (param $out i32) (return i32)) ``` The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. @@ -157,7 +157,7 @@ These values are equal to the values returned on error by the version 2 (see Date: Tue, 4 Jul 2023 20:35:13 +0200 Subject: [PATCH 09/16] And another mistake fixed --- text/0004-remove-unnecessary-allocator-usage.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index d8d82879e..ce5a71a48 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -79,8 +79,6 @@ The new functions directly return the number of bytes that were written in the ` (param $data i64) (param $version i32) (param $out i32)) (func $ext_default_child_storage_root_version_3 (param $child_storage_key i64) (param $out i32)) -(func $ext_storage_root_version_3 - (param $out i32)) (func $ext_crypto_ed25519_generate_version_2 (param $key_type_id i32) (param $seed i64) (param $out i32)) (func $ext_crypto_sr25519_generate_version_2 From 45cfba82bb9ce37e605b1402219291d5d84b05e6 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 5 Sep 2023 12:43:25 +0200 Subject: [PATCH 10/16] Add behaviour when `out` is out of range --- text/0004-remove-unnecessary-allocator-usage.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index ce5a71a48..becc4fc72 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -87,7 +87,7 @@ The new functions directly return the number of bytes that were written in the ` (param $key_type_id i32) (param $seed i64) (param $out i32) (return i32)) ``` -The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. +The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. ```wat (func $ext_default_child_storage_root_version_3 @@ -96,7 +96,7 @@ The behaviour of these functions is identical to their version 1 or version 2 eq (param $out i32)) ``` -The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. +The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. I have taken the liberty to take the version 1 of these functions as a base rather than the version 2, as a PPP deprecating the version 2 of these functions has previously been accepted: . @@ -112,7 +112,7 @@ I have taken the liberty to take the version 1 of these functions as a base rath (param $removed_count_out i32) (return i32)) ``` -The behaviour of these functions is identical to their version 2 and 3 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the version 3 and 4 of these functions accepts a `removed_count_out` parameter containing the memory location to a 8 bytes buffer where the host writes the number of keys that were removed in little endian. The functions return 1 to indicate that there are keys remaining, and 0 to indicate that all keys have been removed. +The behaviour of these functions is identical to their version 2 and 3 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the version 3 and 4 of these functions accepts a `removed_count_out` parameter containing the memory location to a 8 bytes buffer where the host writes the number of keys that were removed in little endian. The runtime execution stops with an error if `removed_count_out` is outside of the range of the memory of the virtual machine. The functions return 1 to indicate that there are keys remaining, and 0 to indicate that all keys have been removed. Note that there is an alternative proposal to add new host functions with the same names: . This alternative doesn't conflict with this one except for the version number. One proposal or the other will have to use versions 4 and 5 rather than 3 and 4. @@ -127,7 +127,7 @@ func $ext_crypto_ecdsa_sign_version_2 (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. +The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. Note that the return value is 0 on success and 1 on failure, while the previous version of these functions write 1 on success (as it represents a SCALE-encoded `Some`) and 0 on failure (as it represents a SCALE-encoded `None`). Returning 0 on success and non-zero on failure is consistent with common practices in the C programming language and is less surprising than the opposite. @@ -138,7 +138,7 @@ Note that the return value is 0 on success and 1 on failure, while the previous (param $sig i32) (param $msg i32) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. +The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is: From 3444f30a08429462eb4eae58e73e44357d09f59a Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 22 Sep 2023 09:14:46 +0200 Subject: [PATCH 11/16] Update the RFC to remove the allocator altogether --- ...0004-remove-unnecessary-allocator-usage.md | 187 ++++++++++++++---- 1 file changed, 151 insertions(+), 36 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index becc4fc72..fd080b088 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -1,14 +1,14 @@ -# RFC-0004: Remove unnecessary host functions allocator usage +# RFC-0004: Remove the allocator usage | | | | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | 2023-07-04 | -| **Description** | Add alternatives to host functions that make use of the allocator when unnecessary | +| **Description** | Update the runtime-host interface to no longer make use of a host-side allocator | | **Authors** | Pierre Krieger | ## Summary -Add new versions of host functions in order to avoid using the allocator when the buffer being allocated is of a size known at compilation time. +Update the runtime-host interface to no longer make use of a host-side allocator. ## Motivation @@ -18,20 +18,7 @@ The API of many host functions consists in allocating a buffer. For example, whe Even though no benchmark has been done, it is pretty obvious that this design is very inefficient. To continue with the example of `ext_hashing_twox_256_version_1`, it would be more efficient to instead write the output hash to a buffer that was allocated by the runtime on its stack and passed by pointer to the function. Allocating a buffer on the stack in the worst case scenario simply consists in decreasing a number, and in the best case scenario is free. Doing so would save many Wasm memory reads and writes by the allocator, and would save a function call to `ext_allocator_free_version_1`. -After this RFC, only the following functions have no exact equivalent that doesn't use the allocator: - -- `ext_storage_get` -- `ext_default_child_storage_get` -- `ext_storage_next_key` -- `ext_default_child_storage_next_key` -- `ext_crypto_ed25519_public_keys` -- `ext_crypto_sr25519_public_keys` -- `ext_crypto_ecdsa_public_keys` -- `ext_offchain_network_state` -- `ext_offchain_local_storage_get` -- `ext_offchain_http_response_wait` -- `ext_offchain_http_response_headers` -- `ext_offchain_http_response_read_body` +Furthermore, the existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Furthermore, runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to be updated to not use it. ## Stakeholders @@ -39,7 +26,9 @@ No attempt was made at convincing stakeholders. The writer of this RFC believes ## Explanation -This RFC proposes to introduce the following new host functions: +### New host functions + +This section contains a list of new host functions to introduce. ```wat (func $ext_storage_read_version_2 @@ -52,6 +41,24 @@ This RFC proposes to introduce the following new host functions: The signature and behaviour of `ext_storage_read_version_2` and `ext_default_child_storage_read_version_2` is identical to their version 1 equivalent, but the return value has a different meaning. The new functions directly return the number of bytes that were written in the `value_out` buffer. If the entry doesn't exist, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `value_out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. +The runtime execution stops with an error if `value_out` is outside of the range of the memory of the virtual machine, even if the size of the buffer is 0 or if the amount of data to write would be 0 bytes. + +```wat +(func $ext_storage_next_key_version_2 + (param $key i64) (param $out i64) (return i32)) +(func $ext_default_child_storage_next_key_version_2 + (param $child_storage_key i64) (param $key i64) (param $out i64) (return i32)) +``` + +The behaviour of these functions is identical to their version 1 equivalents. +Instead of allocating a buffer, writing the next key to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing [a pointer-size](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size) to the memory location where the host writes the output. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. +These functions return the size, in bytes, of the next key, or `0` if there is no next key. If the size of the next key is larger than the buffer in `out`, the bytes of the key that fit the buffer are written to `out` and any extra byte that doesn't fit is discarded. + +Some notes: + +- It is never possible for the next key to be an empty buffer, because an empty buffer has no preceding key. For this reason, a return value of `0` can unambiguously be used to indicate the lack of next key. +- The `ext_storage_next_key_version_2` and `ext_default_child_storage_next_key_version_2` are typically used in order to enumerate keys that starts with a certain prefix. Given that storage keys are constructed by concatenating hashes, the runtime is expected to know the size of the next key and can allocate a buffer that can fit said key. + ```wat (func $ext_hashing_keccak_256_version_2 (param $data i64) (param $out i32)) @@ -148,6 +155,30 @@ The non-zero value written on failure is: These values are equal to the values returned on error by the version 2 (see ), but incremented by 1 in order to reserve 0 for success. +```wat +(func $ext_crypto_ed25519_num_public_keys_version_1 + (param $key_type_id i32) (return i32)) +(func $ext_crypto_ed25519_public_key_version_2 + (param $key_type_id i32) (param $key_index i32) (param $out i32)) +(func $ext_crypto_sr25519_num_public_keys_version_1 + (param $key_type_id i32) (return i32)) +(func $ext_crypto_sr25519_public_key_version_2 + (param $key_type_id i32) (param $key_index i32) (param $out i32)) +(func $ext_crypto_ecdsa_num_public_keys_version_1 + (param $key_type_id i32) (return i32)) +(func $ext_crypto_ecdsa_public_key_version_2 + (param $key_type_id i32) (param $key_index i32) (param $out i32)) +``` + +The functions superceded the `ext_crypto_ed25519_public_key_version_1`, `ext_crypto_sr25519_public_key_version_1`, and `ext_crypto_ecdsa_public_key_version_1` host functions. + +Instead of calling `ext_crypto_ed25519_public_key_version_1` in order to obtain the list of all keys at once, the runtime should instead call `ext_crypto_ed25519_num_public_keys_version_1` in order to obtain the number of public keys available, then `ext_crypto_ed25519_public_key_version_2` repeatedly. +The `ext_crypto_ed25519_public_key_version_2` function writes the public key of the given `key_index` to the memory location designated by `out`. The `key_index` must be between 0 (included) and `n` (excluded), where `n` is the value returned by `ext_crypto_ed25519_num_public_keys_version_1`. + +The same explanations apply for `ext_crypto_sr25519_public_key_version_1` and `ext_crypto_ecdsa_public_key_version_1`. + +Host implementers should be aware that the list of public keys (including their ordering) must not change while the runtime is running. This is most likely done by copying the list of all available keys either at the start of the execution or the first time the list is accessed. + ```wat (func $ext_offchain_http_request_start_version_2 (param $method i64) (param $uri i64) (param $meta i64) (result i32)) @@ -160,16 +191,54 @@ Host implementers should be aware that, because a zero identifier value was prev ```wat (func $ext_offchain_http_request_write_body_version_2 (param $method i64) (param $uri i64) (param $meta i64) (result i32)) +(func $ext_offchain_http_response_read_body_version_2 + (param $request_id i32) (param $buffer i64) (param $deadline i64) (result i64)) ``` -The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing two bytes in it, and returning a pointer to it, the new version of this function simply indicates what happened: +The behaviour of these functions is identical to their version 1 equivalent. Instead of allocating a buffer, writing two bytes in it, and returning a pointer to it, the new version of these functions simply indicates what happened: -- 0 on success. -- 1 if the deadline was reached. -- 2 if there was an I/O error while processing the request. -- 3 if the identifier of the request is invalid. +- For `ext_offchain_http_request_write_body_version_2`, 0 on success. +- For `ext_offchain_http_response_read_body_version_2`, 0 or a non-zero number of bytes on success. +- -1 if the deadline was reached. +- -2 if there was an I/O error while processing the request. +- -3 if the identifier of the request is invalid. -These values are equal to the values returned on error by the version 1 (see ), but incremented by 1 in order to reserve 0 for success. +These values are equal to the values returned on error by the version 1 (see ), but tweaked in order to reserve positive numbers for success. + +When it comes to `ext_offchain_http_response_read_body_version_2`, the host implementers must not read too much data at once in order to not create ambiguity in the returned value. Given that the size of the `buffer` is always inferior or equal to 4 GiB, this is not a problem. + +```wat +(func $ext_offchain_http_response_wait_version_2 + (param $ids i64) (param $deadline i64) (param $out i32)) +``` + +The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of this function accepts an `out` parameter containing the memory location where the host writes the output. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. + +The encoding of the response code is also modified compared to its version 1 equivalent and each response code now encodes to 4 little endian bytes as described: + +- 100-999: the request has finished with the given HTTP status code. +- -1 if the deadline was reached. +- -2 if there was an I/O error while processing the request. +- -3 if the identifier of the request is invalid. + +The buffer passed to `out` must always have a size of `4 * n` where `n` is the number of elements in the `ids`. + +```wat +(func $ext_offchain_http_response_header_name_version_1 + (param $request_id i32) (param $header_index i32) (param $out i64) (result i64)) +(func $ext_offchain_http_response_header_value_version_1 + (param $request_id i32) (param $header_index i32) (param $out i64) (result i64)) +``` + +These functions supercede the `ext_offchain_http_response_headers_version_1` host function. + +Contrary to `ext_offchain_http_response_headers_version_1`, only one header indicated by `header_index` can be read at a time. Instead of calling `ext_offchain_http_response_headers_version_1` once, the runtime should call `ext_offchain_http_response_header_name_version_1` and `ext_offchain_http_response_header_value_version_1` multiple times with an increasing `header_index`, until a value of `-1` is returned. + +These functions accept an `out` parameter containing [a pointer-size](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size) to the memory location where the header name or value should be written. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. + +These functions return the size, in bytes, of the header name or header value. If request doesn't exist or is in an invalid state (as documented for `ext_offchain_http_response_headers_version_1`) or the `header_index` is out of range, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. + +If the buffer in `out` is too small to fit the entire header name of value, only the bytes that fit are written and the rest are discarded. ```wat (func $ext_offchain_submit_transaction_version_2 @@ -180,6 +249,57 @@ These values are equal to the values returned on error by the version 1 (see host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to be updated to not use it. +- `ext_input_size_version_1`/`ext_input_read_version_1` is inherently slower than obtaining a buffer with the entire data due to the two extra function calls and the extra copying. However, given that this only happens once per runtime call, the cost is expected to be negligible. +- The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions are likely slightly slower than their deprecated counterparts, but given that they are used only in offchain workers this is acceptable. -When it comes to removing the allocator usage from the functions that still use it after this RFC, it can in my opinion be done like this: - -- The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions can be updated to no longer use the allocator by splitting them in two: one function to query the number of items and one function to query an individual item. -- The `ext_offchain_local_storage_get` host functions should be deprecated in favor of a new `ext_offchain_local_storage_read` host function. -- The `ext_storage_next_key` and `ext_default_child_storage_next_key` host functions should also be updated to get a new `read`-like API. Because storage keys are organized by concatenating hashes, their length is always known ahead of time, and as such a buffer containing the next key can be allocated ahead of time. -- The `ext_storage_get` and `ext_default_child_storage_get` host functions should be deprecated in favor of `ext_storage_read` and `ext_default_child_storage_read`. Most of the time (numbers, cryptographic public keys, etc.), the runtime knows ahead of the time the size of the data that it is going to read. Using `read` instead of `get` can be problematic in situations where the size of the data is not known ahead of time, which is `Vec`s and `String`s, as it might require reallocations and mutiple calls to `read` instead of just one to `get`. While `Vec`s and `String`s are pretty uncommon, it is difficult to know ahead of time whether this is an actual problem, and this would need to be benchmarked. A theoretical alternative approach could be to provide an API similar to `mmap`, where the runtime maps a storage value in its memory, but this theoretical approach might not realistically be implementable. - -Furthermore, the input data provided to the runtime is also allocated using the allocator. New host functions that allow reading the input from the runtime would have to be added. +## Future Possibilities -Because all these changes might be controversial and might require benchmarking, and that the removal of the allocator might also be controversial, I have decided to not include them as part of this RFC. +After this RFC, we can remove from the source code of the host the allocator altogether in a future version, by removing support for all the deprecated host functions. +This would remove the possibility to synchronize older blocks, which is probably controversial and requires a some preparations that are out of scope of this RFC. From f06bd6fe13bb597d7e0e6ca29bd1025b0df1007c Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 22 Sep 2023 11:01:25 +0200 Subject: [PATCH 12/16] Some tweaks --- ...0004-remove-unnecessary-allocator-usage.md | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index fd080b088..057b81982 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -1,4 +1,4 @@ -# RFC-0004: Remove the allocator usage +# RFC-0004: Remove the host-side runtime memory allocator | | | | --------------- | ------------------------------------------------------------------------------------------- | @@ -18,11 +18,11 @@ The API of many host functions consists in allocating a buffer. For example, whe Even though no benchmark has been done, it is pretty obvious that this design is very inefficient. To continue with the example of `ext_hashing_twox_256_version_1`, it would be more efficient to instead write the output hash to a buffer that was allocated by the runtime on its stack and passed by pointer to the function. Allocating a buffer on the stack in the worst case scenario simply consists in decreasing a number, and in the best case scenario is free. Doing so would save many Wasm memory reads and writes by the allocator, and would save a function call to `ext_allocator_free_version_1`. -Furthermore, the existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Furthermore, runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to be updated to not use it. +Furthermore, the existence of the host-side allocator has become questionable over time. It is implemented in a very naive way, and for determinism and backwards compatibility reasons it needs to be implemented exactly identically in every client implementation. Runtimes make substantial use of heap memory allocations, and each allocation needs to go twice through the runtime <-> host boundary (once for allocating and once for freeing). Moving the allocator to the runtime side, while it would increase the size of the runtime, would be a good idea. But before the host-side allocator can be deprecated, all the host functions that make use of it need to be updated to not use it. ## Stakeholders -No attempt was made at convincing stakeholders. The writer of this RFC believes that these changes are pretty non-controversial. +No attempt was made at convincing stakeholders. ## Explanation @@ -38,7 +38,7 @@ This section contains a list of new host functions to introduce. (param $offset i32) (result i64)) ``` -The signature and behaviour of `ext_storage_read_version_2` and `ext_default_child_storage_read_version_2` is identical to their version 1 equivalent, but the return value has a different meaning. +The signature and behaviour of `ext_storage_read_version_2` and `ext_default_child_storage_read_version_2` is identical to their version 1 counterparts, but the return value has a different meaning. The new functions directly return the number of bytes that were written in the `value_out` buffer. If the entry doesn't exist, a value of `-1` is returned. Given that the host must never write more bytes than the size of the buffer in `value_out`, and that the size of this buffer is expressed as a 32 bits number, a 64bits value of `-1` is not ambiguous. The runtime execution stops with an error if `value_out` is outside of the range of the memory of the virtual machine, even if the size of the buffer is 0 or if the amount of data to write would be 0 bytes. @@ -50,14 +50,14 @@ The runtime execution stops with an error if `value_out` is outside of the range (param $child_storage_key i64) (param $key i64) (param $out i64) (return i32)) ``` -The behaviour of these functions is identical to their version 1 equivalents. +The behaviour of these functions is identical to their version 1 counterparts. Instead of allocating a buffer, writing the next key to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing [a pointer-size](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size) to the memory location where the host writes the output. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. These functions return the size, in bytes, of the next key, or `0` if there is no next key. If the size of the next key is larger than the buffer in `out`, the bytes of the key that fit the buffer are written to `out` and any extra byte that doesn't fit is discarded. Some notes: -- It is never possible for the next key to be an empty buffer, because an empty buffer has no preceding key. For this reason, a return value of `0` can unambiguously be used to indicate the lack of next key. -- The `ext_storage_next_key_version_2` and `ext_default_child_storage_next_key_version_2` are typically used in order to enumerate keys that starts with a certain prefix. Given that storage keys are constructed by concatenating hashes, the runtime is expected to know the size of the next key and can allocate a buffer that can fit said key. +- It is never possible for the next key to be an empty buffer, because an empty key has no preceding key. For this reason, a return value of `0` can unambiguously be used to indicate the lack of next key. +- The `ext_storage_next_key_version_2` and `ext_default_child_storage_next_key_version_2` are typically used in order to enumerate keys that starts with a certain prefix. Given that storage keys are constructed by concatenating hashes, the runtime is expected to know the size of the next key and can allocate a buffer that can fit said key. When the next key doesn't belong to the desired prefix, it might not fit the buffer, but given that the start of the key is written to the buffer anyway this can be detected in order to avoid calling the function a second time with a larger buffer. ```wat (func $ext_hashing_keccak_256_version_2 @@ -94,7 +94,7 @@ Some notes: (param $key_type_id i32) (param $seed i64) (param $out i32) (return i32)) ``` -The behaviour of these functions is identical to their version 1 or version 2 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. +The behaviour of these functions is identical to their version 1 or version 2 counterparts. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. ```wat (func $ext_default_child_storage_root_version_3 @@ -103,7 +103,7 @@ The behaviour of these functions is identical to their version 1 or version 2 eq (param $out i32)) ``` -The behaviour of these functions is identical to their version 1 and version 2 equivalents. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. +The behaviour of these functions is identical to their version 1 and version 2 counterparts. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new versions of these functions accepts an `out` parameter containing the memory location where the host writes the output. The output is always of a size known at compilation time. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. I have taken the liberty to take the version 1 of these functions as a base rather than the version 2, as a PPP deprecating the version 2 of these functions has previously been accepted: . @@ -119,7 +119,7 @@ I have taken the liberty to take the version 1 of these functions as a base rath (param $removed_count_out i32) (return i32)) ``` -The behaviour of these functions is identical to their version 2 and 3 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the version 3 and 4 of these functions accepts a `removed_count_out` parameter containing the memory location to a 8 bytes buffer where the host writes the number of keys that were removed in little endian. The runtime execution stops with an error if `removed_count_out` is outside of the range of the memory of the virtual machine. The functions return 1 to indicate that there are keys remaining, and 0 to indicate that all keys have been removed. +The behaviour of these functions is identical to their version 2 and 3 counterparts. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the version 3 and 4 of these functions accepts a `removed_count_out` parameter containing the memory location to a 8 bytes buffer where the host writes the number of keys that were removed in little endian. The runtime execution stops with an error if `removed_count_out` is outside of the range of the memory of the virtual machine. The functions return 1 to indicate that there are keys remaining, and 0 to indicate that all keys have been removed. Note that there is an alternative proposal to add new host functions with the same names: . This alternative doesn't conflict with this one except for the version number. One proposal or the other will have to use versions 4 and 5 rather than 3 and 4. @@ -134,7 +134,7 @@ func $ext_crypto_ecdsa_sign_version_2 (param $key_type_id i32) (param $key i32) (param $msg i64) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 1 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. +The behaviour of these functions is identical to their version 1 counterparts. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. If the public key can't be found in the keystore, these functions return `1` and do not write anything to `out`. Note that the return value is 0 on success and 1 on failure, while the previous version of these functions write 1 on success (as it represents a SCALE-encoded `Some`) and 0 on failure (as it represents a SCALE-encoded `None`). Returning 0 on success and non-zero on failure is consistent with common practices in the C programming language and is less surprising than the opposite. @@ -145,7 +145,7 @@ Note that the return value is 0 on success and 1 on failure, while the previous (param $sig i32) (param $msg i32) (param $out i32) (return i64)) ``` -The behaviour of these functions is identical to their version 2 equivalents. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. +The behaviour of these functions is identical to their version 2 counterparts. The new versions of these functions accept an `out` parameter containing the memory location where the host writes the signature. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the function wouldn't write anything to `out`. The signatures are always of a size known at compilation time. On success, these functions return `0`. On failure, these functions return a non-zero value and do not write anything to `out`. The non-zero value written on failure is: @@ -184,7 +184,7 @@ Host implementers should be aware that the list of public keys (including their (param $method i64) (param $uri i64) (param $meta i64) (result i32)) ``` -The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns 0. An identifier of 0 is invalid and is reserved to indicate failure. +The behaviour of this function is identical to its version 1 counterpart. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns 0. An identifier of 0 is invalid and is reserved to indicate failure. Host implementers should be aware that, because a zero identifier value was previously valid, this might require slightly more changes to the host than just adding the new function. @@ -195,7 +195,7 @@ Host implementers should be aware that, because a zero identifier value was prev (param $request_id i32) (param $buffer i64) (param $deadline i64) (result i64)) ``` -The behaviour of these functions is identical to their version 1 equivalent. Instead of allocating a buffer, writing two bytes in it, and returning a pointer to it, the new version of these functions simply indicates what happened: +The behaviour of these functions is identical to their version 1 counterpart. Instead of allocating a buffer, writing two bytes in it, and returning a pointer to it, the new version of these functions simply indicates what happened: - For `ext_offchain_http_request_write_body_version_2`, 0 on success. - For `ext_offchain_http_response_read_body_version_2`, 0 or a non-zero number of bytes on success. @@ -212,9 +212,9 @@ When it comes to `ext_offchain_http_response_read_body_version_2`, the host impl (param $ids i64) (param $deadline i64) (param $out i32)) ``` -The behaviour of this function is identical to its version 1 equivalent. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of this function accepts an `out` parameter containing the memory location where the host writes the output. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. +The behaviour of this function is identical to its version 1 counterpart. Instead of allocating a buffer, writing the output to it, and returning a pointer to it, the new version of this function accepts an `out` parameter containing the memory location where the host writes the output. The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine. -The encoding of the response code is also modified compared to its version 1 equivalent and each response code now encodes to 4 little endian bytes as described: +The encoding of the response code is also modified compared to its version 1 counterpart and each response code now encodes to 4 little endian bytes as described below: - 100-999: the request has finished with the given HTTP status code. - -1 if the deadline was reached. @@ -247,7 +247,7 @@ If the buffer in `out` is too small to fit the entire header name of value, only (param $request_id i32) (param $name i64) (param $value i64) (result i32)) ``` -The behaviour of these functions is identical to their version 1 equivalent. Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions simply return 1 or 0. +The behaviour of these functions is identical to their version 1 counterpart. Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions simply return 1 or 0. ```wat (func $ext_offchain_local_storage_read_version_1 @@ -282,13 +282,13 @@ When a runtime function is called, the host uses the allocator to allocate memor The `ext_input_size_version_1` host function returns the size in bytes of the input data. The `ext_input_read_version_1` host function copies some data from the input data to the memory of the runtime. The `offset` parameter indicates the offset within the input data where to start copying, and must be inferior or equal to the value returned by `ext_input_size_version_1`. The `out` parameter is [a pointer-size](https://spec.polkadot.network/chap-host-api#defn-runtime-pointer-size) containing the buffer where to write to. -The runtime execution stops with an error if `out` is outside of the range of the memory of the virtual machine, even if the amount of data to copy would be 0 bytes. +The runtime execution stops with an error if `offset` is strictly superior to the size of the input data, or if `out` is outside of the range of the memory of the virtual machine, even if the amount of data to copy would be 0 bytes. ### Other changes In addition to the new host functions, this RFC proposes two changes to the runtime-host interface: -- The following function signature is now accepted for runtime entry points: `(func (result i64))`. +- The following function signature is now also accepted for runtime entry points: `(func (result i64))`. - Runtimes no longer need to expose a constant named `__heap_base`. All the host functions that are being superceded by new host functions are now considered deprecated and should no longer be used. @@ -302,9 +302,7 @@ The following other host functions are similarly also considered deprecated: ## Drawbacks -- This RFC might be difficult to implement in Substrate due to the internal code design. It is not clear to the author of this RFC how difficult it would be. - -- In some situations, the runtime might still have to call `ext_allocator_malloc_version_1` then call one of the new functions, which is slightly less performant than when the two operations are combined into one call. The author of this RFC believes that this negligible, and that the performance saved by not allocating a buffer in most situations is worth the trade-off. +This RFC might be difficult to implement in Substrate due to the internal code design. It is not clear to the author of this RFC how difficult it would be. ## Prior Art @@ -317,8 +315,13 @@ The changes in this RFC would need to be benchmarked. This involves implementing It is expected that most host functions are faster or equal speed to their deprecated counterparts, with the following exceptions: - `ext_input_size_version_1`/`ext_input_read_version_1` is inherently slower than obtaining a buffer with the entire data due to the two extra function calls and the extra copying. However, given that this only happens once per runtime call, the cost is expected to be negligible. + - The `ext_crypto_*_public_keys`, `ext_offchain_network_state`, and `ext_offchain_http_*` host functions are likely slightly slower than their deprecated counterparts, but given that they are used only in offchain workers this is acceptable. +- It is unclear how replacing `ext_storage_get` with `ext_storage_read` and `ext_default_child_storage_get` with `ext_default_child_storage_read` will impact performances. + +- It is unclear how the changes to `ext_storage_next_key` and `ext_default_child_storage_next_key` will impact performances. + ## Future Possibilities After this RFC, we can remove from the source code of the host the allocator altogether in a future version, by removing support for all the deprecated host functions. From 857225e2659b547525175c7f346c54e103dbf425 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 27 Nov 2023 14:28:20 +0100 Subject: [PATCH 13/16] Clarify `ext_crypto_ed25519_public_key_version_2` --- text/0004-remove-unnecessary-allocator-usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index 057b81982..e64dceb78 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -173,7 +173,7 @@ These values are equal to the values returned on error by the version 2 (see Date: Mon, 27 Nov 2023 14:32:45 +0100 Subject: [PATCH 14/16] Clarify ext_offchain_http_request_add_header_version_2 and ext_offchain_submit_transaction_version_2 --- text/0004-remove-unnecessary-allocator-usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index e64dceb78..45b20cc88 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -247,7 +247,7 @@ If the buffer in `out` is too small to fit the entire header name of value, only (param $request_id i32) (param $name i64) (param $value i64) (result i32)) ``` -The behaviour of these functions is identical to their version 1 counterpart. Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions simply return 1 or 0. +Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions return `0` or `-1`, where `0` indicates success and `-1` indicates failure. The runtime must interpret any non-`0` value as failure, but the client must always return `-1` in case of failure. ```wat (func $ext_offchain_local_storage_read_version_1 From 3c120e4ba2f0c17f9d180038a483c10795d507b8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 27 Nov 2023 14:32:58 +0100 Subject: [PATCH 15/16] Clarify ext_offchain_http_request_start_version_2 --- text/0004-remove-unnecessary-allocator-usage.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index 45b20cc88..593f66900 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -184,9 +184,7 @@ Host implementers should be aware that the list of public keys (including their (param $method i64) (param $uri i64) (param $meta i64) (result i32)) ``` -The behaviour of this function is identical to its version 1 counterpart. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns 0. An identifier of 0 is invalid and is reserved to indicate failure. - -Host implementers should be aware that, because a zero identifier value was previously valid, this might require slightly more changes to the host than just adding the new function. +The behaviour of this function is identical to its version 1 counterpart. Instead of allocating a buffer, writing the request identifier in it, and returning a pointer to it, the version 2 of this function simply returns the newly-assigned identifier to the HTTP request. On failure, this function returns `-1`. An identifier of `-1` is invalid and is reserved to indicate failure. ```wat (func $ext_offchain_http_request_write_body_version_2 From 89a4fcc5586ffd43741600c1105870fb85dc156f Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 27 Nov 2023 14:34:17 +0100 Subject: [PATCH 16/16] Actually lets use 1 and not -1 --- text/0004-remove-unnecessary-allocator-usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0004-remove-unnecessary-allocator-usage.md b/text/0004-remove-unnecessary-allocator-usage.md index 593f66900..6c62e80c1 100644 --- a/text/0004-remove-unnecessary-allocator-usage.md +++ b/text/0004-remove-unnecessary-allocator-usage.md @@ -245,7 +245,7 @@ If the buffer in `out` is too small to fit the entire header name of value, only (param $request_id i32) (param $name i64) (param $value i64) (result i32)) ``` -Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions return `0` or `-1`, where `0` indicates success and `-1` indicates failure. The runtime must interpret any non-`0` value as failure, but the client must always return `-1` in case of failure. +Instead of allocating a buffer, writing `1` or `0` in it, and returning a pointer to it, the version 2 of these functions return `0` or `1`, where `0` indicates success and `1` indicates failure. The runtime must interpret any non-`0` value as failure, but the client must always return `1` in case of failure. ```wat (func $ext_offchain_local_storage_read_version_1