Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider replacing mempool transactions based on the transaction fee #2781

Closed
jvff opened this issue Sep 21, 2021 · 7 comments
Closed

Consider replacing mempool transactions based on the transaction fee #2781

jvff opened this issue Sep 21, 2021 · 7 comments
Labels
C-enhancement Category: This is an improvement S-needs-investigation Status: Needs further investigation

Comments

@jvff
Copy link
Contributor

jvff commented Sep 21, 2021

Motivation

As of #2765, mempool transactions are rejected if they spend a UTXO that's already spent or a nullifier that's already revealed by another transaction in the mempool. However, this prevents a user from replacing a transaction with another one by setting a higher transaction fee. We should consider adding support for replacing a transaction based on the transaction fee.

There might be more than one conflict when adding a transaction, so a more complete version might be to build two sets of transactions and then select the set that has a higher overall fee (or the set that provides a larger profit to a miner).

Specifications

Zcash implemented ZIP-203 (transaction expiry heights) instead of fee replacement.

So we don't need to implement this ticket, and we might decide not to do it at all.
(zcashd decided not to do it.)

Designs

Related Work

@jvff jvff added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Sep 21, 2021
@jvff jvff added this to the 2021 Sprint 19 milestone Sep 21, 2021
@str4d
Copy link
Contributor

str4d commented Sep 21, 2021

We considered this for zcashd (RBF was one of the potential backports from upstream Bitcoin Core), and decided it was too complex and ad-hoc a mechanism for the benefits it claimed to provide. We instead deployed ZIP 203 with Overwinter which added a transaction expiry field, to give transaction creators explicit consensus control over whether a transaction should be allowed to persist indefinitely in the mempool or not. See zcash/zcash#754 for the associated discussion. With the defaults recommended in ZIP 203 (which thus far, all shielded transactions have followed), transactions cannot become stuck in the mempool.

@dconnolly
Copy link
Contributor

@daira
Copy link
Contributor

daira commented Sep 21, 2021

Note however that expiry is optional (the transaction creator can set nExpiryHeight to 0, or with almost the same effect, to a block height far in the future). If ZIP 401 is implemented then newer transactions will tend to displace older ones when the mempool becomes "full". Also, the mempool is cleared on a network upgrade.

(Previously, zcashd would also clear the mempool on a restart, but I think that is no longer the case.)

@dconnolly
Copy link
Contributor

(Previously, zcashd would also clear the mempool on a restart, but I think that is no longer the case.)

No longer? That's our current plan (rather, cavalierly blow away mempool state on shutdown/restart, on network upgrade, if we need to catch up with chain tip, etc)

@daira
Copy link
Contributor

daira commented Sep 21, 2021

Sorry I'm mistaken, zcashd does still clear the mempool on restart. I was thinking of something else (persisting the banlist, I think).

@teor2345
Copy link
Contributor

(Previously, zcashd would also clear the mempool on a restart, but I think that is no longer the case.)

No longer? That's our current plan (rather, cavalierly blow away mempool state on shutdown/restart, on network upgrade, if we need to catch up with chain tip, etc)

Zebra can afford to blow away the mempool because it eagerly crawls peer mempools, it shares newly verified transactions with peers, and it's connected to lots of peers.

So there's very little risk of transaction loss.

@teor2345 teor2345 added P-Optional S-needs-investigation Status: Needs further investigation labels Sep 21, 2021
@teor2345 teor2345 removed this from the 2021 Sprint 19 milestone Sep 21, 2021
@teor2345
Copy link
Contributor

I've marked this ticket as optional, based on zcashd engineer feedback.

@jvff jvff closed this as completed Mar 18, 2022
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement S-needs-investigation Status: Needs further investigation
Projects
None yet
Development

No branches or pull requests

6 participants