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

Allow increasing an account's nonce without changing its state #796

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Jul 15, 2024

This small PR removes the restriction from nonce to be changed without changing the account state, which was suggested in the related issue: #765.

@Fumuran Fumuran linked an issue Jul 15, 2024 that may be closed by this pull request
@Fumuran Fumuran requested a review from bobbinth July 15, 2024 15:09
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

One question: are there any checks in the transaction kernel MASM for this? Might be good to have a test which ensures that this condition is handled correctly in the transaction kernel as well.

@Fumuran Fumuran requested a review from bobbinth July 16, 2024 13:29
@Fumuran Fumuran force-pushed the andrew-remove-nonce-restriction branch from 0f09350 to f5f10d1 Compare July 16, 2024 13:58
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of nits inline. After these are addressed, we can merge.


## ACCOUNT PROCEDURE WRAPPERS
## ========================================================================================
#TODO: Move this into an account library
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this TODO mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the initial idea was to move account procedure wrappers to the library, but I'm not sure that it is valid for my test. I'll remove this line.

Comment on lines 401 to 407
proc.incr_nonce
call.{ACCOUNT_INCR_NONCE_MAST_ROOT}
# => [0]

drop
# => []
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need this as a separate procedure? Seems simple enough to be just inside the begin ... end statements below.

Comment on lines 412 to 413
## Update the account nonce
## ------------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this header here is helpful as the script is simple enough (similar comment about some other headers in this script).

@Fumuran Fumuran merged commit dd5b7b9 into next Jul 18, 2024
12 checks passed
@Fumuran Fumuran deleted the andrew-remove-nonce-restriction branch July 18, 2024 09:13
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.

Allow increasing an account's nonce without changing its state
2 participants