feat: add WASM host functions (Wasmi version)#6075
feat: add WASM host functions (Wasmi version)#6075mvadari wants to merge 127 commits intoripple/wasmifrom
Conversation
* Reject non-canonical binaries * Review fixes * Cleanup Number2 class * Use enum instead of 0
* add readme to src/xrpl/app/wasm * important block * respond to copilot
* Add functions with many parameters * Add 10k locals function * Module with 5k functions * fix typo Co-authored-by: Mayukha Vadari <mvadari@gmail.com> --------- Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
* clean up some hf code * fix comments * fix ubsan * Revert "fix ubsan"
* Remove ability to re-use wasm module * Check that HFS object is always new * Fix clang format * Remove perf tests * temp build fix * Fix merge
|
|
||
| if (slice->size() == uint256::bytes) | ||
| { | ||
| if (auto ret = hf->isAmendmentEnabled(uint256::fromVoid(slice->data())); *ret == 1) |
There was a problem hiding this comment.
*ret without .has_value() is a dangerous code pattern. It could be UB potentially. While isAmendmentEnabled cannot return an error now, it's not guaranteed that this will not happen in the future. If the hf class is not set correctly (let's say you use base class for some reason), it will actually return an unexpected and instead of proper error handling we will have an UB
| auto const* runtime = reinterpret_cast<InstanceWrapper const*>(hf->getRT()); | ||
| int index = 0; | ||
|
|
||
| if (params->data[1].of.i32 + params->data[3].of.i32 > maxWasmParamLength) |
There was a problem hiding this comment.
I probably raised this before, but can this overflow? These two params are provided by the user right?
| return; | ||
|
|
||
| Number::setround(static_cast<Number::rounding_mode>(mode)); | ||
| Number::setMantissaScale(MantissaRange::mantissa_scale::small); |
There was a problem hiding this comment.
While I don't see the issue here, I see the comments in original implementation:
/** Changes which mantissa scale is used for normalization.
*
* If you think you need to call this outside of unit tests, no you don't.
*/
static void
setMantissaScale(MantissaRange::mantissa_scale scale);
Could you bring this up for discussion?
| if (STI_ARRAY == field->getSType()) | ||
| { | ||
| auto const* arr = static_cast<STArray const*>(field); | ||
| if (sfieldCode >= arr->size()) |
There was a problem hiding this comment.
is there any chance sfieldCode can be negative? This will cause an issue
| // ========================================================= | ||
|
|
||
| Expected<Bytes, HostFunctionError> | ||
| WasmHostFunctionsImpl::getNFT(AccountID const& account, uint256 const& nftId) const |
There was a problem hiding this comment.
this function name is a bit misleading, should it be getNFTUri or something like that?
| int64_t const x = gas >= impFunc.gas ? gas - impFunc.gas : 0; | ||
|
|
||
| if (runtime->setGas(x) < 0) | ||
| { | ||
| wasm_trap_t* trap = reinterpret_cast<wasm_trap_t*>( | ||
| WasmEngine::instance().newTrap("can't set gas")); // LCOV_EXCL_LINE | ||
| return Unexpected(trap); // LCOV_EXCL_LINE | ||
| } | ||
|
|
||
| if (gas < impFunc.gas) | ||
| { | ||
| wasm_trap_t* trap = | ||
| reinterpret_cast<wasm_trap_t*>(WasmEngine::instance().newTrap("hf out of gas")); | ||
| return Unexpected(trap); | ||
| } | ||
|
|
There was a problem hiding this comment.
In case there's not enough gas, we first set gas to 0, and only then we check if there's enough gas to run the function. Is this intentional? Should we first check if there's enough and only then deduct?
| static std::string_view const W_ENV = "env"; | ||
| static std::string_view const W_HOST_LIB = "host_lib"; | ||
| static std::string_view const W_MEM = "memory"; | ||
| static std::string_view const W_STORE = "store"; | ||
| static std::string_view const W_LOAD = "load"; | ||
| static std::string_view const W_SIZE = "size"; | ||
| static std::string_view const W_ALLOC = "allocate"; | ||
| static std::string_view const W_DEALLOC = "deallocate"; | ||
| static std::string_view const W_PROC_EXIT = "proc_exit"; | ||
|
|
||
| static std::string_view const ESCROW_FUNCTION_NAME = "finish"; | ||
|
|
||
| uint32_t const MAX_PAGES = 128; // 8MB = 64KB*128 | ||
|
|
There was a problem hiding this comment.
This creates separate copies per translation unit. Use inline constexpr
| // std::cout << "runEscrowWasm, mod size: " << wasmCode.size() | ||
| // << ", gasLimit: " << gasLimit << ", funcName: " << funcName; |
| float f32; | ||
| double f64; |
There was a problem hiding this comment.
These params are not used if I understand correctly, do you plan to use them later?
|
|
||
| // Create and instantiate the module. | ||
| if (wasmCode.empty()) | ||
| throw std::runtime_error("empty nodule"); |
High Level Overview of Change
This PR adds all the Wasmi integration code and the host functions specified in XLS-102. It also adds tests for all of this.
Note for reviewers: 100k lines of this PR are just WASM-compiled test fixtures, and another 5k lines are WASM test fixture source code. So while this PR is still quite large, it's not as large as it seems on the surface.
Context of Change
This PR depends on #5999 and replaces #5791
It is part of an effort to split up the Smart Escrow work into smaller, more managable-to-review PRs.
XLS-102: https://xls.xrpl.org/xls/XLS-0102-wasm-vm.html
Stack of PRs (from
developoutwards):SmartEscrowasSupported::yes#5811Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
N/A
Test Plan
Several tests are added to this PR.