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

Problem: fee deduction not compatible with parallel execution #447

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Apr 2, 2024

Solution:

  • use virtual send

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@yihuang yihuang requested a review from mmsqe April 2, 2024 05:04
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 26.31579% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 63.29%. Comparing base (59586a5) to head (abc59a5).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #447      +/-   ##
===========================================
- Coverage    63.36%   63.29%   -0.08%     
===========================================
  Files          125      125              
  Lines        12061    12075      +14     
===========================================
  Hits          7643     7643              
- Misses        3875     3889      +14     
  Partials       543      543              
Files Coverage Δ
app/ante/nativefee.go 55.69% <100.00%> (ø)
x/evm/keeper/gas.go 89.74% <100.00%> (ø)
x/evm/keeper/utils.go 77.41% <100.00%> (ø)
app/app.go 80.52% <6.66%> (-1.69%) ⬇️

Copy link
Collaborator

@mmsqe mmsqe left a comment

Choose a reason for hiding this comment

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

do we need update statedb as well?

@yihuang
Copy link
Collaborator Author

yihuang commented Apr 2, 2024

do we need update statedb as well?

there'll be another PR for that, one pr one issue.

@yihuang yihuang enabled auto-merge (squash) April 2, 2024 05:31
Solution:
- use virtual send

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

fix build

fix test

cleanup
@yihuang yihuang merged commit be00d15 into crypto-org-chain:develop Apr 2, 2024
37 of 39 checks passed
@yihuang yihuang deleted the fee-deduction branch April 2, 2024 06:11
@yihuang yihuang mentioned this pull request Apr 2, 2024
7 tasks
zsystm pushed a commit to b-harvest/ethermint that referenced this pull request Jun 3, 2024
… execution (crypto-org-chain#447)

Reason for Cherry-pick:
- Integrate Block STM (Parallel Transaction Execution)
- Avoid common conflict problem (fee collection happens for every tx)
- Add TODOs to track newly added code while cherry pick
- (To-do) handler_test.go and state_transition_test.go remains same and some tests are broken. let's fix out.

Notes
- Use b-harvest's cosmos-sdk which applies minimum requirements for integrating block-stm
- Use No-op app mempool currently, do it later
- Skip Flags (UnsafeSkipUpgrades, InvCheckPeriod, Home) which are already injected when creating EthermintApp
- Remain original code structures (maybe refactored or changed later referencing related commit)
- Authz module has begin blocker, feegrant module has end blocker. both were located under no-op modules comment.
-

Original Commit Message:

* Problem: fee deduction not compatible with parallel execution

Solution:
- use virtual send

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

* fix build

* fix test

* cleanup

---------

Signed-off-by: yihuang <[email protected]>
Cherry-picked-by: zsystm <[email protected]>
zsystm added a commit to b-harvest/ethermint that referenced this pull request Jun 5, 2024
Reason for Cherry-pick:
- refactor noisy codes by referencing cronos ethermint's well structured testing suite
- consider virtual balance for fee collector module because some tests does not trigger ante handler (e.g. direct handler call)

Note:
- this cherry pick came from crypto-org-chain#447
- why additional commit? some interfaces are not compatible because of geth version diff and sdk diff.

Cherry-picked-by: zsystm <[email protected]>
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