Skip to content

Add txin base weight#2001

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
yancyribbens:add-txin-base-weight
Aug 18, 2023
Merged

Add txin base weight#2001
apoelstra merged 2 commits intorust-bitcoin:masterfrom
yancyribbens:add-txin-base-weight

Conversation

@yancyribbens
Copy link
Copy Markdown
Contributor

@yancyribbens yancyribbens commented Aug 17, 2023

Add a base weight const to TxIn. I also used this const in strippedsize() and scaledsize(). As a different PR, I think strippedsize and scaledsize could return Weight instead of usize. Also added a small commit to re-arrange commit messages.

tcharding
tcharding reviewed Aug 17, 2023
View reviewed changes
Comment thread bitcoin/src/blockdata/transaction.rs Outdated
Comment thread bitcoin/src/blockdata/transaction.rs Outdated
Comment on lines 615 to 619
Copy link
Copy Markdown
Member

@tcharding tcharding Aug 17, 2023

Choose a reason for hiding this comment

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

No need for this change IMO, I assume it was because of the code comment on BASE_WEIGHT.

Copy link
Copy Markdown
Contributor Author

@yancyribbens yancyribbens Aug 17, 2023

Choose a reason for hiding this comment

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

I think you mentioned in a previous PR that developer comments go above doc comments?

Copy link
Copy Markdown
Member

@tcharding tcharding Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't remember saying so, I don't have a policy on docs/rustdocs in my mind. You could be mixing up with rustdocs above attributes, which I remember commenting on recently.

Comment thread bitcoin/src/blockdata/transaction.rs Outdated
Comment on lines 716 to 727
Copy link
Copy Markdown
Member

@tcharding tcharding Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The comment could be removed now.

Copy link
Copy Markdown
Contributor Author

@yancyribbens yancyribbens Aug 17, 2023

Choose a reason for hiding this comment

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

done.

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Aug 17, 2023

Thanks for the review @tcharding

apoelstra
apoelstra approved these changes Aug 17, 2023
View reviewed changes
Copy link
Copy Markdown
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 7ec33d2

tcharding
tcharding approved these changes Aug 17, 2023
View reviewed changes
Copy link
Copy Markdown
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.

The first commit is good, the second us unnecessary but harmless, so I'm fine to leave it in.

ACK 7ec33d2

@apoelstra apoelstra merged commit 6726565 into rust-bitcoin:master Aug 18, 2023
@stevenroose
Copy link
Copy Markdown
Collaborator

stevenroose commented Sep 7, 2023

This MR seems incorrect to me.. The inputs is 32 + 4 + 4 bytes in actual size, not weight. So it is missing a multiplier of 4.. The place it's used in it's supposed to be size to, but the Weight is just converted back to wu and interpreted as size..

@stevenroose
Copy link
Copy Markdown
Collaborator

stevenroose commented Sep 7, 2023

Looking into the broader concept this is used in, confuses me a lot. The use of the type Weight is the root of the confusion I think. For me, and for many people I think based on BIPs, a weight unit is used for a witness byte or a quarter of a non-witness byte. So if you could non-witness bytes as weight units "before you apply whatever multiplier", I wouldn't call that "weight".

Especially in the case of this constant, the "base weight" of in input is definitely 4 times their base size. Because these fields are never counted as single weight units.

@stevenroose stevenroose mentioned this pull request Sep 7, 2023
Merged
@tcharding tcharding mentioned this pull request Sep 7, 2023
Closed
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Sep 8, 2023 •
edited
Loading

oof @stevenroose you're right.

I think the "root of the confusion" is that I looked at Weight::from_wu(32 + 4 + 4) and did not notice that this was incorrect. I think the types/units are pretty clear, I'm just dumb.

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Sep 8, 2023

There's a PR which attempts to correct my mistake here: #2053

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Sep 8, 2023

I think the types/units are pretty clear, I'm just dumb.

Feeling pretty dumb as well for missing this.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Sep 8, 2023

Accidents happen, at least we caught it. Someone buy @stevenroose a cold drink.

@yancyribbens yancyribbens mentioned this pull request Sep 9, 2023
Closed
@yancyribbens yancyribbens mentioned this pull request Sep 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yancyribbens @stevenroose @apoelstra @tcharding