From 8821dd06be1f2e63f116ce0054a14f60f7a11dd8 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Mon, 13 Dec 2021 09:07:57 -0500
Subject: [PATCH] don't record allocate_buckets metrics before success
The test demonstrates this now by depending on part of the successful result
from the underlying storage server object.
---
src/_zkapauthorizer/_storage_server.py | 21 ++++++++---
.../tests/test_storage_server.py | 36 +++++++++++++++----
2 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py
index 6d0e314..6145a97 100644
--- a/src/_zkapauthorizer/_storage_server.py
+++ b/src/_zkapauthorizer/_storage_server.py
@@ -292,10 +292,6 @@ class ZKAPAuthorizerStorageServer(Referenceable):
allocated_size,
)
- # XXX Some shares may exist already so those passes aren't necessarily
- # spent...
- self._metric_spending_successes.observe(len(validation.valid))
-
alreadygot, bucketwriters = self._original._allocate_buckets(
storage_index,
renew_secret,
@@ -304,6 +300,22 @@ class ZKAPAuthorizerStorageServer(Referenceable):
allocated_size,
renew_leases=False,
)
+
+ # We just committed to spending some of the presented passes. If
+ # `alreadygot` is not empty then we didn't commit to spending *all* of
+ # them. (Also, we didn't *accept* data for storage yet - but that's a
+ # defect in the spending protocol and metrics can't fix it so just
+ # ignore that for now.)
+ #
+ # This expression mirrors the expression the client uses to determine
+ # how many passes were spent when it processes the result we return to
+ # it.
+ spent_passes = required_passes(
+ self._pass_value,
+ [allocated_size] * len(bucketwriters),
+ )
+ self._metric_spending_successes.observe(spent_passes)
+
# Copy/paste the disconnection handling logic from
# StorageServer.remote_allocate_buckets.
for bw in bucketwriters.values():
@@ -312,6 +324,7 @@ class ZKAPAuthorizerStorageServer(Referenceable):
canary,
disconnect_marker,
)
+
return alreadygot, bucketwriters
def remote_get_buckets(self, storage_index):
diff --git a/src/_zkapauthorizer/tests/test_storage_server.py b/src/_zkapauthorizer/tests/test_storage_server.py
index 6dfbc49..9061e63 100644
--- a/src/_zkapauthorizer/tests/test_storage_server.py
+++ b/src/_zkapauthorizer/tests/test_storage_server.py
@@ -608,18 +608,42 @@ class PassValidationTests(TestCase):
storage_index=storage_indexes(),
renew_secret=lease_renew_secrets(),
cancel_secret=lease_cancel_secrets(),
- sharenums=sharenum_sets(),
+ existing_sharenums=sharenum_sets(),
+ new_sharenums=sharenum_sets(),
size=sizes(),
)
def test_immutable_spending_metrics(
- self, storage_index, renew_secret, cancel_secret, sharenums, size
+ self,
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ existing_sharenums,
+ new_sharenums,
+ size,
):
"""
When ZKAPs are spent to call *allocate_buckets* the number of passes spent
is recorded as a metric.
"""
- num_passes = required_passes(
- self.storage_server._pass_value, [size] * len(sharenums)
+ # maybe create some existing shares that won't need to be paid for by
+ # the subsequent `allocate_buckets` operation - but of which the
+ # client is unaware.
+ write_toy_shares(
+ self.anonymous_storage_server,
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ existing_sharenums,
+ size,
+ LocalReferenceable(None),
+ )
+
+ # The client will present this many passes.
+ num_passes = required_passes(self.pass_value, [size] * len(new_sharenums))
+ # But only this many need to be spent.
+ num_spent_passes = required_passes(
+ self.pass_value,
+ [size] * len(new_sharenums - existing_sharenums),
)
valid_passes = make_passes(
self.signing_key,
@@ -635,14 +659,14 @@ class PassValidationTests(TestCase):
storage_index=storage_index,
renew_secret=renew_secret,
cancel_secret=cancel_secret,
- sharenums=sharenums,
+ sharenums=new_sharenums,
allocated_size=size,
canary=LocalReferenceable(None),
),
)
after_count = read_count(self.storage_server)
- after_bucket = read_bucket(self.storage_server, num_passes)
+ after_bucket = read_bucket(self.storage_server, num_spent_passes)
self.expectThat(
after_count,
--
GitLab