Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework substrate mock runtime #1291

Merged
merged 63 commits into from
Apr 28, 2023
Merged

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Apr 27, 2023

Complete rewrite of the substrate mock runtime:

  • Updates to a wasmi version with the new API (that's a breaking change requiring some substantial changes anyways)
  • Reworked the API of MockSubstrate to hide all logic behind functions. This reduces the API surface to a set of functions on a struct, which will make future changes easier.
  • Re-implement the host functions so that they resemble up-to-date pallet-contracts more closely
  • Introducing separation between Wasm modules (= code), contracts (= code + storage) and accounts (= address +balance + optional contract). This lines the mock implementation up a bit closer to how real nodes work, making implementing additional features in the future easier.

I uncovered a bug in the process. The following contract (adapted from the calls::try_catch_constructor test) traps the execution with MemoryOutOfBounds:

contract c {
    constructor() payable {}

    function test() public returns (int32) {
        int32 x = 0;
        try new other(true) {
            x = 1;
        } catch (bytes memory _c) {
            x = 2;
        }
        return x;
    }
}

contract other {
    constructor(bool rev) public {
        if (rev) {
            revert("foo");
        }
    }

    function _ext() public {}
}

This code also fails on a real substrate contracts node (though this returns just ContractTrapped, obscuring the underlying reason).. Hence I concluded that the out of bounds memory access most likely stems from a bug in the compiler and disabled the failing test for now.

xermicus and others added 30 commits April 19, 2023 16:23
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
xermicus and others added 6 commits April 25, 2023 21:40
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
@xermicus xermicus marked this pull request as ready for review April 27, 2023 17:13
@xermicus xermicus requested review from seanyoung and removed request for seanyoung April 27, 2023 17:15
@xermicus xermicus force-pushed the update-wasmi branch 6 times, most recently from a099271 to 962cadd Compare April 27, 2023 17:33
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
@xermicus xermicus requested a review from seanyoung April 27, 2023 19:58
xermicus and others added 4 commits April 27, 2023 22:28
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, this code is much, much better!

tests/wasm_host_attr/Cargo.toml Outdated Show resolved Hide resolved
let dest_ptr: u32 = args.nth_checked(0)?;
let len_ptr: u32 = args.nth_checked(1)?;
#[seal(0)]
fn seal_transfer(account_ptr: u32, _: u32, value_ptr: u32, _: u32) -> Result<u32, Trap> {
Copy link
Contributor

@seanyoung seanyoung Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seal_tranfer no longer cheecks that the 2nd and 4th argument are correct; we should ensure that our code sets the correct length, else it will not work on a real node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I added those checks

output_ptr: u32,
output_len_ptr: u32,
) -> Result<u32, Trap> {
let input = read_buf(mem, input_ptr, input_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code ensured that _flags was set to zero

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added the check again

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus merged commit adac837 into hyperledger:main Apr 28, 2023
15 checks passed
@xermicus xermicus deleted the update-wasmi branch May 15, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants