-
Notifications
You must be signed in to change notification settings - Fork 486
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
feat: Support injection of ethereum logs for transactions. #1041
base: master
Are you sure you want to change the base?
feat: Support injection of ethereum logs for transactions. #1041
Conversation
6886070
to
f1ee266
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from on memory to storage based evm logs has impact on transaction throughput, both on execution time and pov size. Also, logs data
is unbounded, we cannot store it on events (or CurrentLogs
for that matter, if that happens to be read somewhere in the STF, that unbounded data needs to be fully available in the merkle proof).
<CurrentLogs<T>>::append(&log); | ||
<Pallet<T>>::deposit_event(Event::<T>::Log { log }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good, for 375 gas you can do 2 db writes.
There is an assertion in The only real overhead is: checking CurrentLogs storage the first time it is accessed, wasm-to-host memory copy on append call, and host-to-wasm copy on the transaction end. Also, unbounded log data is a problem with the current approach too. |
e2e7495
to
2312aac
Compare
Allow outer code to inject logs for currently running ethereum transaction.
Allow outer code to inject ethereum logs, without running an ethereum transaction.
2312aac
to
2a94e33
Compare
Ah I see missed that part, sounds good then. Still, there are projects not using |
6e3e102
to
82ab91c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impl wise LGTM but needs to be thoroughly tested
e21ed21
to
c6267dc
Compare
Is this supposed to go into production runtime? If not, please add a feature gate. |
c6267dc
to
38dae31
Compare
38dae31
to
2bba44e
Compare
It is for production. You can see use case here https://github.com/UniqueNetwork/unique-chain/blob/b86b27d80045514c2aa82c2d28b4251b17fc71b2/pallets/nonfungible/src/lib.rs#LL524C22-L524C22. Substrate call is emitting both a substrate and an ethereum events. |
Have you considered implementing the |
This function is callable from both precompile When it is called from substrate side, we still want to see this Log on ethereum side, so we need to inject transaction anyway. |
@CertainLach Have you ever considered constructing a standalone ethereum transaction and invoke You can invoke apply inside your burn substrate call. |
@boundless-forest In our chain, NFT/Fungible/Refungible collections are implemented in pallets, and those pallets are exposed as extrinsics. Thus, both substrate users and ethereum users/contracts may manipulate tokens. We want all token manipulations to be represented as logs on ethereum side, both in the case of ethereum call and in the case of substrate extrinsic call. The other problem this PR solves is substrate event order in case of precompile call. #[pallet::event]
enum Event {
TestEvent(i32),
}
fn precompile_test() {
deposit_event(Event::TestEvent(2));
} event TestEvent(int value);
function test() {
emit TestEvent(1);
precompile.precompileTest();
emit TestEvent(3);
} The expected order of events on the substrate side is
However, because of current event processing (They are stored in memory and only stored after evm execution: https://github.com/paritytech/frontier/blob/master/frame/evm/src/runner/stack.rs#L264), the resulting event order is
Also, this PR exposes events emitted during the |
Your goal is to create Ethereum logs when the users call the paricular substrate extrinsics, and the log can be indexed later by ethereum receipt. Within the
|
If you don't want the overhead of running EVM, you can also just insert a fake transaction into |
To fix the log order issue, you'll want to modify this function: https://github.com/paritytech/frontier/blob/master/frame/evm/src/runner/stack.rs#L565 Instead of inserting into the intermediate Note that this only works in our specific case, as we use |
They are handled the same way as storage access, so this PR unifies storage and log manipulations, they are now both reverted at the same time.
Sometimes we are already in the context of precompile execution, as this function can be called from both substrate extrinsics and ethereum transactions. We can have You don't like automatic flushing done by SignedExtenstion, or the problem is in storing current logs in in-memory storage? |
Will this be merged into the master branch? We are experiencing the same situation, any suggestions or reference code? |
Ethereum logs could be injected for currently running Ethereum transactions or without running an Ethereum transaction.
It could be used to emit ERC721 events while executing substrate transaction for example.