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

Add Weight and FeeRate newtypes #1627

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Add Weight and FeeRate newtypes #1627

merged 1 commit into from
Feb 10, 2023

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Feb 6, 2023 •
edited by apoelstra
Loading

Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion.

Part of #630
Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.)

DanGould
DanGould reviewed Feb 6, 2023
View reviewed changes
bitcoin/src/blockdata/fee_rate.rs
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
#[cfg_attr(feature = "serde", serde(transparent))]
pub struct FeeRate(u64);
Copy link
Contributor

@DanGould DanGould Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Integer (aka "round") FeeRate makes me wary. Any time you combine two PSBTs the resulting feerate is likely a non-integer rational.

It's a privacy concern from the payjoin spec:

If the sender's fee rate is always round, then a blockchain analyst can easily spot the transactions of the sender involving payjoin by checking if, when removing a single input to the suspected payjoin transaction, the resulting fee rate is round. To prevent this, the sender can agree to pay more fee so the receiver make sure that the payjoin transaction fee is also round.

Could representing it as FeeRate(amount_sats: u64, Weight) and comparison of f64s fix this problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's in sat/kwu, same as Core uses and I don't think any wallet uses anything more precise. Even floats have limits on precision, so there's no way to solve it perfectly.

I think for PayJoin we should check if the fee rate exactly matches sat/vb and if yes, match it, if not we should check sat/kwu. The protocol already fixes it so we don't have to worry about it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize it was sat/kwu. Makes sense to me. Don't reinvent the wheel.

@tcharding
Copy link
Member

Needs #1610

@tcharding
Copy link
Member

Some other methods that we may want to consider:
For both Weight and FeeRate

  • to/from_le_bytes
  • Maybe shift left/right?
  • ops by various integer types (e.g, Mul<u16>)

For Weight

  • checked/wrapping/overflowing add/mul/sub

I just looked over the U256 code to see what we might want.

@Kixunil Kixunil force-pushed the size-fee-rate branch 2 times, most recently from b8eab19 to 14bb9ec Compare February 7, 2023 12:46
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 7, 2023

Was going to add to_be_bytes&co but decided not to because the unit is not obvious. In both cases there can be at least two units. Explicit construction is better. A similar argument could be made about serde but I guess that would be too annoying.

Shifts definitely not, this is not a bag of bits.
Multiplication by various integers seems to be a bit odd. No other Rust type has it.

I added checked_ operations since those make sense. Others shouldn't be needed when computing weight.

@Kixunil Kixunil force-pushed the size-fee-rate branch 2 times, most recently from 28d488f to f032905 Compare February 7, 2023 13:14
@apoelstra
Copy link
Member

I'm a bit confused about

  • the meaning of multiplying or dividing FeeRate by a number. I guess it's to "scale a feerate" but why would one do this? Note that the resolution of integers here really become a problem ... the minimum action you can do here is double the feerate. It may make sense to instead have a dedicated bump_by method or something which takes a floating point and adds 1.0, say.
  • adding Weight to itself. As I said in Add new Weight type #1607 I'm suspicious of this operation and think that it's probably incorrect, and it would be better that users need to do some more gymnastics to compose weights.
  • multiplying Weight by numbers doesn't seem meaningful except in pretty niche cases (e.g. adding several identical-looking inputs to a transaction)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 7, 2023

  • Yeah, scaling. It is true that it is unlikely, I added it as a "standard impl". Could remove it if you don't like it.
  • Adding Weight is addressed by transaction parts not returning weight. As I said, I want to write a proper predictor.

adding several identical-looking inputs to a transaction

I happen to have exactly this use case. :)

@apoelstra
Copy link
Member

Yeah, scaling. It is true that it is unlikely, I added it as a "standard impl". Could remove it if you don't like it.

Yeah, I'd prefer we remove the Mul impls. I have no problem supporting scaling, but I think it should be a dedicated method and probably should take a floating-point value.

I rescind my other comments about Weight. That all seems reasonable :).

@Kixunil
Add Weight and FeeRate newtypes
70cf451 
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 8, 2023

I have a WIP fee prediction function which is very important for me to get in. Unfortunately, today went to shit and I'm super tired now. I will try to finish it tomorrow morning if that's OK. If not, I can live with it.

@tcharding
Copy link
Member

Go for it, no stress, its only a "soft" feature freeze :)

@Kixunil Kixunil mentioned this pull request Feb 9, 2023
Merged
apoelstra
apoelstra approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 70cf451

tcharding
tcharding approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 70cf451

@tcharding
Copy link
Member

Nice PR, I learned a few things (you'll see my new issues and comments :)

@tcharding
Copy link
Member

If we want a link to docs in the Weight file we could use: https://en.bitcoin.it/wiki/Weight_units

@apoelstra apoelstra merged commit b6387db into rust-bitcoin:master Feb 10, 2023
@DanGould DanGould mentioned this pull request Feb 10, 2023
Closed
4 tasks
@Kixunil Kixunil deleted the size-fee-rate branch February 10, 2023 08:27
apoelstra added a commit that referenced this pull request Feb 13, 2023
@apoelstra
Merge #1636: Weight prediction
f00b636 
ae2aaaa Add `script_pubkey_lens` method (Martin Habovstiak)
cf068d1 Implement transaction weight prediction (Martin Habovstiak)

Pull request description:

  When creating a transaction one must know the the fee beforehand to set
  appropriate amounts for outputs and to know the fee, weight is required.
  So far we only had a method on an already-constructed transaction. This
  method clearly wasn't helpful when constructing the transaction except
  for hacks like temporarily adding an all-zeroes signature.

  This change adds a function that can compute the transaction weight
  without knowing individual bytes of the scripts, witnesses and other
  elements. It only needs to know their sizes.

  To make the API less error-prone a special, trivial, type is also added
  for computing the lengths of witnesses.

  Based on #1627

ACKs for top commit:
  apoelstra:
    ACK ae2aaaa
  tcharding:
    ACK ae2aaaa

Tree-SHA512: 55376601c2c2826bb0909cc25ff5b65816f0b1a2d57fb2cd8831f3db5382de0f4a364d518b312f0528bb5f44c30f3f74f8d254145eed2bfd65e2332b7c4d7c8b
@yancyribbens yancyribbens mentioned this pull request Feb 24, 2023
Closed
@yancyribbens yancyribbens mentioned this pull request Mar 2, 2023
Merged
apoelstra added a commit that referenced this pull request Mar 4, 2023
@apoelstra
Merge #1685: Improve the public API for Feerate and Weight
1679ad8 
b0b0cdb Improve the public API for Feerate and Weight (yancy)

Pull request description:

  Small nit for #1627 to re-export `Weight` and `FeeRate` to shorten the use path.

  ```
  use bitcoin::Weight;
  use bitcoin::FeeRate;
  ````

ACKs for top commit:
  tcharding:
    ACK b0b0cdb
  Kixunil:
    ACK b0b0cdb
  apoelstra:
    ACK b0b0cdb

Tree-SHA512: 81e508c980c4f37e3791d26616459608dd60e5a6255ef28b3a049c1d27281b2853b92474d42f10031254c8c46878adf666eb709826aa4ffde1b4b3542451e080
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanGould DanGould DanGould left review comments

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
enhancement release notes mention
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kixunil @tcharding @apoelstra @DanGould