Implement delegate_call #80

ermalkaleci · 2024-10-12T17:07:41Z

Closes #67

TODO:

Update pallet-revive to take address instead of code hash

xermicus · 2024-10-14T06:33:59Z

crates/integration/contracts/Delegate.sol

+    address public sender;
+    uint256 public value;
+
+    event DidSetVars();


Testing this is irrelevant here? Keeping the fixtures as concise as possible eases debugging them :)

xermicus · 2024-10-14T06:34:11Z

crates/integration/src/tests.rs

@@ -43,6 +43,7 @@ test_spec!(call, "Caller", "Call.sol");
 test_spec!(transfer, "Transfer", "Transfer.sol");
 test_spec!(return_data_oob, "ReturnDataOob", "ReturnDataOob.sol");
 test_spec!(immutables, "Immutables", "Immutables.sol");
+test_spec!(delegate, "Delegate", "Delegate.sol");


This could use some better test coverage, i.e. calling into non-existent accounts, plain accounts, recursively into self (from runtime code and the constructor).

xermicus · 2024-10-14T06:35:07Z

crates/llvm-context/src/polkavm/evm/call.rs

+    let extcodehash_pointer =
+        context.build_alloca_at_entry(context.word_type(), "extcodehash_pointer");
+
+    context.build_runtime_call(


I think with the current runtime this isn’t necessarily correct (might never be written to, so reading from it is potentially UB).

The reason delegate_call accepts a code hash instead of an address is probably so that people can upload code that they can delegate call into without having to instantiate first. But this isn’t expressible in Solidity (if it was it would be problematic because of the immutables - we assume to be instantiated first). Ideally for Solidity, the runtime API would take care of the code hash image lookup. Moving it into the runtime reduces contract code size and closes the door for related bugs in contracts. But it would need either a change or a new API (to preserve to use case of not requiring to instantiate new code hashes first).

If we decide to stick with the current version of delegate_call, I’d like this PR to wait for #77 to avoid too much churn.

I would prefer we change pallet-revive to accept address rather than code hash. I was trying to work with what we have

Yes the pallet doing the address lookup would definitively be preferred here.

FYI the pallet is under construction too, it isn't deployed anywhere. At that stage we are free to make breaking changes however we wish, as long as they are justified.

Maybe I should have stated that earlier. Both the compiler and the pallet are new, in the works and not yet deployed anywhere. But they are not entirely new code bases. We generally try to adapt the EVM behavior, as to introduce not too many differences from what people expect on EVM :)

So the rule of thumb is, anything that differs should be investigated and changed. It doesn't mean that you have to do it all (even though we can use any helping hands) - if you feel like it is getting too tiring I can open corresponding polkadot-sdk issues and add them to the tracking issue.

I thought pallet-revive will work with ink! and rust contracts and thought we need to somehow make revive work around that. If that's not that case then I can open PR on pallet-revive

ink! will work fine on the revive-runtime since all changes can be reflected in a new ink! version. We need the runtime to align with the incumbent platform and not vice-versa.

I can open PR on pallet-revive

Great, thanks! Just link this discussion there for context. I'm un-opinionated how this should exactly look like - having distinct delegate_call_by_hash and delegate_call_by_address syscalls seems reasonable. Or just remove the ability to delegate call by hashes altogether if it is not required anymore. But I don't know.

With immutables being a thing now does it even make sense to support delegate calling by code hash? We probably just want to remove it and just allow by contract address.

Yeah the nature of immutables turns delegate calling by hash into a potential footgun. I'm in favor of changing it to take an address instead, it's not required for Solidity. Should it for some reason turn out super viable for other languages we can still add it later.

xermicus · 2024-10-14T06:35:24Z

crates/llvm-context/src/polkavm/evm/call.rs

+    let output_offset = context.safe_truncate_int_to_xlen(output_offset)?;
+    let output_length = context.safe_truncate_int_to_xlen(output_length)?;
+
+    // TODO: What to supply here? Is there a weight to gas?


We need some further investigation on this. Why is gas not a supported parameter in the pallet? Can it be supported? If so, that needs either an issue or PR for the pallet. Otherwise the gas argument should be removed from this function altogether and the reason documented.

Yeah no specific reason. Probably an oversight because we thought it doesn't make sense still you are calling in your own contracts storage anyways which is quite sensitive. We probably want to just make delegate call another call flag. Since we are also only supplying an address instead of a code hash.

I think we should replace weight (ref_time, proof_size) with just gas on revive. Solidity has no concept of PoV and all weight (substrate) related operations should be handled by pallet-revive (gas to weight and so on)

It cannot really be handled by pallet-revive alone. Since Solidity has no concept of it we have to assume some default limits for proof size and storage deposit limit. As of right now the default is just "use all of the remaining". This is the most compatible but doesn't allow proper limiting. The plan is to expose the extended interface via pre-compile to allow Solidity contracts to limit all dimensions.

That being said, we decided to leave all parameters on the host function and revive just fills them out with 0. This way other languages can at least use them.

Yes you are right, for Solidity, ideally we just pass through what we get and let the pallet handle it. For other languages however this might be undesirable since they can just decide to not ignore it. Mapping everything it into the 1D (ref_time) space implies wasting block-space. Which is especially undesired for other languages like Rust/ink! because we expect people to use those for implementing more compute heavy logic. So to me it seems that ideally we'd have both. Or, since this is a pointer anyways, it could point to something like enum Weights { Gas(U256), Native { ref_time: u64, proof_size: u64 } }

True but I don't see how we can match 1 unit of gas with 1 unit of time (we may come close though). Anyway renaming to gas_limit and storage_limit makes more sense to me.

True but I don't see how we can match 1 unit of gas with 1 unit of time (we may come close though)

We are doing this: 1 gas = 1 ref_time = 1 picosecond of execution on the reference machine

Why renaming it? I don't see how just renaming anything would help in any way?

Why renaming it?

To make it less confusing. Just an idea, I am fine either way

Ok I see but gas_limit and storage_limit are terms that neither exist in EVM nor in Substrate :) I think that this would only increase the confusion.

xermicus · 2024-10-14T06:35:32Z

crates/llvm-context/src/polkavm/evm/call.rs

+    let output_length_pointer = context.build_alloca_at_entry(context.xlen_type(), "output_length");
+    context.build_store(output_length_pointer, output_length)?;
+
+    let flags = context.xlen_type().const_int(0u64, false);


Delegate calls can be reentrant.

pallet-revive doesn't allow reentrant for delegate_call https://github.com/paritytech/polkadot-sdk/blob/d1c115b6197bf6c45d5640594f0432e6c2781a4f/substrate/frame/revive/src/wasm/runtime.rs#L1030

Ah okay. Would be good to know the reason for this and document it :) I mean ideally this would be changed in the pallet to better match the EVM semantics.

At first I thought pallet-revive was updated to fully support revive but now it seems more just a copy of pallet-contracts

Yeah, pallet-revive was started as a fork of pallet-contracts :) Since the contracts pallet is already close to what we need and heavily audited, we decided to fork it instead of starting from scratch.

xermicus · 2024-10-14T06:35:43Z

crates/llvm-context/src/polkavm/evm/call.rs

+        ],
+    );
+
+    context.build_byte_swap(context.build_load(extcodehash_pointer, "extcodehash_value")?)?;


Why is this needed?

xermicus · 2024-10-14T06:35:58Z

crates/llvm-context/src/polkavm/evm/call.rs

    _gas: inkwell::values::IntValue<'ctx>,
-    _address: inkwell::values::IntValue<'ctx>,
+    address: inkwell::values::IntValue<'ctx>,
    _value: Option<inkwell::values::IntValue<'ctx>>,


Should be removed.

…te_call

implement delegate_call

64390a2

xermicus reviewed Oct 14, 2024

View reviewed changes

ermalkaleci added 2 commits October 17, 2024 15:27

Merge branch 'main' of github.com:ermalkaleci/revive into feat/delega…

2dd478c

…te_call

update delegate_call to use address, gas_limit

5636824

ermalkaleci marked this pull request as draft October 17, 2024 14:23

cleanup

9f85c92

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement delegate_call #80

Implement delegate_call #80

ermalkaleci commented Oct 12, 2024 •

edited

Loading

xermicus Oct 14, 2024

xermicus Oct 14, 2024

xermicus Oct 14, 2024

ermalkaleci Oct 14, 2024

xermicus Oct 14, 2024

xermicus Oct 14, 2024 •

edited

Loading

ermalkaleci Oct 14, 2024

xermicus Oct 14, 2024 •

edited

Loading

xermicus Oct 14, 2024

xermicus Oct 14, 2024

athei Oct 16, 2024

xermicus Oct 16, 2024

xermicus Oct 14, 2024

athei Oct 16, 2024

ermalkaleci Oct 16, 2024

athei Oct 16, 2024

xermicus Oct 16, 2024 •

edited

Loading

ermalkaleci Oct 17, 2024

athei Oct 17, 2024

xermicus Oct 17, 2024 •

edited

Loading

ermalkaleci Oct 17, 2024

xermicus Oct 17, 2024

xermicus Oct 14, 2024

ermalkaleci Oct 14, 2024

xermicus Oct 14, 2024 •

edited

Loading

ermalkaleci Oct 14, 2024

xermicus Oct 14, 2024 •

edited

Loading

xermicus Oct 14, 2024

xermicus Oct 14, 2024

Implement delegate_call #80

Are you sure you want to change the base?

Implement delegate_call #80

Conversation

ermalkaleci commented Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermalkaleci commented Oct 12, 2024 •

edited

Loading

xermicus Oct 14, 2024 •

edited

Loading

xermicus Oct 14, 2024 •

edited

Loading

xermicus Oct 16, 2024 •

edited

Loading

xermicus Oct 17, 2024 •

edited

Loading

xermicus Oct 14, 2024 •

edited

Loading

xermicus Oct 14, 2024 •

edited

Loading