In the vicinity of a network upgrade, a zcashd node may receive headers
for a non-upgrading chain from its non-upgraded peers (e.g. if the block
at the NU activation height is found more quickly by the non-upgrading
chain). In this situation, the node will end up with two headers at the
NU activation height (and possibly for subsequent block heights).
In the case of Heartwood, the block headers from the non-upgrading chain
do not satisfy the Heartwood header consistency check in
CBlockTreeDB::LoadBlockIndexGuts. In this commit, we restrict the
Heartwood consistency checks to block index objects that were created by
clients that are CHAIN_HISTORY_ROOT_VERSION or better.
Fix advertised version
Closes https://github.com/zcash/zcash/issues/4375 by adding the `-` character to the list of safe ones.
Now i can see stuff like the following in the logs:
```
...
2020-04-13 14:19:37 receive version message: /MagicBean:2.1.1-1/: version 170009, blocks=795400, us=[2800:a4:316b:8e00:ceb:c7b4:3481:197f]:59754, peer=2
...
```
API call `getpeerinfo` will also gets fixed.
memory_cleanse backports
Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#10308
- bitcoin/bitcoin#11196
- bitcoin/bitcoin#11558
- Only the changes that did not conflict.
- bitcoin/bitcoin#16158
Part of #145.
So far, the documentation of memory_cleanse() is a verbatim copy of
the commit message in BoringSSL, where this code was originally
written. However, our code evolved since then, and the commit message
is not particularly helpful in the code but is rather of historical
interested in BoringSSL only.
This commit improves improves the comments around memory_cleanse()
and gives a better rationale for the method that we use. This commit
touches only comments.
Commit fbf327b13868861c2877c5754caf5a9816f2603c ("Minimal code
changes to allow msvc compilation.") was indeed minimal in terms
of lines touched. But as a result of that minimalism it changed the
logic in memory_cleanse() to first call std::memset() and then
additionally the MSVC-specific SecureZeroMemory() function, and it
also moved a comment to the wrong location.
This commit removes the superfluous call to std::memset() on MSVC
and ensures that the comment is in the right position again.
The implementation we currently use from OpenSSL prevents the compiler from optimizing away clensing operations on blocks of memory that are about to be released, but this protection is not extended to link-time optimization. This commit copies the solution cooked up by Google compiler engineers which uses inline assembly directives to instruct the compiler not to optimize out the call under any circumstances. As the code is in-lined, this has the added advantage of removing one more OpenSSL dependency.
Regarding license compatibility, Google's contributions to BoringSSL library, including this code, is made available under the ISC license, which is MIT compatible.
BoringSSL git commit: ad1907fe73334d6c696c8539646c21b11178f20f
Add confirmations, blockheight, blockindex and blocktime to z_listreceivedbyaddress
Fixes https://github.com/zcash/zcash/issues/3724
1- There was a PR to add confirmations to this call at https://github.com/zcash/zcash/pull/3836
I ported the commit from there and fixed test case by incrementing the confirmations as suggested at: https://github.com/zcash/zcash/pull/3836#issuecomment-499927807
2- Then added `blockheight`, `blockindex` and `blocktime`. To avoid some duplicated code (Sprout/Sapling) created a structure `trxblock`.
3- Original issue requests only time and blockindex however i think height is also important; if `blockindex` is the position of the transaction in the block then you are going to need also `height` to find it.
- hashFinalSaplingRoot is now always set to hashLightClientRoot before
Heartwood activation (regardless of whether or not that is the correct
Sapling root), and set to the Sapling root after Heartwood activation
only for blocks that have been passed to ConnectBlock() at least once.
- hashChainHistoryRoot is now always set "correctly" (either null, or
identical to hashLightClientRoot).
We rely on the fact that block headers are downloaded in order, and we
therefore always know the height of a block header, in order to check
whether Heartwood is active for a particular header.
Compute more structures in CTxMemPool::DynamicMemoryUsage()
Closes https://github.com/zcash/zcash/issues/4224
Added more structures to the computation of the mempool memory size as indicated in the issue. RPC call `getmempoolinfo` is the only place where this is used to get the `usage` result.
Lock with cs_main inside gtests that request chain state
Fix https://github.com/zcash/zcash/issues/4389
- Used the lock from boost tests where `chainActive.Height()` is being called: https://github.com/zcash/zcash/blob/master/src/wallet/test/rpc_wallet_tests.cpp#L1323
- I found no other place in the gtests where `chainActive` is used apart from just the same tests `chainActive.Height()` is called. It seems chain state is only used when we fake mine transactions and these are all inside `test_wallet.cpp`.
I might be missing some other patterns to look at, please let me know if so.
The C++ and Rust Equihash validators are intended to have an identical
set of valid Equihash solutions, so this should merely be an
implementation detail. However, deploying the Rust validator at the same
time as a network upgrade reduces the risk of an unintentional consensus
divergence due to undocumented behaviour in either implementation.
Once Heartwood has activated on mainnet, we can verify that all
pre-Heartwood blocks satisfy the Rust validator, and then remove the C++
validator and make Equihash-checking non-contextual again.
This requires moving CheckEquihashSolution() to
ContextualCheckBlockHeader() for all but the genesis block, which has no
effect on consensus; it just means that an invalid Equihash solution is
rejected slightly later in the block validation process.
Quoting the documentation for `std::vector::operator[]`:
Portable programs should never call this function with an argument
n that is out of range, since this causes undefined behavior.
This test was doing just that: performing checks on a non-existent
second Sapling witness (duplicating the Sprout logic that checked two
notes, one of which was in the wallet). The test was instead reading
arbitrary memory after the witness that did exist; in most cases, this
memory was interpreted as a `boost::none` as expected, but in some cases
the memory was interpreted as a "real" witness.
Closeszcash/zcash#4445.
Co-authored-by: Ying Tong <yingtong@ethereum.org>
Add -lightwalletd experimental option
Similar to `-insightexplorer` but loading less indexes.
After testing and code review this should be able to close https://github.com/zcash/zcash/issues/4326
multiple debug categories documentation
The command line help is not clear on how to execute multiple but specific debug categories. Failed with stuff like `-zcashd -debug=category1, category2`, etc to find how to do it after some time.
The proposed line addition should help with that.
Check failing transparent and JoinSplit signatures against the previous network upgrade
This change improves usability across network upgrades, by informing
users when their new transactions are being created with the consensus
branch ID from the previous epoch.
We only check failing signatures against the previous epoch to minimise
the extra computational load on nodes.
A future refactor is needed to similarly check Sapling signatures.
Part of #4260.
The previous version of full_test_suite.py directly called the test
binary, which was being compiled at the same time as the static library.
However, by passing the --tests argument to cargo, rustc was ignoring
several important release-profile configurations, and was also
attempting to link the test binary, which was breaking cross-compilation
builds.
This commit alters src/Makefile.am to only build the static library, and
leaves test compilation to the test runner itself. This ensures that the
tests are only compiled for native builds, when the tests will be run on
the same platform.
std::vector<T> is guaranteed to store T contiguously. However, there is
no guarantee that sizeof(std::array<unsigned char, N>) == N, which
prevents us from interpreting std::vector<std::array<unsigned char, N>>
as &[[u8; N]] on the Rust side of the FFI.
Instead, we define HistoryEntry as a struct wrapping a C array, which
(as checked by static_assert) contains no padding.
The "finalsaplingroothash" field of the getblocktemplate output is no
longer guaranteed to match the actual Sapling commitment tree root, and
has been deprecated. Users should migrate to "lightclientroothash".
CBlockHeader.hashFinalSaplingRoot has been renamed to hashLightClient.
CBlockIndex now stores:
- hashLightClient as from the block header
- hashFinalSaplingRoot, which is accurate for all blocks prior to
Heartwood activation, and all blocks from Heartwood activation onward
that are connected at some point to the main chain in ConnectBlock().
- hashChainHistoryRoot, which is null prior to Heartwood activation, and
set per ZIP 221 from Heartwood activation.
The new block index fields are only written to disk for client version
2.1.2 and above, which will be the first Heartwood-aware clients (even
if Heartwood doesn't have an activation height).
Explanation: if a signal interrupts certain syscalls such as `open`, `read`, or `write`,
then the library function will by default fail with `errno` `EINTR`. But we almost never
check for `EINTR`, so this is likely to cause spurious errors. We want to restart the syscall
instead, which is what `SA_RESTART` is intended to do. Since our signal handlers (defined
in init.cpp) only set a flag, restarting the syscall is safe and is always the Right Thing.
See <https://www.gnu.org/software/libc/manual/html_node/Flags-for-Sigaction.html> and
<https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html> for
further information.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Add support for Sapling full viewing keys
This PR adds Sapling support to `z_exportviewingkey` and `z_importviewingkey`, and stores imported Sapling viewing keys in the wallet.
Closes#3060.
Additional librustzcash integration
This adds librustzcash tests to the full test suite, and brings in the release profile configurations that are currently present in the librustzcash workspace on the other repository. It's very important that we build librustzcash with panic=abort because otherwise the unwinding panics across FFI boundaries could cause undefined behavior.
Fix race conditions during init
Fix race conditions due to accessing `chainActive.Tip()` during init, and other minor cleanups.
Includes backport of https://github.com/bitcoin/bitcoin/pull/8063 .
Fix typos/minor error in comments, and wrap some lines
This minimizes the diff in the example implementation of funding streams in ZIP 207.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Shows reindex progress in metrics screen
Resolves issue #3813.
The "Downloading blocks" message is changed to "Reindexing blocks" during reindex, after reindex is completed the text is reverted back to the first variant.
Reindex progress is shown as a sum of processed file size (we can't use reindexed block number as a progress because we can't predict how many blocks to process at all, we don't know the size of the block before we process it), the result looks like
```
Reindexing blocks | 22.64 MiB / 336.00 MiB (6%, 13583 blocks)
```
This change improves usability across network upgrades, by informing
users when their new transactions are being created with the consensus
branch ID from the previous epoch.
We only check failing signatures against the previous epoch to minimise
the extra computational load on nodes.
Refactor experimental feature handling
Adds new rpc call `getexperimentalfeatures` and also adds experimental features to `getblockchaininfo` output.
Closes#2671.
Bring the librustzcash crate into this repository
Rust dependencies are now canonically pinned within this repository by
`Cargo.lock`. We continue to use the depends system for vendoring the
dependencies, to ensure our Gitian builds continue to function (which have
no network access at build time, and fetch dependencies separately).
The `--enable-online-rust` configure flag replicates the behaviour of the
`LIBRUSTZCASH_OVERRIDE` environment variable (enabling the build system to
use https://crates.io instead of vendored dependencies).
This pulls in the exact version of `librustzcash` that we currently depend on
(corresponding to the `0.1.0` tag in https://github.com/zcash/librustzcash).
The changes to the crate since then will be pulled in as a separate PR.
Part of zcash/librustzcash#155.
Part of #4230.
Upgrade libsodium to 1.0.18
Includes patches that maintain consensus compatibility with libsodium 1.0.15 for Ed25519 pubkey and signature validation.
Replaces #4239. Closes#2872.
Remove time adjustment; instead warn if peer clocks are too different
The policy is: warn if we have seen at least 8 (TIMEDATA_WARNING_SAMPLES) peer times, in the version messages of the first 20 (TIMEDATA_MAX_SAMPLES) unique (by IP address) peers that connect, that are more than 10 minutes (TIMEDATA_WARNING_THRESHOLD seconds) but less than 10 days (TIMEDATA_IGNORE_THRESHOLD seconds) away from local time.
fixes#4338
This enables IDE integration to work (which requires the Cargo.toml to
be in the repo root).
"make clean" no longer runs "cargo clean", because IDE integrations hold
locks on files within the Rust build directory, and an error inside
"cargo clean" error would prevent "make clean" from completing (and
removing other files).
The --enable-online-rust configure flag replicates the behaviour of the
LIBRUSTZCASH_OVERRIDE environment variable (enabling the build system to
use crates.io instead of vendored dependencies).
The Cargo.lock is updated to account for the Zcash Rust crates no longer
being in the same workspace. Dependencies that were not transitive
dependencies of librustzcash are also removed, but no versions change.
Allow negative heights in RPC calls
For issue https://github.com/zcash/zcash/issues/2197
Currently adding the feature to `getblock` and `getblockhash`. There is another candidate: `getblocksubsidy` however i want to have some review about these 2 first before repeating what could be a bad approach.
Initialize ThreadNotifyWallets before additional blocks are imported
The intention of this PR is to address #4301. It seems `ThreadNotifyWallets` assumes that `chainActive.Tip()` on initialization reflects the wallet's view of the active tip, but prior to `ThreadNotifyWallets`'s initialization the active chain ends up being modified by blocks that are imported from disk, so our local `pindexLastTip` ends up out of sync with the wallet's actual "last" tip.
This PR moves the initialization of `ThreadNotifyWallets` to earlier in the process while passing in the current active tip. This addresses the problem for non-fresh nodes, and then for fresh nodes we have the notification thread wait until the genesis block has been loaded.
Change cm to cmu in sapling
Part of https://github.com/zcash/zcash/issues/3446
For each of the 2 commits, suggested change was made, then fixed build errors until compile. Finally ran bitcoin and gtests, both of them passing.
Upstream PRs relating to strMiscWarning
This pulls in upstream PRs bitcoin/bitcoin#7114 and bitcoin/bitcoin#9236 (non-QT parts).
* Fixesbitcoin/bitcoin#6809 - run-of-the-mill exceptions should not get into `strMiscWarning` (which is reported by `getinfo`).
* Eliminate data races for `strMiscWarning` and `fLargeWork*Found`. This moves all access to these data structures through accessor functions and protects them with a lock.
This moves all access to these datastructures through accessor functions
and protects them with a lock.
Relative to the upstream commit, we also add GetMiscWarning() to make this accessible to tests.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
This is a first step in avoiding racy accesses to strMiscWarning.
By itself this commit causes a link error because global variables relating to alerts
are only accessible in the zcashd server, but util is used in other binaries.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
All of these maps are created from scratch on wallet load, so we can
alter their contents without compatibility concerns. With this change,
we can use the existing maps to implement viewing key support. The
downside is that the wallet will take more space in memory, but that can
easily be improved in future by storing ExtFVK fingerprints in some of
the maps (which would improve memory usage even compared to the original
layout).
z_viewtransaction
This RPC method returns all decryptable information for any transaction in the wallet.
Several values are conditionally included in the output for convenience:
- `recovered`: True if an output is not for a Sapling address in the wallet.
- `memoStr`: The text form of an output's memo, if it is valid UTF-8.
- Values are provided both in decimal currency units, and integer zatoshis.
I'm not adding test vectors for non-canonical pubkeys, as that would
require grinding to find a private key corresponding to one of the 19
pubkeys that can be non-canonical.
Reduce duplication in key_io decode
Attempt to fix https://github.com/zcash/zcash/issues/3344 by adding a new `DecodeAny` template function with arguments that will handle all the cases. Then on each decoding this new function is called with the appropriate arguments resulting in reducing code duplication.
Some of the complexity(`boost::optional`) will be removed if `ViewingKey` is implemented for sapling.
Worst-case block verification benchmarks
The micro-benchmark framework from #3858 is used to measure the time to verify various transaction components. These are leveraged by a script that simulates the verification cost of different compositions of full blocks.
Add z_getnewaddress test
Supercedes #3749. Closes#3687.
It takes the commit from @dagurval where a new test case
`rpc_wallet_z_getnewaddress` is added and it adds a check for too many
arguments.
It uses `CheckRPCThrows()` to test the error message of invalid argument.
However, for too many arguments the full help message is displayed, so I
am just testing there is an runtime exception.
Note that the MTP of a block is the median timestamp of the preceding 11 blocks, i.e. it is
typically (with no or only moderate timestamp manipulation) expected to be 6 block intervals
behind that block's timestamp, which *on average* is 450 seconds behind (after Blossom activation).
So the effective limit on future dating of timestamps is ~82.5 minutes. This makes it
exceptionally unlikely --even taking into account feasible timestamp manipulation of this
and previous blocks-- that the chain will stall because no block is found before the limit.
(This may rely on assumptions that do not hold for testnet.)
If an adversary were to have a sufficient fraction of mining power to engineer this situation
then there would be something seriously wrong, and arguably the chain should stall in that
case, pending manual intervention.
Co-authored-by: Jack Grigg <jack@z.cash>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
The use of virtual on these lines is obsolete.
I ran:
"rm -f ./src/zcash-gtest && rm -f ./src/gtest/*.o && make && ./src/zcash-gtest"
before an after making the change. In both cases:
206 test ran
from
32 cases
and
1 test was DISABLED
This allows for fPIE to be used selectively.
Zcash: Second half of upstream commit 17c4d9d1647bbac4b0557136b1c3d98c951feb79
First half was pulled in as c459de2f03
lastCycles was introduced in 35328187463a7078b4206e394c21d5515929c7de which was merged into master yesterday.
Also initialize beginCycles to zero for consistency and completeness.
std::chrono removes portability issues.
Rather than storing doubles, store the untouched time_points. Then
convert to nanoseconds for display. This allows for maximum precision, while
keeping results comparable between differing hardware/operating systems.
Also, display full nanosecond counts rather than sub-second floats.
We were saving a div by caching the inverse as a float, but this
ended up requiring a int -> float -> int conversion, which takes
almost as much time as the difference between float mul and div.
There are lots of other more pressing issues with the bench
framework which probably require simply removing the adaptive
iteration count stuff anyway.
Avoid static analyzer warnings regarding "Function call argument
is a pointer to uninitialized value" in cases where we are
intentionally using such arguments.
This is achieved by using ...
`f(b.begin(), b.end())` (`std::array<char, N>`)
... instead of ...
`f(b, b + N)` (`char b[N]`)
Rationale:
* Reduce false positives by guiding static analyzers regarding our
intentions.
Before this commit:
```
$ clang-tidy-3.5 -checks=* src/bench/base58.cpp
bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage]
EncodeBase58(b, b + 32);
^
$ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp
bench/verify_script.cpp:59:5: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage]
key.Set(vchKey, vchKey + 32, false);
^
$
```
After this commit:
```
$ clang-tidy-3.5 -checks=* src/bench/base58.cpp
$ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp
$
```
Zcash: Only applied changes to src/bench/base58.cpp
The initialization order of global data structures in different
implementation units is undefined. Making use of this is essentially
gambling on what the linker does, the so-called [Static initialization
order fiasco](https://isocpp.org/wiki/faq/ctors#static-init-order).
In this case it apparently worked on Linux but failed on OpenBSD and
FreeBSD.
To create it on first use, make the registration structure local to
a function.
Fixes#8910.
Make sure that the count is a zero modulo the new mask before
scaling, otherwise the next time until a measure triggers
will take only 1/2 as long as accounted for. This caused
the 'min time' to be potentially off by as much as 100%.
There are only a few uses of `insecure_random` outside the tests.
This PR replaces uses of insecure_random (and its accompanying global
state) in the core code with an FastRandomContext that is automatically
seeded on creation.
This is meant to be used for inner loops. The FastRandomContext
can be in the outer scope, or the class itself, then rand32() is used
inside the loop. Useful e.g. for pushing addresses in CNode or the fee
rounding, or randomization for coin selection.
As a context is created per purpose, thus it gets rid of
cross-thread unprotected shared usage of a single set of globals, this
should also get rid of the potential race conditions.
- I'd say TxMempool::check is not called enough to warrant using a special
fast random context, this is switched to GetRand() (open for
discussion...)
- The use of `insecure_rand` in ConnectThroughProxy has been replaced by
an atomic integer counter. The only goal here is to have a different
credentials pair for each connection to go on a different Tor circuit,
it does not need to be random nor unpredictable.
- To avoid having a FastRandomContext on every CNode, the context is
passed into PushAddress as appropriate.
There remains an insecure_random for test usage in `test_random.h`.
Zcash: Resolved conflicts with the following files
src/addrman.cpp
src/main.cpp
src/net.cpp
src/net.h
src/policy/fees.cpp
src/policy/fees.h
src/random.cpp
src/test/merkle_tests.cpp
src/test/net_tests.cpp
src/test/prevector_tests.cpp
src/test/sighash_tests.cpp
src/test/skiplist_tests.cpp
src/test/test_bitcoin.cpp
src/test/versionbits_tests.cpp
src/wallet/test/crypto_tests.cpp
Previously the benchmark code used an integer division (%) with
a non-constant in the inner-loop. This is quite slow on many
processors, especially ones like ARM that lack a hardware divide.
Even on fairly recent x86_64 like haswell an integer division can
take something like 100 cycles-- making it comparable to the
runtime of siphash.
This change avoids the division by using bitmasking instead. This
was especially easy since the count was only increased by doubling.
This change also restarts the timing when the execution time was
very low this avoids mintimes of zero in cases where one execution
ends up below the timer resolution. It also reduces the impact of
the overhead on the final result.
The formatting of the prints is changed to not use scientific
notation make it more machine readable (in particular, gnuplot
croaks on the non-fixedpoint, and it doesn't sort correctly).
This also hoists out all the floating point divisions out of the
semi-hot path because it was easy to do so.
It might be prudent to break out the critical test into a macro
just to guarantee that it gets inlined. It might also make sense
to just save out the intermediate counts and times and get the
floating point completely out of the timing loop (because e.g.
on hardware without a fast hardware FPU like some ARM it will
still be slow enough to distort the results). I haven't done
either of these in this commit.