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

Feat: Check and increment nonce #42

Merged
merged 3 commits into from
Apr 27, 2021
Merged

Conversation

birchmd
Copy link
Member

@birchmd birchmd commented Apr 23, 2021

To prevent replaying transactions, we need to check the nonce matches the nonce of the account, and increment it if it matches.

In the case of directly calling call or deploy_code replaying the transaction is not a concern because it originates from Near, and so is protected by the base system, but I still increment the nonce in those cases to be consistent.

One issue with this is the case where the transaction runs out of Near gas. In that case the state changes are rolled back, including the incremented nonce, which is different behaviour from if the transaction ran on Ethereum directly. I think the best way to handle this would be to make running out of Near gas impossible by setting appropriate limits on the Eth gas. But this probably is not feasible in the short term. We might need to leave this as a known bug for now. I'm interested in what others think about this issue @artob @djsatok @joshuajbouw @mrLSD

@birchmd birchmd requested a review from artob April 23, 2021 18:50
@artob artob self-assigned this Apr 23, 2021
@artob artob added the C-enhancement Category: New feature or request label Apr 23, 2021
@artob artob requested a review from joshuajbouw April 23, 2021 21:30
src/lib.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@joshuajbouw
Copy link
Contributor

If in a usual NEAR contract execution, the nonce being rolled back on failure, I think that should be fine to continue with, as long as it doesn't hinder any data between ETH. But if otherwise, a sort of mandatory cost is a good idea, if thats the only way to solve that.

@birchmd
Copy link
Member Author

birchmd commented Apr 26, 2021

If in a usual NEAR contract execution, the nonce being rolled back on failure

It's not that on the Near base runtime nonces are always rolled back, that is not the case. The problem here is that we track the nonce explicitly as part of the EVM contract state, and contract state changes are rolled back on OOG (this is true on Eth as well of course). So when we run out of Near gas, the EVM contract state changes are rolled back (it is just a normal contract on Near after all), including the nonce change.

A more "near native" solution than what I suggested above (limiting the Eth gas so that we can never run out of Near gas), would be to take advantage of the Near asyc runtime. We could save the nonce change as a transaction first, then execute the EVM call as an async call (receipt) afterwards. So if the receipt runs out of gas, then only those state changes will be rolled back, not the nonce change since it was already committed. One drawback of this solution is that EVM transactions would then always be split over two Near blocks, which could be awkward for tooling.

@joshuajbouw joshuajbouw self-assigned this Apr 26, 2021
@joshuajbouw
Copy link
Contributor

The first solution in the description seems the most favourable for me, and likely will be the solution we will require when we go to main net. The two transaction with the first being the nonce does seem quite awkward for the reasons you posted.

I say, lets leave it open ended for now, be aware of it, and ensure we track it before main net.

Copy link
Contributor

@artob artob left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Nice job!

@artob artob merged commit 241b540 into aurora-is-near:master Apr 27, 2021
This was referenced Apr 28, 2021
mrLSD pushed a commit that referenced this pull request May 5, 2021
* Link to docs in the README. (#18)

* Change deprecated `u64::max_value` to `u64::MAX`. (#38)

* Support custom error messages. (#40)

* Implement `begin_chain` for evm-bully. (#30)

* Implement a faucet method. (#39)

* Implement all Istanbul HF precompiles. (#21)

* Check and increment nonces. (#42)

* Fix the RIPEMD160 and ModExp precompiles. (#44)

* Implement a first draft of `COINBASE` and `GASLIMIT`. (#47)

* Refactor and improve error handling. (#49)

* Replace `raw_call` with the new `submit` API. (#48)

The `raw_call` method is hereby removed in favor of the new `submit` method
that has an extended ABI capable of returning a transaction's revert status
and logged events.

Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Add benchmarks for common EVM operations. (#41)

* Merge branch 'master' into improved-evm-token-logic

* Update error handling to `master`

* fix missing import

* cargo fmt

* Ensure ETH transfers return an execution result. (#48)

* Update to `master`

* fix str types

Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>
mrLSD added a commit that referenced this pull request May 5, 2021
* Added prover & extended types

* Mode Borsh args from types to parameters

* Added fungible tokens

* Added eth-connector

* Modify assert for fee

* Fix formatting

* Extend eth-conenctor with EVM token logic

* Changed eth-connector deposit logic

* Added changes for ETH deposit/withdraw and Engine changes

* Mint ETH-tokens

* Added: transfer_eth

* ETH withdraw basic method

* eth-withdraw validation structure & modified Deposit-eth fields

* eth transfer and withdraw logic

* eip712 message verifier - started encoding

* added encode-packed

* virefy EIP712 message for withdraw

* Changed EIP712 message fields

* Modify logs for EIP712 messages

* Test EIP712

* Tests EIP712

* Integration tests for encode_withdraw_eip712

* Integration tests for encode_withdraw_eip712

* EIP712-Withdraw: improvements and fixes.

* EIP712-Withdraw: fixed encoding rules and order.
* EIP712-Withdraw: `verify_withdraw_eip712` returns `true` only if the
  sender address equals to the address of message signer.
* EIP712-Withdraw: update tests.
* EIP712-Withdraw: refactoring.
* ethabit::encode_token_packed: use right-padded encoding for `Address`.
* WithdrawFromEthCallArgs: fixed `amount` type conversion.

* Extend tests for eth-connector

* eth-connector test deposit & balance & total_supply

* Imporved tests

* FT tests

* Fixed verify_transfer_eip712

* Change test_withdraw_near

* Tests for: ft_transfer, ft_transfer_call

* test_eth_deposit_balance_total_supply

* test and ifx: deposit_eth, withdraw_near

* References in fungible token (#29)

* Use references in fungible_token to avoid cloning

* cargo fmt

* Fix: hide logging behind feature flag

* Remove eth-conenctor transfer methods and deposit for new design

* Completed Deposit logic

* Fix clippy; added comments; improved ft_transfer_call

* Extend external functions for eth-connector

* Added deploy_evm_token

* Added ft_on_transfer logic

* Changed ft_on_transfer & remove json depends

* Changed deposit logic and fixed transfers

* Added register relayer

* Added message coder for ft_transfer_call

* ft_on_transfer - added logic for erc20

* Impoved ft_on_transfer

* ft_on_transfer: call erc20 contract adn send fee to Relayer. Added logs

* eth-connector: Removed unsued methods

* tests: deposit & fixed init test

* tests: depoist, withdraw

* tests: fix test_withdraw_near

* Eth-connector: never skip bridge call.

* Tests: fix ft_transfer_call

* ft_transfer_call - changed gas amountr

* Fixed: test_eth_deposit_balance_total_supply, test_ft_transfer

* Added: test_ft_transfer_call_near_eth

* Clippy fix

* Added test_ft_transfer_call_erc20

* Added test_ft_transfer_call_erc20

* tests: ft_transfer_call for ERC20 changes

* Fix finish_deposit - promise flow when failed for ft_transfer_call

* added: test_deposit_with_same_proof

* Improved EVM token master branch update (#50)

* Link to docs in the README. (#18)

* Change deprecated `u64::max_value` to `u64::MAX`. (#38)

* Support custom error messages. (#40)

* Implement `begin_chain` for evm-bully. (#30)

* Implement a faucet method. (#39)

* Implement all Istanbul HF precompiles. (#21)

* Check and increment nonces. (#42)

* Fix the RIPEMD160 and ModExp precompiles. (#44)

* Implement a first draft of `COINBASE` and `GASLIMIT`. (#47)

* Refactor and improve error handling. (#49)

* Replace `raw_call` with the new `submit` API. (#48)

The `raw_call` method is hereby removed in favor of the new `submit` method
that has an extended ABI capable of returning a transaction's revert status
and logged events.

Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Add benchmarks for common EVM operations. (#41)

* Merge branch 'master' into improved-evm-token-logic

* Update error handling to `master`

* fix missing import

* cargo fmt

* Ensure ETH transfers return an execution result. (#48)

* Update to `master`

* fix str types

Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Update src/connector.rs misspel

Co-authored-by: Joshua J. Bouw <[email protected]>

* Update src/connector.rs change constants error

Co-authored-by: Joshua J. Bouw <[email protected]>

Co-authored-by: Septen <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Joshua J. Bouw <[email protected]>
Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>
@birchmd birchmd deleted the check-nonce branch May 6, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants