-
Notifications
You must be signed in to change notification settings - Fork 81
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
native: add test for custom Koblitz witness verification script #3425
Conversation
3b003ed
to
27f824d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3425 +/- ##
==========================================
+ Coverage 86.12% 86.16% +0.04%
==========================================
Files 332 332
Lines 38435 38443 +8
==========================================
+ Hits 33102 33125 +23
+ Misses 3804 3793 -11
+ Partials 1529 1525 -4 ☔ View full report in Codecov by Sentry. |
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 think it's viable. Minor improvements can be done, but overall it's something that can be discussed.
3fd336f
to
52cbaa0
Compare
daadab9
to
1106552
Compare
@AnnaShaleva I have no preference to solutions, yours here is much more elegent, i can close mine when you are done. But please make it:
Erik has asked to make it scalable, but i think currently we can only focus on dealing k1. And we will need to help COZ to update their neojs SDK as well as other SDKs from other communities. |
1106552
to
75fb036
Compare
Let it be here, CryptoLib extension is there, and I'm still working on the multisignature verification script, will update soon. |
It's based on the constant-length network magic, ref. #3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
75fb036
to
cbf09f2
Compare
@roman-khimov, multisignature version is ready, let's review and improve it (a couple of TODOs are there, we need to decide about them). But overall the algorithm works as expected and is quite clear. Meanwhile I'll add results for other multisig combinations and port CryptoLib's change to the C# repo. |
pkg/crypto/hash/hash.go
Outdated
@@ -46,6 +50,17 @@ func DoubleSha256(data []byte) util.Uint256 { | |||
return hash | |||
} | |||
|
|||
// Keccak256 hashes the incoming byte slice using the | |||
// keccak256 algorithm. | |||
func Keccak256(data []byte) util.Uint256 { |
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.
Please leave it internal to native contract. pkg/crypto is about proper Neo stuff, importing keccak there won't be correct.
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.
But Keccak256 is a proper Neo stuff, and it's not related to the Koblitz-based witnesses. We have keccak256
method in CryptoLib, so it's a part of Neo.
Are you sure we need to keep it inside CryptoLib?
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 exactly. It is available in VM, but you only need it for compatibility with E-chain. And there is zero Neo specifics wrt this hash handling (like Hash160
or DoubleSha256
). Yes, it reminds of RipeMD160
in some ways, but you can't have accounts without it. Keccak
is different, you can live a happy life without it.
I'm mostly concerned about dependency management 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.
Still there, but it's NeoGo-specific.
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.
Fixed.
Multisignature is interesting wrt |
It's based on the constant-length network magic, ref. #3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
10bcdd4
to
f5e4db3
Compare
f5e4db3
to
37d0e73
Compare
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 think we have both simple and multisig working fine at this stage. Simple sig is perfect to me. Multisig can be changed in very minor details, but those are opinionated. Current code is pretty simple and clear, can be reviewed easily. Let's port to C# and collect more feedback.
75020b1
to
fdc3923
Compare
Koblitz-based and Keccak-based transaction witness verification for single signature and multisignature ported from nspcc-dev/neo-go#3425. An alternative to #3205. Signed-off-by: Anna Shaleva <[email protected]>
Make the script a bit shorter. ABORTMSG would cost a bit more. Ported from nspcc-dev/neo-go@fb16891. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
All flag is too wide. A port of nspcc-dev/neo-go@fe292f3. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
Make the script a bit shorter. ABORTMSG would cost a bit more. Ported from nspcc-dev/neo-go@fb16891. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
All flag is too wide. A port of nspcc-dev/neo-go@fe292f3. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
fdc3923
to
d76b736
Compare
…#3209) * Native: extend CryptoLib's verifyWithECDsa with hasher parameter A port of nspcc-dev/neo-go@1e2b438. This commit contains minor protocol extension needed for custom Koblitz-based verification scripts (an alternative to #3205). Replace native CryptoLib's verifyWithECDsa `curve` parameter by `curveHash` parameter which is a enum over supported pairs of named curves and hash functions. NamedCurve enum mark as deprecated and replaced by NamedCurveHash with compatible behaviour. Even though this change is a compatible extension of the protocol, it changes the genesis state due to parameter renaming (CryptoLib's manifest is changed). But we're going to resync chain in 3.7 release anyway, so it's not a big deal. Also, we need to check mainnet and testnet compatibility in case if anyone has ever called verifyWithECDsa with 24 or 25 `curve` value. Signed-off-by: Anna Shaleva <[email protected]> * SmartContract: add extension to ScriptBuilder for System.Contract.Call Group the set of common operations required to emit System.Contract.Call appcall. Signed-off-by: Anna Shaleva <[email protected]> * Native: add an example of custom Koblitz signature verification Koblitz-based and Keccak-based transaction witness verification for single signature and multisignature ported from nspcc-dev/neo-go#3425. An alternative to #3205. Signed-off-by: Anna Shaleva <[email protected]> * SmartContract: make multisig koblitz easier to parse 1. Make prologue be exactly the same as regular CheckMultisig. 2. But instead of "SYSCALL System.Crypto.CheckMultisig" do INITSLOT and K check. 3. This makes all of the code from INITSLOT below be independent of N/M, so one can parse the script beginning in the same way CheckMultisig is parsed and then just compare the rest of it with some known-good blob. 4. The script becomes a tiny bit larger now, but properties above are too good. Ported from nspcc-dev/neo-go@34ee294. Signed-off-by: Anna Shaleva <[email protected]> * SmartContract: use ABORT in Koblitz multisig Make the script a bit shorter. ABORTMSG would cost a bit more. Ported from nspcc-dev/neo-go@fb16891. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]> * SmartContract: reduce callflag scope for Koblitz verification scripts All flag is too wide. A port of nspcc-dev/neo-go@fe292f3. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <[email protected]> * Native: add tests for CryptoLib's verifyWithECDsa No functional changes, just add more unit-tests. Signed-off-by: Anna Shaleva <[email protected]> * Native: update NamedCurveHash values for Keccak256 hasher Use 122 and 123 respectively for secp256k1Keccak256 and secp256r1Keccak256. Signed-off-by: Anna Shaleva <[email protected]> * SmartContract: move EmitAppCallNoArgs to the testing code We're not going to implement custom Koblitz witness generation at the core, and thus, the only user of this API is testing code. Signed-off-by: Anna Shaleva <[email protected]> * Apply suggestions from code review clean ut lines * fix names * Cryptography: cache ECDomainParameters for Secp256r1 and Secp256k1 Signed-off-by: Anna Shaleva <[email protected]> * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs --------- Signed-off-by: Anna Shaleva <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: Jimmy <[email protected]>
Conflicts. |
Every single thing is already implemented in the protocol for Koblitz verification scripts. Signed-off-by: Anna Shaleva <[email protected]>
Replace native CryptoLib's verifyWithECDsa `curve` parameter by `curveHash` parameter which is a enum over supported pairs of named curves and hash functions. Even though this change is a compatible extension of the protocol, it changes the genesis state due to parameter renaming. But we're going to resync chain in 3.7 release anyway, so it's not a big deal. Also, we need to check mainnet and testnet compatibility in case if anyone has ever called verifyWithECDsa with 24 or 25 `curve` value. Signed-off-by: Anna Shaleva <[email protected]>
It's needed for tests and further custom verification script build. Signed-off-by: Anna Shaleva <[email protected]>
It's based on the constant-length network magic, ref. #3425 (comment). Signed-off-by: Anna Shaleva <[email protected]>
Signed-off-by: Anna Shaleva <[email protected]>
Value calculated by calculatenetworkfee is enough to pass the real tx verification. However, network fee may be decreased, so calculations are not quite accurate. Need to investigate, why. Signed-off-by: Anna Shaleva <[email protected]>
1. Make prologue be exactly the same as regular CheckMultisig. 2. But instead of "SYSCALL System.Crypto.CheckMultisig" do INITSLOT and K check. 3. This makes all of the code from INITSLOT below be independent of N/M, so one can parse the script beginning in the same way CheckMultisig is parsed and then just compare the rest of it with some known-good blob. 4. The script becomes a tiny bit larger now, but properties above are too good. Signed-off-by: Roman Khimov <[email protected]> Signed-off-by: Anna Shaleva <[email protected]>
Make the script a bit shorter. ABORTMSG would cost a bit more. Signed-off-by: Roman Khimov <[email protected]> Signed-off-by: Anna Shaleva <[email protected]>
callflag.All is too wide. Signed-off-by: Anna Shaleva <[email protected]>
875f9e8
to
67f9c10
Compare
Use 122 and 123 respectively for Secp256k1Keccak256 and Secp256r1Keccak256, ref. neo-project/neo#3209 (comment). Signed-off-by: Anna Shaleva <[email protected]>
67f9c10
to
6e0926e
Compare
Signed-off-by: Anna Shaleva <[email protected]>
Every single thing is already implemented in the protocol for Koblitz verification scripts. An alternative to neo-project/neo#3205.
A note for reviewers
This PR contains a large number of file changes, but two most important files are:
verifyWithECDsa
extension (./pkg/core/native/crypto.go
file);./pkg/core/native/native_test/cryptolib_verificacation_test.go
file);Single signature verification
keccak256([4-bytes-network-magic-LE, txHash-bytes-BE])
**keccak256([var-bytes-network-magic, txHash-bytes-BE])
keccak256([var-bytes-network-magic, txHash-bytes-BE])
(static magic)keccak256(sha256([var-bytes-network-magic, txHash-bytes-BE]))
keccak256(sha256([4-bytes-network-magic-LE, txHash-bytes-BE]))
* Verification cost is based on the default execution fee value.
** The proposed verification method. Other methods are less preferable and are added for comparison.
Multisignature verification
keccak256([4-bytes-network-magic-LE, txHash-bytes-BE])
*** Verification cost is based on the default execution fee value.
** The proposed verification method. Other methods are less preferable and are added for comparison.
@roman-khimov, let's review&improve&merge, and after that I may create a comment in the core repo (or even an issue).