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

JSON-RPC: Lower to call to a "send" if the target doesn't exist #1302

Closed
Stebalien opened this issue Dec 16, 2022 · 10 comments
Closed

JSON-RPC: Lower to call to a "send" if the target doesn't exist #1302

Stebalien opened this issue Dec 16, 2022 · 10 comments

Comments

@Stebalien
Copy link
Member

Stebalien commented Dec 16, 2022

Currently, we lower a call to send if the parameters are non-empty. However:

  1. This isn't technically correct. We should be able to invoke a contract with no parameters.
  2. This means that any sends to an actor that doesn't yet exist fail instead of just succeeding and doing nothing.

We should do what we're currently doing in FEVM and:

  1. If the target actor is an EVM actor, invoke it (method 2).
  2. If the target actor is an embryo or doesn't exist: replace the "invoke contract" method number with "method send".
  3. Otherwise, fail.

Or, at least, that's what we should do. Unfortunately, this impact signature validity, so I'm going to take a walk now and think about it.

@Stebalien Stebalien added the P0 P0: Critical blocker label Dec 16, 2022
@Stebalien Stebalien added this to the M2.1 (rr10) Carbonado milestone Dec 16, 2022
@jennijuju
Copy link
Member

i feel like you might mean

we lower a call to send if the parameters ARE empty
(its conflicting with point 1 i feel like

@jennijuju
Copy link
Member

so I'm going to take a walk now and think about it.

Hows your walk.

@Stebalien
Copy link
Member Author

Stebalien commented Dec 16, 2022

Not helpful (the walk, that is)

@mriise
Copy link
Contributor

mriise commented Dec 16, 2022

I think we shouldn't replicate what we are doing in FEVM into FVM, and we should also not do what we are currently doing. It should be a user-space decision whether or not to do this auto method num translation at all. FVM as a system should really avoid transform messages as much as possible.

Invoke with no params should either actually invoke the actor with params at the provided method or error, nothing else. This means that sends to things that don't exist will always fail with "no actor at address". Sends are explicit.

I think this sort of behavior embedded in the system in general will cause edge cases to pop up later on. The complexity exists in EVM because it's an EVM complexity, but that should stay in tooling.

@Stebalien
Copy link
Member Author

This isn't about the FVM, but about the current MMVP account abstraction:

  1. Metamask user sends a message to an address that doesn't exist on-chain. This currently "works" because, if the user specifies no params, we turn it into a bare send (we've built this into how we translate transactions into Filecoin messages).
  2. A metamask user tries to perform a value transfer to some smart contract (again, no params). Usually, the smart contract would run code (potentially recording the transfer, or emitting an event). But because we're reducing this to a "bare send", no code will run on chain.

That's the kind of thing I'm trying to fix here, but I'm not sure if we can come up with a good fix.

@Stebalien
Copy link
Member Author

Ok, I'm voting to just drop this and keep the current logic. I.e., we don't support sending with parameters to a target contract that doesn't yet exist.

cc @raulk

@maciejwitowski
Copy link
Contributor

maciejwitowski commented Jan 16, 2023

@raulk @Stebalien Sooo...can we drop it? 🤞

@Stebalien
Copy link
Member Author

Possible solution: Replace "placeholders".

Currently, when sending to an f4 address, we automatically create a generic "placeholder" to store the funds.

Instead, we could have the address manager register a code-cid that should be deployed on first send to an address.

  • This code CID wouldn't be special, the new actor would be an actor like any other.
  • No constructor would run to preserve the method-0 semantics of not running any code. In the case of FEVM, the default here would be an EthAccount.

Then, to deploy a contract and/or account, we'd "upgrade" said account into an EVM actor.

The tricky parts here are:

  1. The interplay between this and account abstraction as upgrading an account is a mutation. However, this isn't unworkable. Executing in two stages (validation, then execution) fixes the issue.
  2. Doing this quickly. At a minimum, this requires changes to FIP0048 and upgrade support.

@Stebalien
Copy link
Member Author

An alternative is to reverse filecoin-project/FIPs#473 (comment) and make placeholders accept arbitrary methods (as in Ethereum). However, this has security implications around re-orgs. I.e.:

  1. User A constructs a new contract at address C (message 1).
  2. User B invokes C to deposit some funds in their account (message 2).
  3. A reorg happens and message 2 lands before message 1. In this case:
    1. If placeholders reject arbitrary method calls, message 2 will fail.
    2. If placeholders accept arbitrary method calls, user B's funds will get deposited in C but C's code won't run (because it hasn't been deployed yet) so there will be no record of the deposit.

This is how Ethereum works but we'd rather not lock ourselves into the same bad design.


There may be a middle-ground given that placeholders are only used by the EVM (for now), but it would be pretty hacky.

@maciejwitowski maciejwitowski assigned Stebalien and unassigned raulk Jan 23, 2023
Stebalien added a commit to filecoin-project/lotus that referenced this issue Jan 26, 2023
This:

1. Always uses InvokeContract, never reducing to a method-0 value transfer.
2. Always encodes the parameters as a cbor byte array for simplicity.

This removes _many_ edge cases and makes it possible to run contract
logic with no parameters.

Depends on filecoin-project/ref-fvm#1567

Fixes the issues outlined in filecoin-project/ref-fvm#1302
Stebalien added a commit to filecoin-project/lotus that referenced this issue Jan 27, 2023
This:

1. Always uses InvokeContract, never reducing to a method-0 value transfer.
2. Always encodes the parameters as a cbor byte array for simplicity.

This removes _many_ edge cases and makes it possible to run contract
logic with no parameters.

Depends on filecoin-project/ref-fvm#1567

Fixes the issues outlined in filecoin-project/ref-fvm#1302
@Stebalien
Copy link
Member Author

We now always call with InvokeContract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants