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

Add functions to collect executed transactions fee in details; #178

Merged
merged 7 commits into from
Mar 21, 2024

Conversation

tao-stones
Copy link

Problem

Working bank should be able to collect executed transactions's fee in detail, separating transaction_fee (half burnt) and prio fee (100% rewarded).

Summary of Changes

  • Add CollectorFeeDetails to bank, which is NOT (de)serialized
  • Add dead_code filter_program_errors_and_collect_fee_details() implements collecting fee_details. This function is not called yet, next PR will invoke it behind a feature gate.

Fixes #

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Could you explain the motivation for why the bank should be aware of the details of collection at all?
It seems all we need from the bank variable is to accumulate reward into a u64 so it can be distributed at the end of slot. In that case, why not simply use a (atomic)u64 for the accumulation?

runtime/src/bank.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
@tao-stones
Copy link
Author

Could you explain the motivation for why the bank should be aware of the details of collection at all? It seems all we need from the bank variable is to accumulate reward into a u64 so it can be distributed at the end of slot. In that case, why not simply use a (atomic)u64 for the accumulation?

For SIMD-0096, how fee is distributed at the end of bank is different based on fee "type". Transaction Fee will be 50% rewarded 50% burnt, and Priority Fee will be 100% rewarded. For that, bank has to keep collected fees in different buckets. struct CollectorFeeDetails for this purpose. In the future, in case there is another type of fee that requires different distribution, say 100% burn, it can be simply added to CollectorFeeDetails.

Fee distribution changes are in next PR (preview).

@apfitzge
Copy link

apfitzge commented Mar 11, 2024

Could you explain the motivation for why the bank should be aware of the details of collection at all? It seems all we need from the bank variable is to accumulate reward into a u64 so it can be distributed at the end of slot. In that case, why not simply use a (atomic)u64 for the accumulation?

For SIMD-0096, how fee is distributed at the end of bank is different based on fee "type". Transaction Fee will be 50% rewarded 50% burnt, and Priority Fee will be 100% rewarded. For that, bank has to keep collected fees in different buckets. struct CollectorFeeDetails for this purpose. In the future, in case there is another type of fee that requires different distribution, say 100% burn, it can be simply added to CollectorFeeDetails.

Fee distribution changes are in next PR (preview).

I understand we have to distribute them differently based on type. But that can be done at transaction level, the bank only needs to be aware of the reward at the end of the slot.

Basically my question is why does the bank need to be aware of the details and not just the result, i.e. reward.

edit: I see now - it's because we do not update total capitalization changes until end of slot. Change to capitalization will depend on the burn rate, so we need the details unless we moved capitalization update by fee into transaction committing.

@t-nelson
Copy link

planning on adding tests?

@tao-stones
Copy link
Author

planning on adding tests?

Yes 💪🏼, one last thing to address from Andrew's last review.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

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

Project coverage is 81.9%. Comparing base (ade9035) to head (f59c93d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #178    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         838      838            
  Lines      226927   227067   +140     
========================================
+ Hits       185918   186049   +131     
- Misses      41009    41018     +9     

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Think we need to discuss the rounding; it seems to be a blocker for this style of accumulation.

I previously expressed my thought on accumulating only reward instead of fee, which I pointed out will not work due to the capitalization update.

Alternatively we could still accumulate 2 variables: reward, burn.
This would let us update capitalization correctly at the end of the slot using burn.
I think that would also let us move forward with this change without blocking on the rounding feature.
wdyt?

runtime/src/bank.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
apfitzge
apfitzge previously approved these changes Mar 18, 2024
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@tao-stones
Copy link
Author

@lheeger-jump Do you have time to check this one out? I'd like to merge this so can put next one out, which distributes collected collector_fee_details by fee type (50/50 for transaction fee, 100% for prio fee).

Copy link

@lheeger-jump lheeger-jump left a comment

Choose a reason for hiding this comment

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

LGTM

@tao-stones tao-stones added the automerge automerge Merge this Pull Request automatically once CI passes label Mar 20, 2024
@mergify mergify bot merged commit 1e08e90 into master Mar 21, 2024
48 checks passed
@mergify mergify bot deleted the simd0096-collect-fee-details branch March 21, 2024 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants