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

ZIP 401 uses serialized size to calculate cost but the zcashd implementation uses RecursiveDynamicUsage #673

Closed
daira opened this issue Feb 7, 2023 · 2 comments · Fixed by #685

Comments

@daira
Copy link
Collaborator

daira commented Feb 7, 2023

I just noticed that our implementation of ZIP 401 (mempool size limiting) uses the RecursiveDynamicUsage of each transaction to calculate its cost, not the serialized size:

WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx, const CAmount& fee)
{
    size_t memUsage = RecursiveDynamicUsage(tx);
    int64_t cost = std::max((int64_t) memUsage, (int64_t) MIN_TX_COST);
    int64_t evictionWeight = cost;
    if (fee < DEFAULT_FEE) {
        evictionWeight += LOW_FEE_PENALTY;
    }
    return WeightedTxInfo(tx.GetHash(), TxWeight(cost, evictionWeight));
}

The ZIP specifies to use the serialized size.

It looks like the implementation has been like that since ZIP 401 was first implemented, and I even made a comment on the relevant line, so I'm surprised that I didn't notice the discrepancy then. The implementation using dynamic usage might be better than what is specified, but if so then we should make an explicit decision about that and update the ZIP accordingly.

@daira
Copy link
Collaborator Author

daira commented Feb 7, 2023

This interacts with #565, because when we chose 4000 (bytes) for the minimum transaction cost for mempool limiting, we were thinking of it as a minimum relative to the serialized size, not relative to the size in memory.

@daira
Copy link
Collaborator Author

daira commented Feb 7, 2023

I also notice that CTxMemPoolEntry uses RecursiveDynamicUsage(*tx) + memusage::DynamicUsage(tx). Should the cost calculation be doing the same? Does DEFAULT_MEMPOOL_TOTAL_COST_LIMIT need to be adjusted?

daira added a commit to daira/zips that referenced this issue Apr 16, 2023
memory size of a transaction than its serialized size.

fixes zcash#673

Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
@daira daira closed this as completed in #685 May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant