Breaking API serves no purpose other than to be incompatible with older versions and other implementations that do support priority
(cherry picked from commit bitcoin/bitcoin@870824e919)
Zcash:
* Update the release notes.
* Update the `prioritisetransaction.py` RPC test.
* We don't have BIP 68 or replace-by-fee.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
* used "fee" to mean "fee rate", "kB" to mean 1000 bytes, "satoshis"
to mean zatoshis, or that incorrectly used "BTC" in place of "ZEC";
* used obsolete concepts such as "zero fee" or "free transaction"; or
* did not accurately document their applicability.
Uses of "satoshis" as a JSON key are not altered.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
(We express it that way rather than 300 zats/1000 bytes, because the
threshold is always rounded to an integer and then multiplied by 3.)
Bitcoin Core added the concept of "dust" in bitcoin/bitcoin#2577.
At that point the dust threshold was tied to three times the
minRelayTxFee rate, with the motivation that if you'd pay more than
a third of the minimum relay fee to spend something, it should be
considered dust. This was implemented as a standard rule rejecting
dust outputs.
This motivation will not apply after ZIP 317 block construction
is implemented: at that point the ZIP 317 marginal fee will be
5000 zats per logical action, but the dust threshold rate will
still be three times 100 zats per 1000 bytes. Those costs would
only coincide if the marginal size per logical action were
5000/300 * 1000 ~= 16667 bytes, and in practice the marginal size
for any kind of input is much smaller than that.
However, to avoid interoperability problems (older wallets creating
transactions that newer nodes will reject because they view the
outputs as dust), we will have to coordinate any increase in the
dust threshold carefully.
More history: in Zcash the minRelayTxFee rate was 5000 zats/1000 bytes
at launch, changed to 1000 zats/1000 bytes in zcashd v1.0.3 and to
100 zats/1000 bytes in zcashd v1.0.7-1 (#2141). The relaying problem
for shielded transactions (#1969) that prompted the latter change was
fixed more thoroughly by the addition of `CFeeRate::GetFeeForRelay`
in #4916, ensuring that a transaction paying `DEFAULT_FEE` can always
be relayed. At the same time the default fee was set to 1000 zats,
per ZIP 313.
An earlier commit in this PR changed relaying policy to be more strict
about enforcing minRelayTxFee. The commit just before this one also
allowed `-minrelaytxfee=0`, which we are going to use to avoid some test
breakage. But if the dust threshold rate were still set to three times
the minRelayTxFee rate, then setting `-minrelaytxfee=0` would have the
side effect of setting the dust threshold to zero, which is not intended.
Bitcoin Core took a different approach to disentangling the dust
threshold from the relay threshold, adding a `-dustrelayfee` option
(bitcoin/bitcoin#9380). We don't want to do that because it is likely
that we will change the dust policy again, and adding a user-visible
config option might conflict with that. Also, it isn't a good idea for
the dust threshold rate to be configurable per node; it's a standard
rule parameter and should only be changed with network-wide coordination
(if it is increased then wallets have to change before nodes, and vice
versa if it is decreased). So for now we set it to a constant that
matches the behaviour before this PR.
Since we can no longer modify the dust threshold, we remove a check
from transaction_tests.cpp that relied on doing so.
This change also indirectly fixes a false-positive assertion error that
would occur in `SpendableInputs::LimitToAmount` if we allowed the dust
threshold to be zero.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Setting minrelaytxfee to 0 will allow all transactions regardless of fee to enter your mempool until it reaches its size limit.
(cherry picked from commit bitcoin/bitcoin@7d4e9509ad)
Zcash: Removed the following misleading sentence from the upstream
commit message:
> However now that mempool limiting is governed by a separate
> incrementalrelay fee, it is an unnecessary restriction to prevent
> a minrelaytxfee of 0.
We do not have `-incrementalrelayfee`, which was added upstream in
bitcoin/bitcoin#9380 . Still, it was pointless to prevent
`-minrelaytxfee=0`, because we did not prevent `-minrelaytxfee=1`
or other low values, which would be similarly ineffective.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Remove GetPriority and ComputePriority. Remove internal machinery for tracking priority in CTxMemPoolEntry.
(cherry picked from commit bitcoin/bitcoin@359e8a03d1)
Zcash:
* We don't have `src/bench/mempool_eviction.cpp`.
* We don't have `-walletrejectlongchains`.
* Now we can remove `MAX_PRIORITY`.
* Fix a comment in `coins.h` while we're changing code next to it.
* Update the `Mempool/PriorityStatsDoNotCrash` regression test.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
This a breaking API change to the prioritisetransaction RPC call which previously required exactly three arguments and now requires exactly two (hash and feeDelta). The function prioritiseTransaction is also updated.
(cherry picked from commit bitcoin/bitcoin@f9b9371c60)
Zcash: We don't have `LoadMempool` or `DumpMempool`.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
"startingpriority" and "currentpriority" are no longer returned in the JSON information about a mempool entry. This affects getmempoolancestors, getmempooldescendants, getmempooolentry, and getrawmempool.
(cherry picked from commit bitcoin/bitcoin@49be7e1bef)
Zcash:
* Adjusted because we don't have bitcoin/bitcoin@5ec0cde371
* Update Zcash-specific tests.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Remove all coin age priority functionality from unit tests and RPC tests.
(cherry picked from commit bitcoin/bitcoin@0315888d0d)
Zcash:
* We cannot remove the `pool` parameter from the `CTxMemPool` constructor
because we do not have bitcoin/bitcoin#9138. (Backporting that PR is
unnecessary and would be a distraction from the purpose of this one;
the changes made by it are orthgonal.)
* We don't have `prioritise_transaction.py`, `MempoolAncestorIndexingTest`,
`MempoolSizeLimitTest`, or the `estimateSmartFee` functionality, so omit
the changes for those.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
The prioritisetransaction API can always be used if a transaction needs to be submitted that bypasses minRelayTxFee.
(cherry picked from commit bitcoin/bitcoin@ad727f4eaf)
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Unused everywhere now except one test.
(cherry picked from commit bitcoin/bitcoin@fe282acd76)
Zcash: We don't have `estimateSmartPriority` which was also removed in
the upstream commit. We have to keep `MAX_PRIORITY` for a little longer
because `CCoinsViewCache::GetPriority` still uses it.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
-printpriority output is now changed to only show the fee rate and hash of transactions included in a block by the mining code.
(cherry picked from commit bitcoin/bitcoin@400b15147c)
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Remove ability of mining code to fill part of a block with transactions sorted by coin age.
(cherry picked from commit bitcoin/bitcoin@272b25a6a9)
Zcash:
* Add release notes.
* Remove a static assertion that no longer applies.
* Spell "prioritise" and "prioritisation" (when referring to tx prioritisation) consistently
with 'ise', to match the "prioritisetransaction" RPC call.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
The RPC call was already deprecated.
(cherry picked from commit bitcoin/bitcoin@12839cdd56)
Zcash: we do not have `estimatesmartpriority`.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
This removes the option from the wallet to not pay a fee on "small"
transactions which spend "old" inputs.
This code is no longer worth keeping around, as almost all miners
prefer not to include transactions which pay no fee at all.
(cherry picked from commit bitcoin/bitcoin@ddf58c7573)
Zcash:
* Add release notes.
* Use `UIError` instead of `InitWarning` for the warning that
`-sendfreetransactions` is no longer supported (because we haven't
refactored `InitWarning` to be accessible from the wallet code).
* Update the `show_help` RPC test to remove `-sendfreetransactions`.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
0bd581a add release notes for removal of priority estimation (Alex Morcos)
b2322e0 Remove priority estimation (Alex Morcos)
(cherry picked from commit bitcoin/bitcoin@3c03dc2cfc)
Zcash:
* MAX_PRIORITY is still needed because it's used for the priority of
shielded transactions.
* Changes relating to "smart priority" are omitted since we do not have
that.
* Change the new minimum client version for `fee_estimates.dat` to
FEE_ESTIMATES_WITHOUT_PRIORITY_VERSION == 5050000. Immediately make
sure we write at least that version and accept it for reading.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
One test in AcceptToMemoryPool was to compare a transaction's fee
agains the value returned by GetMinRelayFee. This value was zero for
all small transactions. For larger transactions (between
DEFAULT_BLOCK_PRIORITY_SIZE and MAX_STANDARD_TX_SIZE), this function
was preventing low fee transactions from ever being accepted.
With this function removed, we will now allow transactions in that range
with fees (including modifications via PrioritiseTransaction) below
the minRelayTxFee, provided that they have sufficient priority.
(cherry picked from commit bitcoin/bitcoin@901b01d674)
Zcash: Take the upstream commit message (in particular the references to
priority) with a pinch of salt, since we're going to be removing priority
in the backport of bitcoin/bitcoin#9602 . The main reason to include this
commit is that it removes `GetMinRelayFee`, which is assumed to have
happened before bitcoin/bitcoin#9602 .
Note that Zcash doesn't have upstream's `-maxmempool` parameter; it
uses ZIP 401 for mempool size limiting instead.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
(cherry picked from commit bitcoin/bitcoin@27fae3484c)
Zcash: The change to tests is not included because we do not have
`qa/rpc-tests/prioritize_transaction.py` (`prioritisetransaction.py` is
very different). There is no point adding a test for this when it's
immediately going to be broken by the removal of priority and "free"
transactions. It will be up to the PRs that add the ZIP 317 changes to
make sure that fee deltas are taken into account for transactions
entering the mempool.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
Redo the feerate index to be based on mining score, rather than fee.
Update mempool_packages.py to test prioritisetransaction's effect on
package scores.
(cherry picked from commit bitcoin/bitcoin@eb306664e7)
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
fixes#6518
In a ZIP sync meeting we decided that:
* The minimum cost should be changed to 10000, in order to avoid
penalizing Orchard-using transactions too much relative to other
transactions.
* `low_fee_penalty` should be changed to 40000. This preserves the
property that a transaction paying less than the ZIP 317 conventional
fee is deprioritized relative to a min-cost, conventional-fee
transaction by a factor of 5, as in the original design.
* The recommended default for `mempooltxcostlimit` should remain at
80000000. Rationale: 80000000 was chosen so that the worst-case size
of the mempool would be equal to the worst-case size of 40 blocks,
which is the current default transaction expiry delta. That reasoning
still holds even with the above changes.
* `eviction_memory_entries` remains at 40000. It could have been lowered
given that there will now be at most 80000000/10000 = 8000 transactions
"in-flight", but it doesn't need to be because the rationale that
"40000 [RecentlyEvicted queue] entries can be stored in ~1.6 MB,
which is small compared to other node memory usage" still holds.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
When a `zcash-cli` command fails, it attempts to print the help message for the command. However,
making the `help` call can also fail, and there was a bug in this check, so that we tried to display
the help message when the `help` call failed, and tried to display the error when the `help` call
succeeded – both leading to an assertion failure.
This also makes some minor changes to the output formatting.
Fixes#6561
Greg Pfeil
Kris Nuttycombe
Luke Dashjr
Daira Emma Hopwood
Alex Morcos
MarcoFalke
Wladimir J. van der Laan
Suhas Daftuar
Suhas Daftuar
str4d