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

txsizes: count change output in estimating tx size #797

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

journeyer
Copy link

In the estimation of transaction size, there is an uncommon scenario in which the required space for serializing the count of outputs is calculated wrong since the change output is not counted in the count of outputs.

The source of the problem is just an unused variable which this PR fixes it.

in estimating transaction size, the number of transaction outputs
sometimes was calculated one less than actual since change output
was not counted; this commit makes sure that it's counted when it
exists.
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Great fix!

Only final ask here is a test case that reproduces the size divergence and shows that this PR fixes things. It's clear from the diff that if we added a change output that it wouldn't be accounted for, but ideally we can have that assertion locked in at the test level.

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.

2 participants