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.

Sign up for GitHub

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

Jump to bottom

Improve async message checks #4706

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Improve async message checks #4706

merged 3 commits into from
Jun 25, 2024

Conversation

Leo-Besancon
Copy link
Collaborator

@Leo-Besancon Leo-Besancon commented Jun 12, 2024 •
edited
Loading

This improves checks on async messages.

  • Avoid adding a message to the pool if the validity slot makes it not executable
  • Better catch errors to avoid async message being partly executed (load module fail but transfer coin successful and not reverted)
  • document all added functions
  • try in sandbox /simulation/labnet
    • if part of node-launch, checked using the resync_check flag
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Leo-Besancon Leo-Besancon mentioned this pull request Jun 12, 2024
Closed
@Leo-Besancon Leo-Besancon force-pushed the fix_checks_async_msg branch from 1ad9edd to d29eb55 Compare June 13, 2024 13:26
@Leo-Besancon Leo-Besancon force-pushed the fix_checks_async_msg branch from 9b497eb to a9fe1dc Compare June 18, 2024 06:22
@Leo-Besancon Leo-Besancon changed the base branch from main to mainnet_2_3 June 18, 2024 06:22
@Leo-Besancon Leo-Besancon marked this pull request as ready for review June 19, 2024 12:27
bilboquet
bilboquet approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@bilboquet bilboquet left a comment

Choose a reason for hiding this comment

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

I'm fine with the code.
It would be nice to have a TU for each change.
That would help to track the intent of the commit (but that's far from mandatory).

@Leo-Besancon
Copy link
Collaborator Author

I'm fine with the code. It would be nice to have a TU for each change. That would help to track the intent of the commit (but that's far from mandatory).

Thanks, I'll see what I can do :)

@Leo-Besancon Leo-Besancon merged commit 77544e2 into mainnet_2_3 Jun 25, 2024
@Leo-Besancon Leo-Besancon deleted the fix_checks_async_msg branch June 25, 2024 06:07
Leo-Besancon added a commit that referenced this pull request Jul 2, 2024
@Leo-Besancon
Improve async message checks (#4706)
b64e360 
* Improve async message checks

* Change checks for async messages

* Add unit tests
Leo-Besancon added a commit that referenced this pull request Jul 17, 2024
@Leo-Besancon
Improve async message checks (#4706)
a6b46f0 
* Improve async message checks

* Change checks for async messages

* Add unit tests
Leo-Besancon added a commit that referenced this pull request Aug 1, 2024
@sydhds @Leo-Besancon @modship
Consistent expiry period for async message and block operations (#4722)
736a9ed 
* Asc message execution - requery message bytecode after each message execution (#4710)

* Requery bytecode

* cargo fmt

* fix call stack inconsistency (#4709)

* Improve async message checks (#4706)

* Improve async message checks

* Change checks for async messages

* Add unit tests

* Fix ledger change to take into account cancelled message balance change (#4715)

* Take again the speculative changes after async message cancellation

* use .apply() to merge the two LedgerChanges

* Fix: we cannot combine two ledger changes with apply

* avoid cloning the changes

* Remove comment

* Fix async msg same slot (#4718)

* fix open rpc spec (#4716)

* Add eliminated_new_messages in eliminated_msg

---------

Co-authored-by: Modship <[email protected]>

* Consistent expiry period for async message and block operations

* Update message validity for expiration

* Minor comment fix

---------

Co-authored-by: Leo-Besancon <[email protected]>
Co-authored-by: Modship <[email protected]>
Leo-Besancon added a commit that referenced this pull request Oct 16, 2024
@Leo-Besancon @modship
Add SC recursion limit (#4729)
3833083 
* Asc message execution - requery message bytecode after each message execution (#4710)

* Requery bytecode

* cargo fmt

* fix call stack inconsistency (#4709)

* Improve async message checks (#4706)

* Improve async message checks

* Change checks for async messages

* Add unit tests

* Fix ledger change to take into account cancelled message balance change (#4715)

* Take again the speculative changes after async message cancellation

* use .apply() to merge the two LedgerChanges

* Fix: we cannot combine two ledger changes with apply

* avoid cloning the changes

* Remove comment

* Fix async msg same slot (#4718)

* fix open rpc spec (#4716)

* Add eliminated_new_messages in eliminated_msg

---------

Co-authored-by: Modship <[email protected]>

* Add initial code for recursion limit

* Latest runtime

* Run CI on PRs based on mainnet_2_3

* fmt

* Fix config and add UTs

* Update scenarios_mandatories.rs

* Review comments (CI for all branches starting with "mainnet_" + comment)

* Update ci.yml

* Remove manual increment / decrement in interface implementation

* fmt + update sc_runtime + fix warning

* Update test

* Update constants.rs

* Updated execution config for tests

* Updated usize -> u16 for recursion counter and limits

* Update test comments

* Add comments regarding the needs of this limits

* Update sc-runtime branch

---------

Co-authored-by: Modship <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@modship modship modship approved these changes

@bilboquet bilboquet bilboquet approved these changes

@AurelienFT AurelienFT Awaiting requested review from AurelienFT

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Leo-Besancon @bilboquet @modship