Skip to content

plan: add some clarifying docs #824

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

Merged
merged 1 commit into from
Jun 5, 2025
Merged

Conversation

benma
Copy link
Contributor

@benma benma commented Jun 4, 2025

"The size in bytes of the script sig that" is ambiguous and could refer to the actual scriptSig, exlcuding the varint prefix that indicates its size.

The code comment in witness_size is promoted to the docstring, as its helpful to anyone trying to compute the right size of a transaction.

"The size in bytes of the script sig that" is ambiguous and could
refer to the actual scriptSig, exlcuding the varint prefix that
indicates its size.

The code comment in witness_size is promoted to the docstring, as its
helpful to anyone trying to compute the right size of a transaction.
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 e89e226; successfully ran local tests; nice, good call

@apoelstra apoelstra merged commit f023c39 into rust-bitcoin:master Jun 5, 2025
31 checks passed
@benma benma deleted the size-doc branch June 5, 2025 19:50
@benma
Copy link
Contributor Author

benma commented Jun 5, 2025

@apoelstra speaking of, I just noticed that plan.satisfy() returns the scriptSig as a ScriptBuf, which does not include the var-int prefix, so it's a inconsistent that the size of the var-int prefix is included in plan.sigscript_size().

Furthermore, it's not possible to know the size of the scriptsig without the var-int prefix during planning.

I think it would make sense to offer a function to return the size excluding the var-int prefix. WDYT?

@apoelstra
Copy link
Member

Oh, yeah, I like this. Definitely agreed that plan.satisfy().len() ought to return the same value as plan.???_size().

I am torn between deprecating the version that has the length prefix, or not. (We definitely should not just silently change the behavior of scriptsig_size()! We need a new name. Maybe satisfy_len?)

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