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

EVM: Implement CALLACTOR/METHODNUM and REVERT abort semantics #633

Merged
merged 12 commits into from
Sep 12, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Sep 10, 2022

This pr completes our cross-contract call story:

  • Implements the CALLACTOR and METHODNUM opcodes to call a specific actor method.
  • Removes the InvokeParams envelope in contract invocation, it is now raw data.
  • which allows us to lift the restrictions on CALL so that it can call the InvokeContract method in any actor.
  • and provides real REVERT semantics, handled by call/callactor, so that we leave the correct success value on the stack and let the contract handle reverts however it pleases.

Notes:

  • REVERT doesn't propagate return data yet; this will be fixed in a subsequent pr, as it requires fvm support for aborting with data.
  • The assembler doesn't let us specify arbitrary opcodes, so I had to resort to a gnarly hack and introduce our own mnemonic opcodes for callactor and methodnum.

Partially addresses filecoin-project/ref-fvm#811.

Closes filecoin-project/ref-fvm#805.

@vyzo vyzo requested a review from raulk September 10, 2022 09:52
@maciejwitowski maciejwitowski added this to the M2.1 (r3) Copper milestone Sep 12, 2022
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Nice! Just two nits for nicer code.

Comment on lines +260 to +264
let input_data = if let Some(MemoryRegion { offset, size }) = input_region {
&memory[offset..][..size.get()]
} else {
&[]
}
Copy link
Member

Choose a reason for hiding this comment

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

Just use unwrap_or_default() here:

Suggested change
let input_data = if let Some(MemoryRegion { offset, size }) = input_region {
&memory[offset..][..size.get()]
} else {
&[]
}
let input_data = input_region
.map(|MemoryRegion { offset, size }| &memory[offset..][..size.get()])
.unwrap_or_default();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm, more scala.....

What's up with the war on IF? Programming is impossible without it.

Also note that the compiler will have to apply smarts to ultimately generate the code we are substituting, so the only thing achieved by this so called "idiomatic" nonsense is obfuscation and ever increasing compile times....

Copy link
Member

Choose a reason for hiding this comment

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

It's not Scala, it's Rust. You have an Option here and a type that implements Default. So doing this is the natural way. The Rust compiler optimises for code like this, but we can check with https://godbolt.org if you're in doubt (not sure if it supports Wasm).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesnt resolve either obfuscation or ever increasing compile times ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, I just to be clear here: I am not getting git blamed for this. If you feel too strongly about it, feel free to apply the obfuscator in the EXT* pr :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and don't forget the environment 😆
Then I guess we are already using a small nation's energy output to compile our warez, so that's just a drop in the bucket.

actors/evm/src/interpreter/instructions/call.rs Outdated Show resolved Hide resolved
Comment on lines 87 to 101
// this is a hack to support mnemonics for the FEVM extension opcodes
// it is really ugly, but the etk assmebler doesn't currently support any way to
// directly embed (otherwise invalid) asm instructions in the stream... sigh.
// Ideally we would just do them as macros like
// %macro methodnum()
// %0xb1
// %end
// Note that to add insult to injury, macros cannot %include_hex... double sigh.
// So fuck it, we'll just hack this until there is support.
// See also https://github.com/quilt/etk/issues/110
fn with_fevm_extensions(body: &str) -> String {
body.to_owned()
.replace("@callactor", "%include_hex(\"tests/opcodes/callactor.hex\")")
.replace("@methodnum", "%include_hex(\"tests/opcodes/methodnum.hex\")")
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice work figuring out a workaround!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was really annoying.... Not perfect, but we can live with it until upstream gets support for arbitrary instructions. If they don't act on my issue, I'll send them a pr.

@vyzo
Copy link
Contributor Author

vyzo commented Sep 12, 2022

we had a github down snafu, will edit a comment and do another push.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #633 (0c25f19) into next (bf3f20b) will decrease coverage by 0.03%.
The diff coverage is 81.18%.

❗ Current head 0c25f19 differs from pull request most recent head 234434d. Consider uploading reports for the commit 234434d to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #633      +/-   ##
==========================================
- Coverage   82.17%   82.14%   -0.04%     
==========================================
  Files         111      111              
  Lines       19635    19691      +56     
==========================================
+ Hits        16136    16176      +40     
- Misses       3499     3515      +16     
Impacted Files Coverage Δ
actors/evm/src/interpreter/opcode.rs 19.04% <ø> (ø)
actors/evm/src/interpreter/output.rs 8.33% <ø> (ø)
actors/evm/src/lib.rs 74.62% <69.23%> (-2.43%) ⬇️
actors/evm/src/interpreter/instructions/call.rs 83.08% <84.28%> (+1.43%) ⬆️
actors/evm/src/interpreter/execution.rs 88.88% <100.00%> (+0.17%) ⬆️
actors/evm/src/interpreter/instructions/memory.rs 78.20% <100.00%> (+1.28%) ⬆️
actors/miner/src/testing.rs 91.76% <0.00%> (-0.99%) ⬇️
actors/reward/src/lib.rs 83.73% <0.00%> (+0.81%) ⬆️
... and 2 more

@vyzo vyzo merged commit f5be9a9 into next Sep 12, 2022
@vyzo vyzo deleted the next-callactor branch September 12, 2022 12:21
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Oct 3, 2022
…in-project#633)

* implement CALLACTOR/METHODNUM

* signal EVM_CONTRACT_REVERTED error on reverts

* handle REVERT in CALL/CALLACTOR result

* update comments about REVERT semantics

* simplify call error handling

* fix tests

* fix empty input data handling for calls

* add mnemonic opcodes for CALLACTOR and METHODNUM to assembler

such a hack, but no other way.

* add methodnum and callactor tests

* rustfmt

* lift return

* fix comment
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.

4 participants