Optimize VerifyWitness and tx.verify #2054
Merged
Conversation
This file contains 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
|
Plese wait for #2052 |
Closed
added 2 commits
November 25, 2020 10:47
|
What does this optimize? |
We'll add adjustable opcode pricing later and also want to keep Therefore, we want to separate signature verification from cost verification. |
roman-khimov
added a commit
to roman-khimov/neo
that referenced
this pull request
Nov 26, 2020
MemoryPool contents is always valid (verified) against some snapshot. This snapshot is only changed when new block is added. Between blocks we only have one valid chain state that can be read by multiple threads without any issues, thus we can execute concurrently not only state-independent, but also state-dependent parts of transaction verification. To simplify execution flow (minimize single-threaded Blockchain message handling and eliminate duplicate DB accesses for ContainsTransaction) TransactionRouter is an independent entity now, though of course we can also attach the same actor functionality to MemoryPool itself. TransactionVerificationContext balance checking is moved to MemoryPool from Transaction with this, Transaction shouldn't care (it can check overall GAS balance though). This is directly related to neo-project#2045 work (it solves state access problem there) and in some sense is an alternative to neo-project#2054 (makes fee calculation easier, though IsStandardContract() trick to optimize out these contracts during reverification is still relevant and can be added here). At this stage it's just a prototype, some additional optimizations and simplifications are possible of course, but this prototype shows the direction and the main question for now is whether this direction is interesting for us.
|
Conflicts. |
added 2 commits
December 2, 2020 14:24
|
Attached is TPS comparison between this PR and newest master branch code. Env: 1 consensus node with 6 i7 cores and 16 GB rams
Besides, a TPS comparison between the newest master branch code and commit 6fc4161:
It seems that TPS has dropped since then. |
|
Looks like a revert from #1507 |
Some partes is reverted. |
erikzhang
reviewed
Dec 2, 2020
added 2 commits
December 2, 2020 20:10
erikzhang
reviewed
Dec 2, 2020
shargon
reviewed
Dec 2, 2020
| if (witnesses[i].VerificationScript.IsSignatureContract()) | ||
| net_fee -= (ApplicationEngine.OpCodePrices[OpCode.PUSHDATA1] * 2 + ApplicationEngine.OpCodePrices[OpCode.PUSHNULL] + ApplicationEngine.OpCodePrices[OpCode.SYSCALL] + ApplicationEngine.ECDsaVerifyPrice); | ||
| else if (witnesses[i].VerificationScript.IsMultiSigContract(out int m, out int n)) | ||
| net_fee -= (ApplicationEngine.OpCodePrices[OpCode.PUSHDATA1] * (m + n) + ApplicationEngine.OpCodePrices[OpCode.PUSHINT8] * 2 + ApplicationEngine.OpCodePrices[OpCode.PUSHNULL] + ApplicationEngine.OpCodePrices[OpCode.SYSCALL] + ApplicationEngine.ECDsaVerifyPrice * n); |
There was a problem hiding this comment.
It could be any kind of push, same price but just to take care..
shargon
reviewed
Dec 2, 2020
| UInt160[] hashes = GetScriptHashesForVerifying(null); | ||
| if (hashes.Length != witnesses.Length) return VerifyResult.Invalid; | ||
| for (int i = 0; i < hashes.Length; i++) | ||
| if (witnesses[i].VerificationScript.IsStandardContract()) |
There was a problem hiding this comment.
Maybe we can cache if it's standar inside the witness, otherwise we check it at least twice.
shargon
reviewed
Dec 2, 2020
shargon
reviewed
Dec 3, 2020
Co-authored-by: Shargon <[email protected]>
erikzhang
reviewed
Dec 3, 2020
shargon
reviewed
Dec 3, 2020
Co-authored-by: Shargon <[email protected]>
erikzhang
reviewed
Dec 4, 2020
added 2 commits
December 4, 2020 15:23
erikzhang
reviewed
Dec 4, 2020
erikzhang
approved these changes
Dec 4, 2020
shargon
reviewed
Dec 4, 2020
| return true; | ||
| } | ||
|
|
||
| internal static bool VerifyWitness(this IVerifiable verifiable, StoreView snapshot, UInt160 hash, Witness witness, long gas, out long fee) |
There was a problem hiding this comment.
fee it's only computed if it return true, it works for our current model, but we should take care.
shargon
approved these changes
Dec 4, 2020
|
Merge? |
ShawnYun
pushed a commit
to ShawnYun/neo
that referenced
this pull request
Jan 8, 2021
cloud8little
pushed a commit
to cloud8little/neo
that referenced
this pull request
Jan 24, 2021
ixje
added a commit
to CityOfZion/neo-mamba
that referenced
this pull request
Feb 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.