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

Adapt the Transaction::verify API to not require mutating state #327

Closed
wants to merge 1 commit into from

Conversation

@stevenroose
Copy link
Collaborator

commented Sep 5, 2019

Proposal to improve a bit on #319.

I think it makes more sense for the verify method to keep track of
double-spending itself instead of requiring the closure to do that.
When internally keeping track of double-spending, the closure can just
be a simple inspector closure and doesn't need to mutate any state,
making it a lot simpler and potentially more efficient.

Also, OutPoint is Copy, so doesn't need to be referenced.

@codecov-io

This comment has been minimized.

Copy link

commented Sep 5, 2019

Codecov Report

Merging #327 into master will increase coverage by 0.28%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   81.98%   82.26%   +0.28%     
==========================================
  Files          38       38              
  Lines        6998     7095      +97     
==========================================
+ Hits         5737     5837     +100     
+ Misses       1261     1258       -3
Impacted Files Coverage Δ
src/blockdata/transaction.rs 94.64% <89.47%> (+1.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de9ccde...2534bdd. Read the comment docs.

where S: FnMut(&OutPoint) -> Option<TxOut> {
let tx = serialize(&*self);
pub fn verify<'a, S>(&self, get_utxo: S) -> Result<(), script::Error>
where S: Fn(OutPoint) -> Option<&'a TxOut> {

This comment has been minimized.

Copy link
@apoelstra

apoelstra Sep 5, 2019

Member

Since you now call it only once, FnOnce would be more general and more appropriate.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Sep 6, 2019

Author Collaborator

I never fully understood Fn vs FnOnce. You call the method for every input, right? So not once. Unless "once" here has some extra meaning I'm missing.

This comment has been minimized.

Copy link
@elichai

elichai Sep 6, 2019

Contributor

FnOnce is a closure that cannot be called more than once, usually by moving a non Copy type from the outside environment to the closure.
example: https://play.rust-lang.org/?gist=9f86723ad4d2b0530a45d4047f311efc

This comment has been minimized.

Copy link
@apoelstra

apoelstra Sep 6, 2019

Member

Oh, you're right, it's called in a loop. So you need to use FnMut.

Please do not use Fn. It is never necessary and always restrictive.

@stevenroose stevenroose force-pushed the stevenroose:tx-verify branch from 85c9eb5 to 2b641f1 Sep 6, 2019

@stevenroose stevenroose changed the title Adapt the Transaction::verify API to not use FnMut Adapt the Transaction::verify API to not require mutating state Sep 10, 2019

@tamasblummer

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

An important step of transaction verification is that it does only spend an output once, this is easy to forget as we have seen in Bitcoin Core. If you want Fn instead of FnMut then the function itself should prove that inputs are not reused. That would be better than the current code that relies that used outputs are removed.

@stevenroose stevenroose force-pushed the stevenroose:tx-verify branch from 2b641f1 to d8812d0 Sep 13, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2019

An important step of transaction verification is that it does only spend an output once, this is easy to forget as we have seen in Bitcoin Core. If you want Fn instead of FnMut then the function itself should prove that inputs are not reused. That would be better than the current code that relies that used outputs are removed.

This PR also does that. It checks for double spends inside the tx. For block verification or so, the verifier has to keep track of inputs on an extra level. One could argue that this method is not useful in that scenario, as then you'd be checking twice. But we learned from experience that we better check twice than accidentally not at all ;)

@tamasblummer
Copy link
Member

left a comment

Like as is and we could add one more check cheaply: some of inputs <= sum of outputs

@tamasblummer

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

maybe also no output should be below DUST limit.

@tamasblummer

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

maybe also check that it meets a target fee/vByte (parameter to verify)

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2019

I fixed the FnOnce->FnMut change. But while trying to address your other comments, @tamasblummer , #330 made more sense to me. Otherwise we'd need a script::Error::AmountsDontMatch error or something and that's strange.

@stevenroose stevenroose force-pushed the stevenroose:tx-verify branch from 8afc1d0 to 2534bdd Sep 13, 2019

Adapt the Transaction::verify API to not require mutations
I think it makes more sense for the verify method to keep track of
double-spending itself instead of requiring the closure to do that.
When internally keeping track of double-spending, the closure can just
be a simple inspector closure and doesn't need to mutate any state,
making it a lot simpler and potentially more efficient.

Also, OutPoint is Copy, so doesn't need to be referenced.
@tamasblummer

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

yeah, let's close this one then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.