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.

Sign up for GitHub

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

Jump to bottom

get_fee_rate_statistics should skip first (cellbase) Transaction #4647

Merged
merged 2 commits into from
Sep 14, 2024
Merged

get_fee_rate_statistics should skip first (cellbase) Transaction #4647

merged 2 commits into from
Sep 14, 2024

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Sep 13, 2024 •
edited
Loading

What problem does this PR solve?

Want to fix #4646 and #4350

What is changed and how it works?

Related changes

  • get_fee_rate_statistics skip first cellbase in BlockExt

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note 

None: Exclude this PR from the release note.

@eval-exec eval-exec requested a review from a team as a code owner September 13, 2024 09:11
@eval-exec eval-exec requested review from doitian and removed request for a team September 13, 2024 09:11
@eval-exec eval-exec marked this pull request as draft September 13, 2024 09:12
@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch 3 times, most recently from 193c323 to 80b3f80 Compare September 13, 2024 09:22
@eval-exec eval-exec marked this pull request as ready for review September 13, 2024 09:22
@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch from 80b3f80 to 2540df0 Compare September 13, 2024 09:22
@eval-exec eval-exec changed the title get_fee_rate_statistics skip first tx get_fee_rate_statistics should skip first (cellbase) Transaction Sep 13, 2024
@phroi
Copy link

phroi commented Sep 13, 2024

Good catch 👍

quake
quake reviewed Sep 13, 2024
View reviewed changes
rpc/src/util/fee_rate.rs
) {
)
// skip cellbase (first element in the Vec)
.skip(1)
Copy link
Member

Choose a reason for hiding this comment

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

may better to change line 86 also:

-            if !block_ext.txs_fees.is_empty()
+            if block_ext.txs_fees.len() > 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, updated.

@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch from 2540df0 to 1f213eb Compare September 13, 2024 12:47
@eval-exec
FeeRateCollector::statistics should skip first cellbase value
1a15a68 
Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch from 1f213eb to 1a15a68 Compare September 13, 2024 12:48
quake
quake previously approved these changes Sep 13, 2024
View reviewed changes
@quake quake added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 13, 2024
@eval-exec eval-exec marked this pull request as draft September 14, 2024 01:57
zhangsoledad
zhangsoledad previously approved these changes Sep 14, 2024
View reviewed changes
@eval-exec eval-exec dismissed stale reviews from zhangsoledad and quake via 5bf859f September 14, 2024 04:45
@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch from 5bf859f to 216fd7e Compare September 14, 2024 04:46
@eval-exec
Fix ckb-rpc test_fee_rate_statics unit test, first element in txs_fee…
92680dc 
…s is belong to cellbase

Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the exec/do-not-contains-cell-base-in-get_fee_rate_statistics branch from 216fd7e to 92680dc Compare September 14, 2024 04:47
@eval-exec eval-exec marked this pull request as ready for review September 14, 2024 04:58
@eval-exec eval-exec added b:rpc Break RPC interface s:backport-needed labels Sep 14, 2024
@eval-exec
Copy link
Collaborator Author

eval-exec commented Sep 14, 2024 •
edited
Loading

Fixed the unit test, should put cellbase (cycle, tx_fee, tx_size) as first element in BlockExt's fields, waiting for CI.

@quake quake added this pull request to the merge queue Sep 14, 2024
Merged via the queue into nervosnetwork:develop with commit e0b4489 Sep 14, 2024
32 checks passed
@eval-exec eval-exec mentioned this pull request Sep 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quake quake quake approved these changes

@doitian doitian Awaiting requested review from doitian doitian is a code owner automatically assigned from nervosnetwork/ckb-dev

@driftluo driftluo Awaiting requested review from driftluo

@yangby-cryptape yangby-cryptape Awaiting requested review from yangby-cryptape

@chenyukang chenyukang Awaiting requested review from chenyukang

@zhangsoledad zhangsoledad Awaiting requested review from zhangsoledad

Assignees
No one assigned
Labels
b:rpc Break RPC interface s:backport-needed
Projects
CKB Kanban
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] fee_rate_statistics returns a feeRate median lower than 1000
4 participants
@eval-exec @phroi @quake @zhangsoledad