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.

Sign up for GitHub

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

Jump to bottom

Separate out bitcoinconsensus validation code #1912

Merged
merged 5 commits into from Jul 25, 2023
Merged

Separate out bitcoinconsensus validation code #1912

merged 5 commits into from Jul 25, 2023

Conversation

tcharding
Copy link
Member

Pull all the code that depends on bitcoinconsensus out into a separate module consensus::validation.

While we are at it clean up the error returns by making functions return error types specific to their needs instead of general error types. We still need the std error hack to implement std::error::Error if bitcoinconsensus feature is on but not bitcoinconsensus-std.

This is a re-spin of work done by @stevenroose in #330, things that are different:

  • Use specific error types for script vs transaction error instead of a single general error
  • Did improvements to docs including linking to docsrs for bitcoinconsensus type.

apoelstra
apoelstra previously approved these changes Jun 21, 2023
View reviewed changes
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 bf20993

yancyribbens
yancyribbens reviewed Jun 22, 2023
View reviewed changes
bitcoin/Cargo.toml Outdated Show resolved Hide resolved
@yancyribbens
Copy link
Contributor

While we are at it clean up the error returns by making functions return
error types specific to their needs instead of general error types

Could that have been in a separate commit? Seems like it could be easier to read/review if it was a few commits.

@tcharding
Copy link
Member Author

tcharding commented Jun 23, 2023 •
edited

Could that have been in a separate commit? Seems like it could be easier to read/review if it was a few commits.

Yep sure thing, I can do that. Thanks for the review!

@tcharding tcharding force-pushed the 06-20-consensus-validation branch 2 times, most recently from d085d61 to 222cb0c Compare June 23, 2023 05:22
@tcharding
Copy link
Member Author

The error changes are now separate. The final patch is just splitting the bitcoinconsenus code out to the new consensus::validation module.

@tcharding tcharding force-pushed the 06-20-consensus-validation branch 2 times, most recently from de1ba99 to 467d4bc Compare June 23, 2023 06:07
@apoelstra
Copy link
Member

review checkpoint 467d4bc

@tcharding
Copy link
Member Author

Fixed review suggestions and also simplified the consensus import in validation.rs.

apoelstra
apoelstra approved these changes Jun 28, 2023
View reviewed changes
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 ea79465

@tcharding
Copy link
Member Author

Rebased on master (to pick up #1927). No other changes.

@tcharding
Copy link
Member Author

@stevenroose can we get an ack/nack please (since this was your work originally).

apoelstra
apoelstra previously approved these changes Jul 13, 2023
View reviewed changes
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 a9d5761

@tcharding
Copy link
Member Author

Rebase only, no other changes.

@tcharding
Add comment to bitcoinconsensus dependency
8ed74c4 
Add a comment to point users towards the `bitcoinconsensus-std` feature
for std builds.
@tcharding
script: Return bitcoinconsensus::Error from verify functions
ca2512f 
There is no need no nest the `bitcoinconsensus::Error` type within the
`script::Error`, it is the only error type returned by the verify
functions so just return it directly.
@tcharding
transaction:: Return custom error from verify function
ca38dbd 
There is not need to return the general `script::Error` from the
transaction verify functions. We can better describe the error path by
returning a custom error.
@tcharding
Remove unused import statement
0cd1af1 
This import is not used, our CI obviously does not warn for all feature
combinations.
@tcharding
Separate out bitcoinconsensus validation code
dae2b50 
Pull all the code that depends on `bitcoinconsensus` out into a separate
module `consensus::validation`.

Leave transaction testing of bitcoinconsensus code in the transaction
module.
@tcharding
Copy link
Member Author

Pulled master and rebased, woops.

apoelstra
apoelstra approved these changes Jul 18, 2023
View reviewed changes
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 dae2b50

stevenroose
stevenroose approved these changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

utACK

@stevenroose stevenroose merged commit 56343bd into rust-bitcoin:master Jul 25, 2023
29 checks passed
@apoelstra
Copy link
Member

@stevenroose in future could you use the bitcoin core merge script to do merges?

@tcharding tcharding deleted the 06-20-consensus-validation branch July 26, 2023 21:28
@tcharding tcharding mentioned this pull request Aug 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yancyribbens yancyribbens yancyribbens left review comments

@stevenroose stevenroose stevenroose approved these changes

@apoelstra apoelstra apoelstra 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.

None yet
4 participants
@tcharding @yancyribbens @apoelstra @stevenroose