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(eth-implicit-accounts): Stabilize protocol change #11765

Merged
merged 28 commits into from
Jul 17, 2024

Conversation

birchmd
Copy link
Contributor

@birchmd birchmd commented Jul 10, 2024

This PR includes changes to the wallet contract based on the audit performed by AuditOne. With the audit complete, we are also ready to stabilize the eth-implicit accounts protocol feature. This is also done as part of this PR.

This is a relatively large PR. To make things easier for nearcore reviewers I recommend looking only at the last commit (which does the protocol stabilization). The various changes to the wallet contract done in the other commits have been reviewed by the auditors that raised the issues each commit is addressing.

birchmd added 21 commits July 1, 2024 15:34
…uring the transaction. Rational: this allows relayers to pay for the gas on a user's behalf (by initiating the transaction from their own account) without also needing to pay for other base token transfers involved in the transaction (eg minting wrapped NEAR).
…Near does not guarantee execution order of receipts, so user actions may still be executed out of order even if transactions with sequential nonces arrive in the correct order; the only way to guarantee sequential order of execution is to prevent simultanious in-flight transactions.
…s. Rational: this prevents faulty relayers from griefing users by intentionally submitting transactions incorrectly to prevent their execution. If the nonce is not incremented then an honest relayer can still send the user's original transaction.
…ract call fails. This prevents the user from taking funds from a relayer by intentionally making failed calls. This does not protect the relayer from its own mistakes (e.g. incorrect nonce or invalid transaction); the relayer must still be confident the cross-contract call will execute when choosing to attach NEAR.
@birchmd birchmd requested a review from a team as a code owner July 10, 2024 18:23
@staffik
Copy link
Contributor

staffik commented Jul 12, 2024

Will review it today, but it will require a code owner review too.

Copy link
Contributor

@staffik staffik left a comment

Choose a reason for hiding this comment

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

LGTM!

@bowenwang1996
Copy link
Collaborator

@birchmd could you fix the failing tests? Thanks!

…eeded because testnet has stateless validation, but mainnet does not and doing the stateless upgrade plus another protocol change does not work in the pytests.
@birchmd
Copy link
Contributor Author

birchmd commented Jul 15, 2024

@bowenwang1996

RE: test failures

The test failures were all in pytest related to stopping a current mainnet node and starting a node from the current branch. I think the reason the tests we failing is because stateless validation is not on mainnet yet and the pytest framework is not setup to do the stateless migration followed by another protocol change. Switching the pytest setup to compare against the current testnet release instead of mainnet made some of the tests pass. But the database migration test still fails. When it tries to restart the node it panics with an error about missing a trie value for congestion info:

2024-07-15T16:05:44.501891Z ERROR chain: Failed to get the genesis congestion infos. err=StorageError(MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j))
thread 'main' panicked at neard/src/cli.rs:564:14:
start_with_config: Storage Error: MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j)

I do not think this failure is relayed to my change because if I apply ba06adc directly on top of current master I get the same error. So my best guess is that something with congestion info is not currently being properly committed to the trie, but I am not sure. Do you have any suggestions what I could try to fix it?

@bowenwang1996
Copy link
Collaborator

@bowenwang1996

RE: test failures

The test failures were all in pytest related to stopping a current mainnet node and starting a node from the current branch. I think the reason the tests we failing is because stateless validation is not on mainnet yet and the pytest framework is not setup to do the stateless migration followed by another protocol change. Switching the pytest setup to compare against the current testnet release instead of mainnet made some of the tests pass. But the database migration test still fails. When it tries to restart the node it panics with an error about missing a trie value for congestion info:

2024-07-15T16:05:44.501891Z ERROR chain: Failed to get the genesis congestion infos. err=StorageError(MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j))
thread 'main' panicked at neard/src/cli.rs:564:14:
start_with_config: Storage Error: MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j)

I do not think this failure is relayed to my change because if I apply ba06adc directly on top of current master I get the same error. So my best guess is that something with congestion info is not currently being properly committed to the trie, but I am not sure. Do you have any suggestions what I could try to fix it?

@Longarithm could you take a look?

@staffik
Copy link
Contributor

staffik commented Jul 16, 2024

@bowenwang1996
RE: test failures
The test failures were all in pytest related to stopping a current mainnet node and starting a node from the current branch. I think the reason the tests we failing is because stateless validation is not on mainnet yet and the pytest framework is not setup to do the stateless migration followed by another protocol change. Switching the pytest setup to compare against the current testnet release instead of mainnet made some of the tests pass. But the database migration test still fails. When it tries to restart the node it panics with an error about missing a trie value for congestion info:

2024-07-15T16:05:44.501891Z ERROR chain: Failed to get the genesis congestion infos. err=StorageError(MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j))
thread 'main' panicked at neard/src/cli.rs:564:14:
start_with_config: Storage Error: MissingTrieValue(TrieStorage, D28wpBm6P7quQjYKPCLAWUutVxaCTVwuahCKeYCmzu2j)

I do not think this failure is relayed to my change because if I apply ba06adc directly on top of current master I get the same error. So my best guess is that something with congestion info is not currently being properly committed to the trie, but I am not sure. Do you have any suggestions what I could try to fix it?

@Longarithm could you take a look?

Alex is OOO, I am looking into it.

@staffik
Copy link
Contributor

staffik commented Jul 16, 2024

@birchmd I would add these 2 commits: 43c5764 and 1029bd1 instead of this one: ba06adc.

Just waiting for a resolution of my Zulip question: https://near.zulipchat.com/#narrow/stream/295302-general/topic/Protocol.20version.2070

birchmd and others added 4 commits July 16, 2024 19:35
…his is needed because testnet has stateless validation, but mainnet does not and doing the stateless upgrade plus another protocol change does not work in the pytests."

This reverts commit ba06adc.
@birchmd
Copy link
Contributor Author

birchmd commented Jul 16, 2024

Thanks @staffik ! I've added your changes. It's still not clear to me why testing upgrading against testnet doesn't work, but I'm glad your changes to pass the tests!

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.77%. Comparing base (54bfccb) to head (fae3a0c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11765   +/-   ##
=======================================
  Coverage   71.77%   71.77%           
=======================================
  Files         796      796           
  Lines      162935   162932    -3     
  Branches   162935   162932    -3     
=======================================
+ Hits       116949   116950    +1     
+ Misses      40933    40928    -5     
- Partials     5053     5054    +1     
Flag Coverage Δ
backward-compatibility 0.23% <0.00%> (ø)
db-migration 0.23% <0.00%> (ø)
genesis-check 1.35% <0.00%> (ø)
integration-tests 37.91% <14.28%> (+0.06%) ⬆️
linux 71.39% <100.00%> (+0.08%) ⬆️
linux-nightly 71.34% <100.00%> (-0.02%) ⬇️
macos 54.66% <85.71%> (+0.11%) ⬆️
pytests 1.58% <0.00%> (+<0.01%) ⬆️
sanity-checks 1.38% <0.00%> (ø)
unittests 66.22% <100.00%> (+<0.01%) ⬆️
upgradability 0.28% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bowenwang1996 bowenwang1996 added this pull request to the merge queue Jul 17, 2024
Merged via the queue into near:master with commit fe4ac1a Jul 17, 2024
29 of 30 checks passed
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.

None yet

3 participants