diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py index 6d0e3144e1446a1b1d2711993900609413ef11a7..6145a9767ac6cb4f4e76b0a443573ade930a9fd8 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 6dfbc491fe6132574b820ca4ee5d58a0fffb872d..9061e632f73c116d779d15c71ce083e59bbab887 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,