Add zcash.mempool.size.transactions and zcash.mempool.size.bytes metrics

[conradoplg]

Motivation

We need metrics for the mempool per #2626

Specifications

Designs

Solution

Add zcash.mempool.size.transactions and zcash.mempool.size.bytes metrics (see #2626 )

It's part of #2626, but does not closes it.

Review

Just a draft to show the current work, and a question: is it worthwhile to track transactions_serialized_size in this slightly error-prone way to avoid iterating the entire list to compute the total size every time?

#2780 uses the total size of transactions of the mempool which is tracked by the code in this PR, so @dconnolly might want to review this soon.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

Remaining metrics will be done in a separate PR.

[teor2345]

is it worthwhile to track transactions_serialized_size in this slightly error-prone way to avoid iterating the entire list to compute the total size every time?

The code in this PR seems like it will always give the correct mempool serialized size.

But there's some risk that we'll refactor the code, and forget to add or remove the size when changing the mempool transactions. But I think the VerifiedSet is small enough that this is ok for now.

(If we ever have a bug, we can wrap the set in a type that updates the size and metrics when the set changes.)

[teor2345]

@dconnolly I just noticed that this calculation is very similar to the eviction weight from ZIP-401 in #2780.
(But it's not the same, because small transactions are given the minimum weight.)

So we might want to merge this PR pretty quickly to avoid conflicts.

[conradoplg]

@dconnolly I just noticed that this calculation is very similar to the eviction weight from ZIP-401 in #2780. (But it's not the same, because small transactions are given the minimum weight.)

So we might want to merge this PR pretty quickly to avoid conflicts.

I've moved this PR out of draft and will keep working on the remaining metrics in a separate PR to make that easier.

[dconnolly]

Is it effective to, instead of tracking a separate size value from the source of truth, the transactions in the verified set itself, iterate over all the tx's in the verified set, and add up their sizes? Looks like they are pre-calculated on each UnminedTx already (which is not my favorite for the same reasons but it exists now so). There can be a method size() on VerifiedSet that is then called by the update_metrics() method.

[conradoplg]

Is it effective to, instead of tracking a separate size value from the source of truth, the transactions in the verified set itself, iterate over all the tx's in the verified set, and add up their sizes? Looks like they are pre-calculated on each UnminedTx already (which is not my favorite for the same reasons but it exists now so). There can be a method size() on VerifiedSet that is then called by the update_metrics() method.

That's the alternative, but a bit inefficient. I don't really know if that's a problem or not. It could mean iterating ~20K times (per ZIP-401 limits) each time a transaction is inserted or removed in the mempool which seems bad, on the other hand the mempool should be usually much smaller.

[dconnolly]

Is it effective to, instead of tracking a separate size value from the source of truth, the transactions in the verified set itself, iterate over all the tx's in the verified set, and add up their sizes? Looks like they are pre-calculated on each UnminedTx already (which is not my favorite for the same reasons but it exists now so). There can be a method size() on VerifiedSet that is then called by the update_metrics() method.

That's the alternative, but a bit inefficient. I don't really know if that's a problem or not. It could mean iterating ~20K times (per ZIP-401 limits) each time a transaction is inserted or removed in the mempool which seems bad, on the other hand the mempool should be usually much smaller.

Yeah if Zcash average mempool were / gets larger, it would be inefficient, but it seems like our mainnet mempool has single-digit counts of transactions most of the time, maybe we can get away with this for the time being and not have to worry about a refactor misaligning these tracker values

[conradoplg]

I've also just learned that increment_gauge (and decrement) exists in more recent metrics so we can use that in this PR and not keep track of the total size, but that will have to be done in #2780 anyway. But we'd need to update metrics. Let me know what's better.

[dconnolly]

increment_gauge

Ah careful, we are currently using a fork of metrics because we need to stay on tokio 0.3 for now; yet another reason to upgrade to 1.0 tokio

[conradoplg]

I've implemented the on-the-fly computation in 4402cda

Feel free to tinker with the PR (i.e. revert that commit if the old approach ends up being chosen) to speed things up during the next hours, I don't want to block further work that depends on this 😁

[dconnolly]

Awesome 😁

[teor2345]

Is it effective to, instead of tracking a separate size value from the source of truth, the transactions in the verified set itself, iterate over all the tx's in the verified set, and add up their sizes? Looks like they are pre-calculated on each UnminedTx already (which is not my favorite for the same reasons but it exists now so). There can be a method size() on VerifiedSet that is then called by the update_metrics() method.

That's the alternative, but a bit inefficient. I don't really know if that's a problem or not. It could mean iterating ~20K times (per ZIP-401 limits) each time a transaction is inserted or removed in the mempool which seems bad, on the other hand the mempool should be usually much smaller.

Yeah if Zcash average mempool were / gets larger, it would be inefficient, but it seems like our mainnet mempool has single-digit counts of transactions most of the time, maybe we can get away with this for the time being and not have to worry about a refactor misaligning these tracker values

ZIP-401 is designed for denial of service prevention.
And performance really matters during a denial of service attack.
So we have to code ZIP-401 for the maximum mempool size, rather than the typical current mempool size.

If we want a more reliable API, we could wrap the set in a type that updates the size and metrics when the set changes.
(VerifiedSet already does this, but it's a bit more than a simple wrapper.)