Improve speed and accuracy of zcbenchmark validatelargetx
The verification test, in a loop, passes `spending_tx` (a `CMutableTransaction`) to the constructor of `MutableTransactionSignatureChecker`, which immediately uses it to create a non-mutable `CTransaction` object, which is used for the actual verification process.
Since `spending_tx` is not mutated during the verification loop & process, we can instead convert it to a `CTransaction` just once, and use it with `TransactionSignatureChecker`.
This removes the time to create `CTransaction` objects from the benchmark itself.
Results show an improvement in running time to complete the verification phase of the test and consistent times across z7 and z8 releases.
```
Benchmarks on i3 processor:
z7 old 228.67205900 --> z7 new 49.27225200
z7 old 229.90048900 --> z7 new 48.38650700
z8 old 295.77963800 --> z8 new 48.37695100
z8 old 294.32640100 --> z8 new 49.93216100
```
Add gtest for non-malleable txids.
1. Test txid of normal transaction is double sha256 with sigscript input data removed
2. Test txid of coinbase transaction is double sha256 over whole tx
You can run a single test with:
./zcash-gtest --gtest_filter="txid_tests*"
A fix for transaction malleability
This PR fixes transaction malleability by not including the sigscript of transaction inputs and joinsplit sigs when hashing the txid.
This PR supercedes PR #1101 which was a minimal solution based on a new serialization flag.
This PR introduces GetTxid() to distinguish between getting a transaction id and the double sha256 hash.
The key changes are:
- Adding GetTxid() method to CTransaction which makes a copy of the transaction, clearing out the sigscript and joinsplitsig fields, before hashing.
- Verifying that every call to GetHash() actually wants a txid, and replacing with GetTxid().
- Renaming GetHash() to GetSerializeHash()
- Rationale: In future, upstream code we want to merge will use GetHash() but we don't know the intent. We should check to see if the intent is to receive a txid (most likely) in which case we replace with GetTxid(), or if upstream actually wants a double hash of the transaction we can use GetSerializeHash().
- Updated genesis data in chainparams.cpp
Note that coinbase transactions are excluded as they need the sigscript hashed to help avoid duplicate txids per BIP34:
- This modification is related to a question from @ebfull on PR #1101 - "Can we think of a way this change allows us to construct two transactions with the same txid which can simultaneously appear in the blockchain? My guess is it would be possible to construct a coinbase transaction of such a form... this surely breaks invariants."
This PR Passes all tests in test_bitcoin (test data was updated in bloom_tests, miner_tests and script_tests).
Remove the assumption that n/(k+1) is a multiple of 8
This version works, but generates the initial rows in a way that is probably
not what we want to specify.
Closes#1148
This version works, but generates the initial rows in a way that is not what we
want to specify. See #1175 for resolving this.
Co-author: Daira Hopwood <daira@jacaranda.org>
[WIP] Add more commands to run unit tests under valgrind.
This runs both zcash-gtest and test_bitcoin under valgrind. There's a corresponding PR to the buildbot config https://github.com/Electric-Coin-Company/bbotzc/pull/23. Closes#761.
Extend try catch block around call to libsnark verifier
As discussed in #1126.
@daira Per your [comment](https://github.com/zcash/zcash/pull/1126#issuecomment-234714939):
> I would like assertion errors during tests to cause a test failure (unless the test explicitly expects them). Can we split this into verify and verify_internal, where the former does the try/catch around a call to verify_internal, and the latter is called by verification unit tests?
Did you mean move everything inside the extended try/catch to verify_internal, or just the call to r1cs_ppzksnark_verifier_strong_IC?
Taylor Hornby
zkbot
Robert C. Seacord
Simon
Jack Grigg