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

fix(rpc): return the cycles of the first non-cellbase transaction as cellbase's cycles #3763

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

yangby-cryptape
Copy link
Collaborator

@yangby-cryptape yangby-cryptape commented Dec 20, 2022

What problem does this PR solve?

Problem Summary:

When call the RPC method get_transaction:

  • If a block has no non-cellbase transactions, the cycles of its cellbase are null.

    e.g.:

  • If a block has at least one non-cellbase transactions, the cycles of its cellbase are the same as the first non-cellbase transaction.

    Since 0.saturating_sub(1) == 1.saturating_sub(1) == 0 in the follow code:

    ckb/rpc/src/module/chain.rs

    Lines 2034 to 2036 in adcfac4

    block_ext
    .cycles
    .and_then(|v| v.get(tx_info.index.saturating_sub(1)).copied())

    e.g.:

If I understood correctly, the cycles for cellbases should always be null.

Check List

Tests

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

Release note

Title Only: Include only the PR title in the release note.

@yangby-cryptape yangby-cryptape marked this pull request as ready for review December 20, 2022 12:36
@yangby-cryptape yangby-cryptape requested a review from a team as a code owner December 20, 2022 12:36
@yangby-cryptape yangby-cryptape requested review from zhangsoledad and removed request for a team December 20, 2022 12:36
Copy link
Collaborator

@eval-exec eval-exec left a comment

Choose a reason for hiding this comment

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

Do fn get_transaction_verbosity2(below of fn get_transaction_verbosity1) need this check?

@yangby-cryptape
Copy link
Collaborator Author

Do fn get_transaction_verbosity2(below of fn get_transaction_verbosity1) need this check?

Yes, I missed. I will update this PR.

@zhangsoledad
Copy link
Member

bors r+

@bors bors bot merged commit 21d9ddd into nervosnetwork:develop Dec 21, 2022
@yangby-cryptape yangby-cryptape deleted the pr/fix-cellbase-cycles branch December 22, 2022 03:54
bors bot added a commit that referenced this pull request Dec 22, 2022
3770: [backport] fix(rpc): return the cycles of the first non-cellbase transaction as cellbase's cycles r=quake a=yangby-cryptape

### What problem does this PR solve?

Backport #3763.

### Check List

Tests

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

### Release note

```release-note
Title Only: Include only the PR title in the release note.
```

Co-authored-by: Boyu Yang <[email protected]>
@doitian doitian mentioned this pull request Jan 12, 2023
4 tasks
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.

3 participants