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

[replay] Add transactions to skip for V2 loader #15207

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Nov 6, 2024

Description

There are well-known differences between existing loader and new loader & code cache, see here: https://docs.google.com/spreadsheets/d/16IVAe72g3Rcf3LqMKaj80a0Isa8ma3SaJFMVzOtVOfI/edit?usp=sharing. We should skip those.

Replay run: https://github.com/aptos-labs/aptos-core/actions/runs/11727539596

How Has This Been Tested?

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link
Contributor Author

georgemitenkov commented Nov 6, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@georgemitenkov georgemitenkov changed the title [replay] Add tns to skip [replay] Add txns to skip Nov 6, 2024
@georgemitenkov georgemitenkov changed the title [replay] Add txns to skip [replay] Add txns to skip for V2 loader Nov 6, 2024
@georgemitenkov georgemitenkov changed the title [replay] Add txns to skip for V2 loader [replay] Add transactions to skip for V2 loader Nov 6, 2024
@georgemitenkov georgemitenkov mentioned this pull request Nov 6, 2024
24 tasks
@georgemitenkov georgemitenkov marked this pull request as ready for review November 6, 2024 09:13
@georgemitenkov georgemitenkov requested a review from a team as a code owner November 6, 2024 09:13
@georgemitenkov georgemitenkov changed the base branch from george/parent-block-checks to main November 9, 2024 16:58
@@ -90,7 +90,7 @@ jobs:
SUB_DIR: e1
HISTORY_START: 518000000
#TXNS_TO_SKIP: 12253479 12277499 148358668
TXNS_TO_SKIP: "0"
TXNS_TO_SKIP: 523296049 523298111 575378008 575383660 575455170 575457845 575461986 575470789 596888095 612286393 981890562 1011802725 1030023658 1652667187 1652669110 1652686860 1652689144 1652722847 1654190659 1654191460 1673664597 1730942524 1730944006 1730945331 1730945814 1730946863 1730952284 1730956071 1730959853 1730967212 1731006027 1759904543 1792070645 1797308520 1798657294 1806178096
Copy link
Contributor

Choose a reason for hiding this comment

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

@sherry-x are we fine with skipping transactions on mainnet?

@georgemitenkov how recent are these transactions? If leader v2 is onchain flag - what do these represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this range, the first skip is 27.03.24 and the last is basically now: 17.10.24. The reason why we want to skip is that we can completely remove old V1 code then, and the trade off few dozen of transactions that publish code seems good.

See the spreadsheet for actual divergences, those are only due to bugs in old loader (went to through them one by one). For instance, we were flushing module cache and losing error messages, there are a number of cases where the difference is None vs Some(msg) in VMStatus. There are also 2 other cases: 1) init_module not linking correctly before, 2) not topologically sorted bundles not linking correctly and failing to publish.

Ideally, I would not skip but specify the difference in write set, but we cannot do it with the current replay, hence skipping and documenting in a spreadsheet what is the diff.

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.

2 participants