Skip to content

Make CFeeRate work with uint64_t sizes #23633

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
Closed
wants to merge 1 commit into from
Closed

Make CFeeRate work with uint64_t sizes #23633

wants to merge 1 commit into from

Conversation

kiminuo
Copy link
Contributor

@kiminuo kiminuo commented Nov 30, 2021 •
edited
Loading

txmempool.h contains functions:

  • uint64_t GetSizeWithDescendants() const
  • uint64_t GetSizeWithAncestors() const

A return value (i.e. uint64_t) of these functions can be used to instantiate CFeeRate. In the current master branch, this would require a cast to uint32_t because CFeeRate has the following constructor:

bitcoin/src/policy/feerate.cpp

Line 12 in 29074ba

CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)

This PR attempts to remove the need for the casting.

The PR's commit is cherry-picked from sipa's https://github.com/sipa/bitcoin/commits/202111_mempoolfr branch (see #21422 (comment)) which improves #21422.

The PR is currently a draft as I get:

policy/feerate.cpp: In constructor ‘CFeeRate::CFeeRate(const CAmount&, uint64_t)’:
policy/feerate.cpp:14:25: warning: narrowing conversion of ‘num_bytes’ from ‘uint64_t’ {aka ‘long unsigned int’} to ‘int64_t’ {aka ‘long int’} [-Wnarrowing]
   14 |     const int64_t nSize{num_bytes};
      |                         ^~~~~~~~~
policy/feerate.cpp: In member function ‘CAmount CFeeRate::GetFee(uint64_t) const’:
policy/feerate.cpp:25:25: warning: narrowing conversion of ‘num_bytes’ from ‘uint64_t’ {aka ‘long unsigned int’} to ‘int64_t’ {aka ‘long int’} [-Wnarrowing]
   25 |     const int64_t nSize{num_bytes};

during compilation.

Anyway, I would like to get some early feedback if the PR makes sense to you or not.

@sipa
Copy link
Member

sipa commented Nov 30, 2021 •
edited
Loading

Please don't @ mention people in PR descriptions. They'll get github notifications every time j-random-altcoin cherry-picks them.

@maflcko
Copy link
Member

maflcko commented Nov 30, 2021

Concept ACK, but please link to the commit that you are reverting

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 30, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24305 (Docs: [policy] Remove outdated confusing comment by Xekyo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@kiminuo
Copy link
Contributor Author

kiminuo commented Nov 30, 2021

Please don't @ mention people in PR descriptions. They'll get github notifications every time j-random-altcoin cherry-picks them.

Fixed. Sorry for that.

luke-jr
luke-jr reviewed Nov 30, 2021
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. A block can only be at the very most 4 MB today, which uint32_t is more than sufficient for. Does the "fee rate" across ancestors/descendants actually matter for sizes larger than this? It doesn't make sense for miners to take fees they won't get into consideration...

If anything, perhaps we should revisit where we get the combined fee rates, and make sure we ignore anything that wouldn't fit in a block?

@maflcko
Copy link
Member

maflcko commented Dec 1, 2021

References:

@kiminuo
Copy link
Contributor Author

kiminuo commented Dec 1, 2021

Does the "fee rate" across ancestors/descendants actually matter for sizes larger than this?

Maybe @sdaftuar can explain why uint64_t GetSizeWithDescendants() const returns uint64_t given that he added it 🙏. That would be helpful to decide whether to work on this PR more or just close it.

@luke-jr
Copy link
Member

luke-jr commented Dec 1, 2021

Perhaps the GetSizeWith* functions need to limit their scope to at most a single block of transactions. But then we also need to calculate the combined fee with that same scope. Ugh.

@maflcko maflcko mentioned this pull request Jan 3, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Feb 11, 2022
Merged
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@glozow
Copy link
Member

glozow commented Oct 12, 2022

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

@glozow glozow closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@luke-jr luke-jr luke-jr left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase TX fees and policy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kiminuo @sipa @maflcko @DrahtBot @luke-jr @glozow