CalculateMemPoolAncestors was always looping over a transaction's inputs
to find in-mempool parents. When adding a new transaction, this is the
correct behavior, but when removing a transaction, we want to use the
ancestor set that would be calculated by walking mapLinks (which should
in general be the same set, except during a reorg when the mempool is
in an inconsistent state, and the mapLinks-based calculation would be the
correct one).
(cherry picked from commit bitcoin/bitcoin@60de0d5826)
Associate with each CTxMemPoolEntry all the size/fees of descendant
mempool transactions. Sort mempool by max(feerate of entry, feerate
of descendants). Update statistics on-the-fly as transactions enter
or leave the mempool.
Also add ancestor and descendant limiting, so that transactions can
be rejected if the number or size of unconfirmed ancestors exceeds
a target, or if adding a transaction would cause some other mempool
entry to have too many (or too large) a set of unconfirmed in-
mempool descendants.
(cherry picked from commit bitcoin/bitcoin@5add7a74a6)
Zcash:
- Mempool methods were adapted to our mempool changes.
- Default ancestor and descendant size limits were double to account for
our larger block size.
- The mempool_packages RPC test fee was adapted to account for our
emissions curve (which results in a smaller per-block reward that
needs to be split into smaller shards for sequential transactions.
- Includes some modifications to account for us backporting
bitcoin/bitcoin@f3fe83673e early in
zcash/zcash#5269.
This change improves clock management for zcashd by ensuring
that all clock methods (obtaining seconds, milliseconds, and
microseconds since the epoch) agree under testing conditions
using `-mocktime`, and also adds a feature that allows tests
to specify an offset to the system clock; this is useful to
allow comprehensive testing of the "timejacking attack mitigation"
consensus rules.
Now that we call `Consensus::CheckTxShieldedInputs` on all transactions
in `CTxMemPool::check`, we don't need to separately check the nullifiers
and anchors directly. That change also fixed a bug where we weren't
previously making the same assertions about Orchard Actions.
Both transparent and shielded inputs have contextual checks that need to
be enforced in the consensus rules. For shielded inputs, these are that
the anchors in transactions correspond to real commitment tree states
(to ensure that the spent notes existed), and that their nullifiers are
not being double-spent.
When Sprout was first added to the codebase, we added input checks in
the same places that transparent inputs were checked; namely anywhere
`CCoinsViewCache::HaveInputs` is called. These all happened to be gated
on `!tx.IsCoinBase()`, which was fine because we did not allow Sprout
JoinSplits in coinbase transactions (enforced with a non-contextual
check).
When we added Sapling we also allowed coinbase outputs to Sapling
addresses (shielded coinbase). We updated `HaveShieldedRequirements` to
check Sapling anchors and nullifiers, but didn't change the consensus
code to call it on coinbase. This was fine because Sapling Spends and
Outputs are separate, and we did not allow Sapling Spends in coinbase
transactions (meaning that there were no anchors or nullifiers to
enforce the input rules on).
Orchard falls into an interesting middle-ground:
- We allowed coinbase outputs to Orchard addresses, to enable Sapling
shielded coinbase users to migrate to Orchard.
- Orchard uses Actions, which are a hybrid of Sprout JoinSplits and
Sapling Spends/Outputs. That is, an Orchard Action comprises a single
spend and a single output.
To maintain the "no shielded spends in coinbase" rule, we added an
`enableSpends` flag to the Orchard circuit. We force it to be set to
`false` for coinbase, ensuring that all Orchard spends in a coinbase use
dummy (zero-valued) notes. However, this is insufficient: the coinbase
transaction will still contain an Orchard anchor and nullifiers, and
these need to be correctly constrained.
In particular, not constraining the Orchard nullifiers in a coinbase
transaction enables a Faerie Gold attack. We explicitly require that
Orchard nullifiers are unique, so that there is a unique input to the
nullifier derivation. Without the coinbase check, the following attack
is possible:
- An adversary creates an Orchard Action sending some amount of ZEC to a
victim address, with a dummy spent note. The entire transaction can be
fully-shielded by placing the real spent note in a separate Action.
- The adversary uses the exact same dummy note in a coinbase
transaction, creating the exact same output note (same victim address
and amount).
- The victim now has two notes with the same ZEC amount, but can only
spend one of them because they have the same nullifier.
This commit fixes the consensus bug by calling `HaveShieldedRequirements`
outside of `!tx.IsCoinBase()` gates. To simplify its usage, there is now
a `Consensus::CheckTxShieldedInputs` function that handles the logging
and validation state updates. We also move shielded input checks from
`ContextualCheckInputs` to `ContextualCheckShieldedInputs`; these now
mirror each other in that they check contextual rules on transparent and
shielded inputs respectively, followed by checking signatures.
The ability to GETDATA a transaction which has not (yet) been relayed
is a privacy loss vector.
The use of the mempool for this was added as part of the mempool p2p
message and is only needed to fetch transactions returned by it.
(cherry picked from commit bitcoin/bitcoin@7e908c7b82)
Previously Bitcoin would send 1/4 of transactions out to all peers
instantly. This causes high overhead because it makes >80% of
INVs size 1. Doing so harms privacy, because it limits the
amount of source obscurity a transaction can receive.
These randomized broadcasts also disobeyed transaction dependencies
and required use of the orphan pool. Because the orphan pool is
so small this leads to poor propagation for dependent transactions.
When the bypass wasn't in effect, transactions were sent in the
order they were received. This avoided creating orphans but
undermines privacy fairly significantly.
This commit:
Eliminates the bypass. The bypass is replaced by halving the
average delay for outbound peers.
Sorts candidate transactions for INV by their topological
depth then by their feerate (then hash); removing the
information leakage and providing priority service to
higher fee transactions.
Limits the amount of transactions sent in a single INV to
7tx/sec (and twice that for outbound); this limits the
harm of low fee transaction floods, gives faster relay
service to higher fee transactions. The 7 sounds lower
than it really is because received advertisements need
not be sent, and because the aggregate rate is multipled
by the number of peers.
(cherry picked from commit f2d3ba73860e875972738d1da1507124d0971ae5)
Zcash: Candidate transactions for INV are not sorted by their
topological depth because we haven't backported bitcoin/bitcoin#6654.
The score index is meant to represent the order of priority for being included in a block for miners. Initially this is set to the transactions modified (by any feeDelta) fee rate. Index improvements and unit tests by sdaftuar.
(cherry picked from commit f3fe83673e84ef4d20b3026faa397cad17212ff8)
Zcash: Also includes some small refactors from bitcoin/bitcoin#6654 which
we have not backported.
Store sum of legacy and P2SH sig op counts. This is calculated in AcceptToMemory pool and storing it saves redoing the expensive calculation in block template creation.
(cherry picked from commit c49d5bc9e6c97c47c0bd78604b2c393a7e4af097)
Current master crashes on OSX with an exception: "boost: mutex lock failed in pthread_mutex_lock: Invalid argument"
(cherry picked from commit 0d699fc821048ab9316b0004e6552c8f1dc5e5f4)
Zcash: Also adds the `clear` call that this was fixing. Upstream added it
in https://github.com/bitcoin/bitcoin/pull/6722 which we never backported
(instead implementing our own mempool limiting logic).
(cherry picked from commit b7b48c8bbdf7a90861610b035d8b0a247ef78c45)
Zcash: Excluding changes to code we haven't backported yet that cause
too many conflicts.
This is ~1.7x slower than the Lookup3-of-Xor-with-salt construct we were
using before, but it is a primitive designed for exactly this.
Zcash: Propagate CCoinsKeyHasher -> SaltedTxidHasher changes to where
we've used CCoinsKeyHasher outside of its original scope.
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
ThreadNotifyWallets now collects all notification-related state at once,
and then executes wallet logic based on the collected state after
dropping any locks it is holding.
Daira Hopwood
Kris Nuttycombe
Suhas Daftuar
Jim Posen
ebfull
Jack Grigg
therealyingtong
Pieter Wuille
Gregory Maxwell
Alex Morcos
Matt Corallo
Jonas Schnelli
Kris Nuttycombe
Karl-Johan Alm
Jorge Timón
Alfredo Garcia
Homu
Wladimir J. van der Laan
Eirik Ogilvie-Wigley
Larry Ruane
Dimitris Apostolou