Skip to content

refactor: reuse calculated fees#1462

Merged
mergify[bot] merged 5 commits intoanza-xyz:masterfrom
jstarry:refactor/pass-fee
May 29, 2024
Merged

refactor: reuse calculated fees#1462
mergify[bot] merged 5 commits intoanza-xyz:masterfrom
jstarry:refactor/pass-fee

Conversation

@jstarry
Copy link
Copy Markdown

@jstarry jstarry commented May 22, 2024

Problem

Transaction fees are recalculated quite a few times in the transaction pipeline for no good reason

Summary of Changes

Store calculated fee details in LoadedTransaction and TransactionExecutionDetails so that fees don't need to be recalculated downstream

Fixes #

@jstarry jstarry force-pushed the refactor/pass-fee branch from 8ca3036 to c6e0942 Compare May 22, 2024 23:21
@jstarry jstarry force-pushed the refactor/pass-fee branch from c6e0942 to ae5ca7b Compare May 24, 2024 04:17
@jstarry jstarry marked this pull request as ready for review May 24, 2024 04:22
@jstarry jstarry requested review from apfitzge and tao-stones May 24, 2024 04:22
@apfitzge
Copy link
Copy Markdown

Just looking at title it seems effort is similar to Tao's tx meta.
Might wanna coordinate with him or get his thoughts on this.

@jstarry jstarry force-pushed the refactor/pass-fee branch from ae5ca7b to 3ff403c Compare May 24, 2024 13:55
@tao-stones
Copy link
Copy Markdown

Cached FeeDetails should be invalidated between epoch boundary, should it? Inputs to calculate_fee_details() such as budget_limits are often feature gates dependent, so does calculation itself.

One version of solana_runtime_transaction::transaction_meta set ttl to cached data with get_last_slot_in_epoch(); While it helped to reuse cached value within epoch, the interface was rather messy: getter needs to take current_slot as additional parameter, and returns Option<>. In case of None returned (crossed epoch), callsite needs to find all necessary inputs to refresh the cached value. Is there better way?

@jstarry
Copy link
Copy Markdown
Author

jstarry commented May 24, 2024

Epoch boundaries aren't relevant to this PR refactoring because the fee that this PR is reusing is calculated during transaction loading / execution in the SVM. Epoch boundaries are only relevant when scheduling transactions in banking stage. This PR doesn't change any behavior in the banking stage and so I don't think this will conflict at all with tx meta work that @tao-stones is working on

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.7%. Comparing base (a4a009e) to head (abe1f46).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1462   +/-   ##
=======================================
  Coverage    82.7%    82.7%           
=======================================
  Files         876      876           
  Lines      371118   371045   -73     
=======================================
- Hits       307263   307212   -51     
+ Misses      63855    63833   -22     

@jstarry jstarry force-pushed the refactor/pass-fee branch from 3ff403c to abe1f46 Compare May 26, 2024 17:12
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

Agree caching FeeDetails at LoadedTransaction makes a lot sense in this PR - bank is going to collect exact fee that account_loader calculated and validated against payer account. My early commit is due to the concern that dev could accidentally cache and reuse LoadedTransaction (at banking stage) cross epoch boundary, even it's not intentional. Perhaps that's overly concerned? Other than that, the PR looks great.

Comment thread svm/src/account_loader.rs Outdated
Comment thread sdk/src/fee.rs
@jstarry
Copy link
Copy Markdown
Author

jstarry commented May 29, 2024

My early comment is due to the concern that dev could accidentally cache and reuse LoadedTransaction (at banking stage) cross epoch boundary, even it's not intentional. Perhaps that's overly concerned?

Yeah, I don't think it's a valid concern because basically nothing in LoadedTransaction should be loaded during one bank and then used in a new bank, let alone across an epoch boundary.

tao-stones
tao-stones previously approved these changes May 29, 2024
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm - thanks for clarification.

@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify mergify Bot removed the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify
Copy link
Copy Markdown

mergify Bot commented May 29, 2024

automerge label removed due to a CI failure

@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify mergify Bot merged commit 2cc540a into anza-xyz:master May 29, 2024
@jstarry jstarry deleted the refactor/pass-fee branch May 29, 2024 20:30
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
…nza-xyz#298, anza-xyz#356, anza-xyz#357)

Original version written by Andrew Fitzgerald <apfitzge@gmail.com>.

Current version rewritten to match the upstream scheduler code as of

  commit ef4f90f
  Author: Faycel Kouteib <faycel.kouteib@anza.xyz>
  Date:   Wed Mar 26 16:29:14 2025 -0700

      Run 'fetch-core-bpf.sh' from any directory (anza-xyz#5518)

This also brings the invalidator logic closer to the upstream scheduler,
as we are now changing the upstream Committer, rather than running our
own copy.  There is still a lot of code that was copied and modified.
Maybe, eventually, we are going to reduce the copy/paste even more.

Previous version:

  commit 8cd231e9c2435119f80d77ffcab164e3dc7e4cd2
  Author: Andrew Fitzgerald <apfitzge@gmail.com>
  Date:   Tue Sep 12 14:20:08 2023 -0700

      Failed tx fastpath support (anza-xyz#133, anza-xyz#229, anza-xyz#356, anza-xyz#357)

      Includes:

      - replay: atomicbool instead of singleton for dropping packets (anza-xyz#224)

          commit db8607411b7016eb9a1f09dae07d9a9c375cac10
          Author: kirill lykov <kirill.lykov@solana.com>
          Date:   Thu Feb 8 10:52:48 2024 +0100

              replay: atomicbool instead of singleton for dropping packets (anza-xyz#224)

              * use atomicbool instead of singleton to drop packets

              * add use for Ordering

              Co-authored-by: Illia Bobyr <ilya.bobyr@gmail.com>
              Signed-off-by: kirill lykov <lykov.kirill@gmail.com>

              * rename drop_packets

              ---------

              Signed-off-by: kirill lykov <lykov.kirill@gmail.com>
              Co-authored-by: Illia Bobyr <ilya.bobyr@gmail.com>

      - check_transactions in record_and_commit_batch (anza-xyz#229)

      - An update to the fee calculation due to

          commit 2cc540a
          Author: Justin Starry <justin@anza.xyz>
          Date:   Wed May 29 15:41:47 2024 -0400

              refactor: reuse calculated fees (anza-xyz#1462)

      - remove dead code (anza-xyz#298)

          commit c791d1c62f02b436d291744c88f122c51d80b4a8
          Author: Brennan <brennan.watt@anza.xyz>
          Date:   Fri Mar 22 06:45:29 2024 -0700

              remove dead code (anza-xyz#298)

      - populate fee details (anza-xyz#357)

          commit 701e2cad0e9afc3911e1cbc06c849eda7b1ee5aa
          Author: Brennan <brennan.watt@anza.xyz>
          Date:   Thu Jul 4 06:35:01 2024 -0700

              populate fee details (anza-xyz#357)

      - adversary::failed_transaction_hotpath: Count only required singatures (anza-xyz#356)

          commit 7482b7976e968a645b6c9500d8c753b65b2acec5
          Author: Illia Bobyr <illia.bobyr@anza.xyz>
          Date:   Mon Jul 8 17:40:26 2024 -0700

              adversary::failed_transaction_hotpath: Count only required singatures (anza-xyz#356)

      - adversary::failed_transaction_hotpath: Use solana_fee

        commit 0e9c9ab92909b677c8be6a6453af1dea96f81cc5
        Author: Illia Bobyr <illia.bobyr@anza.xyz>
        Date:   Tue Jul 30 03:04:27 2024 -0700

            adversary::failed_transaction_hotpath: Use solana_fee

            Matching changes in

                commit 5604a4d
                Author: Andrew Fitzgerald <apfitzge@gmail.com>
                Date:   Fri Jul 26 10:08:47 2024 -0400

                    Separate fee-calculation from SDK (anza-xyz#2120)

            anza-xyz#2120

    Refactored-by: Illia Bobyr <illia.bobyr@anza.xyz>
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.

4 participants