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
Add verify_with_flags to Script and Transaction #598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I meant to add this a while ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3aaa5d6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 3aaa5d6
Also, this looks non-API-breaking, will tag it that way unless someone disagrees and I overlooked something. |
/// * index - the input index in spending which is spending this transaction | ||
/// * amount - the amount this script guards | ||
/// * spending - the transaction that attempts to spend the output holding this script | ||
/// Shorthand for [Self::verify_with_flags] with flag [bitcoinconsensus::VERIFY_ALL] | ||
pub fn verify (&self, index: usize, amount: u64, spending: &[u8]) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be Amount
? (especially once we adopt wider Amount
use like in #599)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep this non API breaking and not change yet the verify
method signature, but I changed the amount type in verify_with_flags
in 69117a1
src/blockdata/script.rs
Outdated
/// * `amount` - the amount this script guards | ||
/// * `spending` - the transaction that attempts to spend the output holding this script | ||
/// * `flags` - verification flags, see [bitcoinconsensus::VERIFY_ALL] and similar | ||
pub fn verify_with_flags<F: Into<u32>>(&self, index: usize, amount: u64, spending: &[u8], flags: F) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ibid
pub fn verify_with_flags<F: Into<u32>>(&self, index: usize, amount: u64, spending: &[u8], flags: F) -> Result<(), Error> { | |
pub fn verify_with_flags<F: Into<u32>>(&self, index: usize, amount: Amount, spending: &[u8], flags: F) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 69117a1
src/blockdata/transaction.rs
Outdated
pub fn verify<S>(&self, mut spent: S) -> Result<(), script::Error> | ||
where S: FnMut(&OutPoint) -> Option<TxOut> { | ||
pub fn verify_with_flags<S, F>(&self, mut spent: S, flags: F) -> Result<(), script::Error> | ||
where S: FnMut(&OutPoint) -> Option<TxOut>, F : Into<u32> + Copy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we need to require flags to be Copy
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that flags is used in a for, but yeah I solved by creating the u32 before in d1f4c0a
69117a1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 69117a1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 69117a1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 69117a1
In https://github.com/RCasatta/blocks_iterator/blob/verify/examples/verify.rs I would like to verify mainnet and testnet blocks. However, at the moment the verify flag is not exposed in the API so it's not possible (without using bitcoinconsensus directly).
I preserved the original method and added
verify_with_flags
so that it should not be a breaking change.The flag parameter is taken as
Into<u32>
so that if we introduce a VerifyFlags type/builder in bitcoinconsensus the migration should be more smooth.