Skip PrecomputedTransactionData hashing for cache hits.#13233
Closed
TheBlueMatt wants to merge 2 commits intobitcoin:masterfrom
Closed
Skip PrecomputedTransactionData hashing for cache hits.#13233TheBlueMatt wants to merge 2 commits intobitcoin:masterfrom
TheBlueMatt wants to merge 2 commits intobitcoin:masterfrom
Conversation
fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless.
maflcko
reviewed
May 14, 2018
| bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) | ||
| bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) | ||
| { | ||
| if (!tx.IsCoinBase()) |
Member
There was a problem hiding this comment.
nit: since you re-indent the whole function body you might also remove this scope and put a if (tx.IsCoinBase()) return true; in the first line.
Contributor
There was a problem hiding this comment.
Yeah, I also prefer early return in these cases.
promag
reviewed
May 14, 2018
| bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) | ||
| bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) | ||
| { | ||
| if (!tx.IsCoinBase()) |
Contributor
There was a problem hiding this comment.
Yeah, I also prefer early return in these cases.
promag
reviewed
May 14, 2018
| { | ||
| // Cache is calculated only for transactions with witness | ||
| if (txTo.HasWitness()) { | ||
| if (!ready && txTo.HasWitness()) { |
Contributor
There was a problem hiding this comment.
It should not be possible to call ComputeHashes with different transactions right? I have verified that currently it's the case, but maybe we should prevent invalid usage?
Contributor
|
No benchmarks? |
fanquake
added a commit
that referenced
this pull request
Sep 2, 2019
9b92538 Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo) Pull request description: fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless. This is extracted from #13233 /cc TheBlueMatt. Recommend reviewing with `git show --ignore-all-space`, i.e.: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1 ACKs for top commit: TheBlueMatt: utACK 9b92538. Checked diff had no functional change and new comment copy looks correct. kallewoof: ACK 9b92538 ajtowns: ACK 9b92538 ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to `assert(!tx.IsCoinBase());`. fanquake: ACK 9b92538 - Notes / testing below. Tree-SHA512: add253a3e8cf4b33eddbc49efcec333c14b5ea61c7d34e43230351d40cff6adc919a75b91c72c4de8647a395284db74a61639f4c67848d4b2fec3a705b557790
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 3, 2019
…Inputs 9b92538 Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo) Pull request description: fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless. This is extracted from bitcoin#13233 /cc TheBlueMatt. Recommend reviewing with `git show --ignore-all-space`, i.e.: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1 ACKs for top commit: TheBlueMatt: utACK 9b92538. Checked diff had no functional change and new comment copy looks correct. kallewoof: ACK 9b92538 ajtowns: ACK 9b92538 ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to `assert(!tx.IsCoinBase());`. fanquake: ACK 9b92538 - Notes / testing below. Tree-SHA512: add253a3e8cf4b33eddbc49efcec333c14b5ea61c7d34e43230351d40cff6adc919a75b91c72c4de8647a395284db74a61639f4c67848d4b2fec3a705b557790
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We currently calculate PrecomputedTransactionData for all transactitons that appear in blocks, but this is entirely uneccessary on the vast majority of transactions near the tip due to the script cache. Thus, we move the calculation into CheckInputs after the script cache check. I played around with a number of refactors to CheckInputs to do this a bit cleaner but it seems rather difficult to do so, so I just stuck with the obvious version