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

Some additional inspectors on Script and Witness #2646

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stevenroose
Copy link
Collaborator

Bundled these because they are very similar. Got a bunch of larger changes coming up based on these. I've been using these for a while for TXHASH work.

@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Apr 2, 2024
@coveralls
Copy link

coveralls commented Apr 2, 2024

Pull Request Test Coverage Report for Build 8522463247

Details

  • 90 of 103 (87.38%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.003%) to 83.133%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/blockdata/witness.rs 90 94 95.74%
bitcoin/src/blockdata/script/borrowed.rs 0 9 0.0%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/blockdata/witness.rs 1 89.39%
Totals Coverage Status
Change from base Build 8515631286: -0.003%
Covered Lines: 19158
Relevant Lines: 23045

💛 - Coveralls

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 ac4db63 but will need to wait for next release. I think we should merge these as-is although they will be much clearer after we do script tagging.

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 ac4db63

Comment on lines +412 to +418
/// Get the taproot control block following BIP341 rules.
///
/// This does not guarantee that this represents a P2TR [`Witness`]. It
/// merely gets the last or second to last element depending on the first
/// byte of the last element being equal to 0x50. See
/// [Script::is_p2tr](crate::blockdata::script::Script::is_p2tr) to
/// check whether this is actually a Taproot witness.
Copy link
Member

Choose a reason for hiding this comment

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

In case you want it, a bunch of these docs could be cleaned up to look like this:

    /// Gets the taproot control block following BIP341 rules.
    ///
    /// This does not guarantee that this represents a P2TR [`Witness`]. It merely gets the last or
    /// second to last element depending on the first byte of the last element being equal to 0x50.
    ///
    /// See [`Script::is_p2tr`] to check whether this is actually a Taproot witness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants