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

txsize now just counts the serialized bytes #1639

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

JaredCorduan
Copy link
Contributor

Our txsize function was written to make coin selection easier, but it turns out not to help due to witnesses changing during coin selection. So now it is both complex and not helpful. This PR simplifies it to just the actual size in bytes.

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM!

We'll also need to update the spec addendum.

@JaredCorduan JaredCorduan force-pushed the jc/txsize-just-serialized-bytes branch from e0cbc35 to 3a2f9c1 Compare July 10, 2020 21:45
@JaredCorduan
Copy link
Contributor Author

I figured out what happened to the trace generation. We actually were making uses of the properties of our txsize function in the coin selection algorithm used by the transaction generators. We would check that our fee was large enough, and then swap out txouts.

Since the old txsize was actually helpful to us, I've renamed it as txsizeBound (and similarly minfeeBound) to be used by our generators. And perhaps others will find them helpful as well.

@JaredCorduan JaredCorduan force-pushed the jc/txsize-just-serialized-bytes branch from 3a2f9c1 to ba46040 Compare July 10, 2020 22:01
@JaredCorduan JaredCorduan force-pushed the jc/txsize-just-serialized-bytes branch from ba46040 to 2724f7c Compare July 10, 2020 22:48
@JaredCorduan JaredCorduan merged commit 25360f7 into master Jul 12, 2020
@iohk-bors iohk-bors bot deleted the jc/txsize-just-serialized-bytes branch July 12, 2020 02:29
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.

4 participants