I don’t think it’s possible for this error condition to be tested for `z_shieldcoinbase` any more
1. to hit this failure, we need a fee higher than the amount available in coinbase UTXOs;
2. if there are no UTXOs to shield, we fail before reaching an “insufficient funds” check;
3. coinbase UTXOs are >1 ZEC, which is the highest we can set `-maxtxfee` to without emitting a
warning;
4. emitting a warning causes the test to fail.
Previously, `WalletTxBuilder` expected a vector of payments with known amounts.
This meant that operations like `z_shieldcoinbase` and `z_mergetoaddress` needed
to pre-calculate the amount to be sent to the target address.
With ZIP 317 fees, this became harder to do. Now `WalletTxBuilder` accepts
either a vector of payments or a single address (and optional memo) for the net
amount to be paid to.
This also converts `z_shieldcoinbase` to use this approach. `z_mergetoaddress`
conversion is blocked by #6569.
This avoids a race condition where node 1 attempts to fetch the block
while it is considered invalid by node 0, which eventually leads to the
nodes becoming disconnected, preventing the test from advancing.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
There was a race condition here between the block generated by node 0
being broadcast to node 1, and being invalidated by node 0. Due to the
changes we made to the difficulty adjustment algorithm, the invalidated
block and the block replacing it generated by node 0 would have the same
chain work; thus if node 1 observed the invalidated block, it would
never reorg to the replacement block.
We fix this by having node 0 reconsider the invalidated block before
generating its next block. This avoids the chain fork, and also clears
the mempool to provide better separation between test stages.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
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>
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>
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>
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>
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>
Without `AllowRevealedRecipients`, we can’t send transparent change, but previously we asserted if
we couldn’t get a transparent change address. Now it returns a new `TransparentChangeNotAllowed`
failure, which is just a more specific `TransparentRecipientNotAllowed` to avoid confusion when
there are no explicit unshielded recipients.
- still support explicit fixed fees everywhere – if a caller provides a fee, we
don’t adjust it
- treat negative fees as signifier for use of ZIP 317 fees when a fee needs to
be provided because of additional positional arguments
When deciding whether we can pay Orchard receivers, rather than checking
whether we have “sufficient non-Sprout funds”, we can just check whether the
selector selects Sprout – if it doesn’t then we can select Orchard receivers
regardless, and we’ll catch insufficient funds later.
This is also helpful for ZIP 317, because in that case we don’t even know the
total non-Sprout funds necessary until after we decide which receivers to pay.
Do the check deeper, preventing test_bitcoin from being able to bypass it. This
also moves it out of z_sendmany-specific code, which will be helpful when we add
other operations, like sendfromaccount.
Greg Pfeil
Daira Emma Hopwood
str4d
Kris Nuttycombe
Luke Dashjr
Alex Morcos
MarcoFalke
Suhas Daftuar