Split signature throughput tracking out of `FeeCalculator` #8447

t-nelson · 2020-02-25T20:09:25Z

Problem

FeeCalculator does too much. While initially intended as a utility to calculate transaction fees, it has grown to also track cluster signature throughput for calculating the next block's FeeCalculator. This requires significantly more stored state, making it prohibitive to store a FeeCalculator on chain as required to resolve #7967.

Summary of Changes

Factor out signature throughput tracking to new FeeRateGovernor type
Replace FeeCalculator with FeeRateGovernor as appropriate
Expose recent FeeRateGovernor via RPC

Toward #7967

sdk/src/client.rs

codecov · 2020-02-25T22:21:30Z

Codecov Report

Merging #8447 into master will decrease coverage by 0.1%.
The diff coverage is 75.1%.

@@           Coverage Diff            @@
##           master   #8447     +/-   ##
========================================
- Coverage    80.3%   80.1%   -0.2%     
========================================
  Files         256     256             
  Lines       56554   55279   -1275     
========================================
- Hits        45413   44297   -1116     
+ Misses      11141   10982    -159

sdk/src/fee_calculator.rs

t-nelson · 2020-02-27T22:01:10Z

@mvines how are we looking here?

CriesofCarrots

lgtm, fwiw

mvines

Please update the JSON RPC API docs too

client/src/rpc_request.rs

core/src/rpc.rs

sdk/src/fee_calculator.rs

Leaving `FeeCalculator` to *only* calculate transaction fees

Appease BPF tests

This reverts commit f303562.

…Calculator`

Pull request has been modified.

t-nelson · 2020-02-28T18:55:37Z

@mvines last 4 should resolve the outstandings.

While updating the docs I was reminded that we have a usize in the FeeRateGovernor. I went ahead and kicked it out in preference of a u64 for portability's sake; fe63a19. I did leave a clamp to std::u32::MAX in place in the code, though it looks unnecessary now.

solana/sdk/src/fee_calculator.rs

Lines 104 to 106 in fe63a19

    
           me.target_lamports_per_signature 
        
               * std::cmp::min(latest_signatures_per_slot, std::u32::MAX as u64) 
        
                   as u64

docs/src/apps/jsonrpc-api.md

mvines

lgtm, just that one last comment

… reply

Pull request has been modified.

* SDK: Split new `FeeRateGovernor` out of `FeeCalculator` Leaving `FeeCalculator` to *only* calculate transaction fees * Replace `FeeCalculator` with `FeeRateGovernor` as appropriate * Expose recent `FeeRateGovernor` to clients * Move `burn()` back into `FeeCalculator` Appease BPF tests * Revert "Move `burn()` back into `FeeCalculator`" This reverts commit f303562. * Adjust BPF `Fee` sysvar test to reflect removal of `burn()` from `FeeCalculator` * Make `FeeRateGovernor`'s `lamports_per_signature` private * rebase artifacts * fmt * Drop 'Recent' * Drop _with_commitment variant * Use a more portable integer for `target_signatures_per_slot` * Add docs for `getReeRateCalculator` JSON RPC method * Don't return `lamports_per_signature` in `getFeeRateGovernor` JSONRPC reply (cherry picked from commit 90bedd7)

…8610) automerge

…bs#8447) * SDK: Split new `FeeRateGovernor` out of `FeeCalculator` Leaving `FeeCalculator` to *only* calculate transaction fees * Replace `FeeCalculator` with `FeeRateGovernor` as appropriate * Expose recent `FeeRateGovernor` to clients * Move `burn()` back into `FeeCalculator` Appease BPF tests * Revert "Move `burn()` back into `FeeCalculator`" This reverts commit f303562. * Adjust BPF `Fee` sysvar test to reflect removal of `burn()` from `FeeCalculator` * Make `FeeRateGovernor`'s `lamports_per_signature` private * rebase artifacts * fmt * Drop 'Recent' * Drop _with_commitment variant * Use a more portable integer for `target_signatures_per_slot` * Add docs for `getReeRateCalculator` JSON RPC method * Don't return `lamports_per_signature` in `getFeeRateGovernor` JSONRPC reply

t-nelson requested review from mvines, CriesofCarrots and rob-solana February 25, 2020 20:09

t-nelson commented Feb 25, 2020

View reviewed changes

sdk/src/client.rs Outdated Show resolved Hide resolved

t-nelson force-pushed the fee_calc_amputation branch from 74a7539 to f303562 Compare February 25, 2020 21:37

mvines reviewed Feb 26, 2020

View reviewed changes

sdk/src/fee_calculator.rs Outdated Show resolved Hide resolved

mvines reviewed Feb 26, 2020

View reviewed changes

sdk/src/fee_calculator.rs Outdated Show resolved Hide resolved

t-nelson force-pushed the fee_calc_amputation branch 3 times, most recently from f6fb667 to 4c5f81a Compare February 27, 2020 16:49

CriesofCarrots previously approved these changes Feb 27, 2020

View reviewed changes

mvines reviewed Feb 28, 2020

View reviewed changes

client/src/rpc_request.rs Outdated Show resolved Hide resolved

core/src/rpc.rs Outdated Show resolved Hide resolved

sdk/src/fee_calculator.rs Show resolved Hide resolved

t-nelson added 13 commits February 28, 2020 11:06

SDK: Split new FeeRateGovernor out of FeeCalculator

1222454

Leaving `FeeCalculator` to *only* calculate transaction fees

Replace FeeCalculator with FeeRateGovernor as appropriate

a743f5d

Expose recent FeeRateGovernor to clients

a1f94a1

Move burn() back into FeeCalculator

480415a

Appease BPF tests

Revert "Move burn() back into FeeCalculator"

bb0bcda

This reverts commit f303562.

Adjust BPF Fee sysvar test to reflect removal of burn() from `Fee…

bc51870

…Calculator`

Make FeeRateGovernor's lamports_per_signature private

6b7e887

rebase artifacts

438a1eb

fmt

cbef36c

Drop 'Recent'

22bbc1d

Drop _with_commitment variant

514fc57

Use a more portable integer for target_signatures_per_slot

fe63a19

Add docs for getReeRateCalculator JSON RPC method

54c4254

t-nelson force-pushed the fee_calc_amputation branch from 4c5f81a to 54c4254 Compare February 28, 2020 18:28

mvines reviewed Feb 28, 2020

View reviewed changes

docs/src/apps/jsonrpc-api.md Outdated Show resolved Hide resolved

mvines previously approved these changes Feb 28, 2020

View reviewed changes

Don't return lamports_per_signature in getFeeRateGovernor JSONRPC…

8cf79e9

… reply

t-nelson merged commit 90bedd7 into solana-labs:master Feb 28, 2020

t-nelson deleted the fee_calc_amputation branch February 28, 2020 20:27

t-nelson added the v1.0 label Mar 4, 2020

solana-grimes pushed a commit that referenced this pull request Mar 4, 2020

Split signature throughput tracking out of FeeCalculator (#8447) (#…

7d86179

…8610) automerge

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split signature throughput tracking out of `FeeCalculator` #8447

Split signature throughput tracking out of `FeeCalculator` #8447

Uh oh!

t-nelson commented Feb 25, 2020

Uh oh!

Uh oh!

codecov bot commented Feb 25, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Feb 27, 2020

Uh oh!

CriesofCarrots left a comment

Uh oh!

mvines left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Feb 28, 2020

Uh oh!

Uh oh!

mvines left a comment

Uh oh!

Uh oh!

Split signature throughput tracking out of FeeCalculator #8447

Split signature throughput tracking out of FeeCalculator #8447

Uh oh!

Conversation

t-nelson commented Feb 25, 2020

Problem

Summary of Changes

Uh oh!

Uh oh!

codecov bot commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Feb 27, 2020

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-nelson commented Feb 28, 2020

Uh oh!

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Split signature throughput tracking out of `FeeCalculator` #8447

Split signature throughput tracking out of `FeeCalculator` #8447

codecov bot commented Feb 25, 2020 •

edited

Loading