Refactor: Initialize PrecomputedTransactionData in CheckInputScripts #18401

jnewbery · 2020-03-22T03:18:03Z

This is a single commit taken from the Schnorr/Taproot PR #17977.

Add a default constructor to PrecomputedTransactionData, which doesn't initialize the struct's members. Instead they're initialized inside the CheckInputScripts() function. This allows a later commit to add the spent UTXOs to that structure. The spent UTXOs are required for the schnorr signature hash, since it commits to the scriptPubKeys. See https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#common-signature-message for details.

By itself, this isn't really an improvement to the code, but I think it makes sense to separate out the refactor/moveonly commits from PR #17977 so that PR is only the logical changes needed for Schnorr/Taproot.

DrahtBot · 2020-03-22T10:39:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 #18071 (Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 by JeremyRubin)
Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/validation.cpp

src/script/interpreter.cpp

sipa · 2020-03-26T00:20:37Z

Concept ACK, of course.

sipa · 2020-03-28T01:18:12Z

Code review ACK, post squash, and for 0.21.

jnewbery · 2020-03-28T03:06:53Z

Squashed commits. No code changes.

theStack

Code-review ACK b409b61 💻
Checked that the resulting code logic remains the same, the only internal difference is that the initialization of the precomputed transaction data happens later, in CheckInputScripts() rather than in AcceptSingleTransaction().

instagibbs · 2020-04-01T14:18:42Z

src/script/interpreter.cpp

-    if (txTo.HasWitness()) {
+    if (!txTo.HasWitness()) return;
+
+    if (!ready) {


while we're touching this code can we make ready be something more self-explanatory? Especially if we're expanding condition checks in the future. e.g., m_amounts_spent_ready

This, plus the discussion here: https://github.com/bitcoin/bitcoin/pull/18401/files#r401812158 has made me think that there shouldn't be two different ready bools in the final taproot implementation, since we only ever want to initialize the object once. I've changed this to m_ready and made some minor changes to the Init function.

fjahr · 2020-04-01T16:13:53Z

Code-review ACK b409b61

Also ran tests locally.

jonatack

ACK b409b61

src/validation.cpp

src/script/interpreter.cpp

sipa · 2020-04-01T21:20:30Z

@jnewbery I guess this calls for splitting up CheckInputScripts into two separate functions (one looking up inputs and initializing txdata, and one doing the rest). I suggest keeping that for a future improvement, though.

jnewbery · 2020-04-01T21:31:53Z

@sipa

... this calls for splitting up CheckInputScripts...

Sounds sensible now that it has a potentially unexpected side-effect of initializing the txdata. I agree that we should save that for later.

Would you prefer to leave the assert in this commit, or revert to not having it?

sipa · 2020-04-01T21:50:13Z

Would you prefer to leave the assert in this commit, or revert to not having it?

No opinion either way.

src/script/interpreter.cpp

jonatack

re-ACK 851908d for 0.21

src/validation.cpp

maflcko · 2020-04-02T18:02:50Z

Does this speed up IBD?

sipa · 2020-04-02T18:07:05Z

@MarcoFalke Probably slightly, as it avoids constructing precomputed tx data in the assumevalid range.

Add a default constructor to `PrecomputedTransactionData`, which doesn't initialize the struct's members. Instead they're initialized inside the `CheckInputScripts()` function. This allows a later commit to add the spent UTXOs to that structure.

jnewbery · 2020-04-12T01:33:11Z

Squashed commits

jonatack · 2020-04-14T18:59:41Z

Re-ACK f63dec1 git diff 851908d f63dec1 shows no change since last ACK.

jnewbery · 2020-04-15T00:25:29Z

I think this should be ready for merge if @sipa @fjahr @theStack reACK.

sipa · 2020-04-15T06:48:55Z

utACK f63dec1

ariard

Code Review ACK f63dec1

Add a commit commenting PrecomputedTransactionData, there is a lot of context around it : ariard@5624724

Few aside questions, :

As far as I can tell, we're going to compute "midstate" even if validation fail before signature verification (like a script spending a coin already spent), instead of hashing-at-first-use ? (not sure if ternary in SignatureHash are currently exercised)
Why we don't cache nVersion and nLocktime they won't change given the sigops, is this because they're not worth it considering their size ?

I guess minimizing code complexity answers both.

theStack · 2020-04-15T10:31:20Z

re-ACK f63dec1
Verified that since my last ACK only minor changes happened that didn't change the overall logic (s/ready/m_ready/, conditional call of .Init() method in CheckInputScripts(), minor reordering of conditions and added assert() in .Init())

jnewbery · 2020-04-15T14:11:35Z

As far as I can tell, we're going to compute "midstate" even if validation fail before signature verification (like a script spending a coin already spent), instead of hashing-at-first-use ? (not sure if ternary in SignatureHash are currently exercised)

No. Coins checks are done in MemPoolAccept::PreChecks(), which is called before PrecomputedTransactionData is instantiated (both before and after this PR).

Why we don't cache nVersion and nLocktime they won't change given the sigops, is this because they're not worth it considering their size ?

Because those fields aren't hashed before going into the sighash message. hashPrevouts, hashSequence and hashOutputs are all hashed before going into that message, so caching the hash digests means we don't have to recalculate those hashes every time.

Add a commit commenting PrecomputedTransactionData, there is a lot of context around it : ariard/bitcoin@5624724

Feel free to open a follow-up PR. No need to overload this one now that we have several ACKs.

fjahr · 2020-04-15T14:42:58Z

Re-ACK f63dec1

Only changes since my last ACK were minor improvements addressing review comments and squash of commits.

ariard · 2020-04-15T18:27:18Z

No. Coins checks are done in MemPoolAccept::PreChecks(), which is called before PrecomputedTransactionData is instantiated (both before and after this PR).

Coin checks was just a (bad) example, you may have a scriptpubkey without any sigops or script may fail before first one.

Because those fields aren't hashed before going into the sighash message. hashPrevouts, hashSequence and hashOutputs are all hashed before going into that message, so caching the hash digests means we don't have to recalculate those hashes every time.

Hmmm right, reusing their hash would break final digest, I understand better what BIP143 aim to achieve compare to original transaction digest algo.

Feel free to open a follow-up PR. No need to overload this one now that we have several ACKs.

Will do, ofc no need to break ACKs for docs!

jnewbery · 2020-04-15T18:55:54Z

Coin checks was just a (bad) example, you may have a scriptpubkey without any sigops or script may fail before first one.

We need the precomputed transaction data to be cached before any script checking because in the ConnectBlock case, each input script validation is parallelized separately. If the cache was filled during script execution, you'd need locking between those threads to ensure they're not trying to read/write concurrently.

ariard · 2020-04-15T19:32:17Z

We need the precomputed transaction data to be cached before any script checking because in the ConnectBlock case

Right but not in the mempool case. Thanks for answer, will update comment with this insight in a follow-up PR.

…in CheckInputScripts f63dec1 [REFACTOR] Initialize PrecomputedTransactionData in CheckInputScripts (Pieter Wuille) Pull request description: This is a single commit taken from the Schnorr/Taproot PR bitcoin#17977. Add a default constructor to `PrecomputedTransactionData`, which doesn't initialize the struct's members. Instead they're initialized inside the `CheckInputScripts()` function. This allows a later commit to add the spent UTXOs to that structure. The spent UTXOs are required for the schnorr signature hash, since it commits to the scriptPubKeys. See https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#common-signature-message for details. By itself, this isn't really an improvement to the code, but I think it makes sense to separate out the refactor/moveonly commits from PR bitcoin#17977 so that PR is only the logical changes needed for Schnorr/Taproot. ACKs for top commit: jonatack: Re-ACK f63dec1 `git diff 851908d f63dec1` shows no change since last ACK. sipa: utACK f63dec1 theStack: re-ACK f63dec1 fjahr: Re-ACK f63dec1 ariard: Code Review ACK f63dec1 Tree-SHA512: ecf9154077824ae4c274b4341e985797f3648c0cb0c31cb25ce382163b923a3acbc7048683720be4ae3663501801129cd0f48c441a36f049cc304ebe9f30994e

maflcko · 2020-04-24T14:40:36Z

I did some more benchmarking and this should speed up CreateNewBlock by a factor inversely related to the number of transactions in the block.

So a block with one transaction (beside the coinbase tx) should be generated about twice as fast. Whereas a block with 1000 txs (beside the coinbase tx) should be generated at a slight but almost no speedup.

…in CheckInputScripts f63dec1 [REFACTOR] Initialize PrecomputedTransactionData in CheckInputScripts (Pieter Wuille) Pull request description: This is a single commit taken from the Schnorr/Taproot PR bitcoin#17977. Add a default constructor to `PrecomputedTransactionData`, which doesn't initialize the struct's members. Instead they're initialized inside the `CheckInputScripts()` function. This allows a later commit to add the spent UTXOs to that structure. The spent UTXOs are required for the schnorr signature hash, since it commits to the scriptPubKeys. See https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#common-signature-message for details. By itself, this isn't really an improvement to the code, but I think it makes sense to separate out the refactor/moveonly commits from PR bitcoin#17977 so that PR is only the logical changes needed for Schnorr/Taproot. ACKs for top commit: jonatack: Re-ACK f63dec1 `git diff 851908d f63dec1` shows no change since last ACK. sipa: utACK f63dec1 theStack: re-ACK f63dec1 fjahr: Re-ACK f63dec1 ariard: Code Review ACK f63dec1 Tree-SHA512: ecf9154077824ae4c274b4341e985797f3648c0cb0c31cb25ce382163b923a3acbc7048683720be4ae3663501801129cd0f48c441a36f049cc304ebe9f30994e

fanquake added the Refactoring label Mar 22, 2020

jnewbery mentioned this pull request Mar 22, 2020

Future PRs bitcoin-core-review-club/website#14

Closed

This was referenced Mar 22, 2020

Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 #18071

Closed

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977

Closed

promag reviewed Mar 22, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 23, 2020

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Closed

sipa reviewed Mar 26, 2020

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

laanwj added this to the 0.21.0 milestone Mar 27, 2020

jnewbery force-pushed the 2020-03-precomputedtransactiondata branch from de43efc to b409b61 Compare March 28, 2020 03:06

theStack approved these changes Mar 29, 2020

View reviewed changes

instagibbs reviewed Apr 1, 2020

View reviewed changes

jonatack reviewed Apr 1, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Apr 1, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

jkczyz reviewed Apr 1, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

bitcoin deleted a comment from sipa Apr 1, 2020

vasild reviewed Apr 2, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

jonatack reviewed Apr 2, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

jnewbery force-pushed the 2020-03-precomputedtransactiondata branch from 851908d to f63dec1 Compare April 12, 2020 01:32

ariard reviewed Apr 15, 2020

View reviewed changes

maflcko merged commit e16718a into bitcoin:master Apr 16, 2020

jnewbery deleted the 2020-03-precomputedtransactiondata branch April 16, 2020 12:55

robot-visions mentioned this pull request Apr 18, 2020

tests: Don't initialize PrecomputedTransactionData in txvalidationcache tests #18675

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Refactor: Initialize PrecomputedTransactionData in CheckInputScripts #18401

Refactor: Initialize PrecomputedTransactionData in CheckInputScripts #18401

Uh oh!

Conversation

jnewbery commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Mar 26, 2020

Uh oh!

sipa commented Mar 28, 2020

Uh oh!

jnewbery commented Mar 28, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr commented Apr 1, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Apr 1, 2020

Uh oh!

jnewbery commented Apr 1, 2020

Uh oh!

sipa commented Apr 1, 2020

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 2, 2020

Uh oh!

sipa commented Apr 2, 2020

Uh oh!

jnewbery commented Apr 12, 2020

Uh oh!

jonatack commented Apr 14, 2020

Uh oh!

jnewbery commented Apr 15, 2020

Uh oh!

sipa commented Apr 15, 2020

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack commented Apr 15, 2020

Uh oh!

jnewbery commented Apr 15, 2020

Uh oh!

fjahr commented Apr 15, 2020

Uh oh!

ariard commented Apr 15, 2020

Uh oh!

jnewbery commented Apr 15, 2020

Uh oh!

ariard commented Apr 15, 2020

Uh oh!

maflcko commented Apr 24, 2020

Uh oh!

jnewbery commented Mar 22, 2020 •

edited

Loading

DrahtBot commented Mar 22, 2020 •

edited

Loading

instagibbs Apr 1, 2020 •

edited

Loading

ariard left a comment •

edited

Loading