From 0689391f3a3023921903cba044f81ae7f9ffeb02 Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Tue, 2 Jun 2020 09:06:20 -0400
Subject: [PATCH] Client-side, don't spend passes for already-uploaded data.
---
src/_zkapauthorizer/_storage_client.py | 68 +++++++++++++-
src/_zkapauthorizer/_storage_server.py | 21 +++++
src/_zkapauthorizer/tests/storage_common.py | 7 ++
src/_zkapauthorizer/tests/test_plugin.py | 48 +++-------
.../tests/test_storage_protocol.py | 93 +++++++++++++++++++
5 files changed, 200 insertions(+), 37 deletions(-)
diff --git a/src/_zkapauthorizer/_storage_client.py b/src/_zkapauthorizer/_storage_client.py
index 8566b24..0594913 100644
--- a/src/_zkapauthorizer/_storage_client.py
+++ b/src/_zkapauthorizer/_storage_client.py
@@ -124,7 +124,7 @@ def invalidate_rejected_passes(passes, more_passes_required):
@inline_callbacks
-def call_with_passes(method, num_passes, get_passes):
+def call_with_passes_with_manual_spend(method, num_passes, get_passes, on_success):
"""
Call a method, passing the requested number of passes as the first
argument, and try again if the call fails with an error related to some of
@@ -142,6 +142,16 @@ def call_with_passes(method, num_passes, get_passes):
:param (int -> IPassGroup) get_passes: A function for getting
passes.
+ :param (object -> IPassGroup -> None) on_success: A function to call when
+ ``method`` succeeds. The first argument is the result of ``method``.
+ The second argument is the ``IPassGroup`` used with the successful
+ call. The intended purpose of this hook is to mark as spent passes in
+ the group which the method has spent. This is useful if the result of
+ ``method`` can be used to determine the operation had a lower cost
+ than the worst-case expected from its inputs.
+
+ Spent passes should be marked as spent. All others should be reset.
+
:return: A ``Deferred`` that fires with whatever the ``Deferred`` returned
by ``method`` fires with (apart from ``MorePassesRequired`` failures
that trigger a retry).
@@ -168,8 +178,7 @@ def call_with_passes(method, num_passes, get_passes):
# fail if we don't have enough tokens.
pass_group = pass_group.expand(num_passes - len(pass_group.passes))
else:
- # Commit the spend of the passes when the operation finally succeeds.
- pass_group.mark_spent()
+ on_success(result, pass_group)
break
except:
# Something went wrong that we can't address with a retry.
@@ -180,6 +189,22 @@ def call_with_passes(method, num_passes, get_passes):
returnValue(result)
+def call_with_passes(method, num_passes, get_passes):
+ """
+ Similar to ``call_with_passes_with_manual_spend`` but automatically spend
+ all passes associated with a successful call of ``method``.
+
+ For parameter documentation, see ``call_with_passes_with_manual_spend``.
+ """
+ return call_with_passes_with_manual_spend(
+ method,
+ num_passes,
+ get_passes,
+ # Commit the spend of the passes when the operation finally succeeds.
+ lambda result, pass_group: pass_group.mark_spent(),
+ )
+
+
def with_rref(f):
"""
Decorate a function so that it automatically receives a
@@ -264,6 +289,40 @@ class ZKAPAuthorizerStorageClient(object):
"get_version",
)
+ def _spend_for_allocate_buckets(
+ self,
+ allocated_size,
+ result,
+ pass_group,
+ ):
+ """
+ Spend some subset of a pass group based on the results of an
+ *allocate_buckets* call.
+
+ :param int allocate_buckets: The size of the shares that may have been
+ allocated.
+
+ :param ({int}, {int: IBucketWriter}) result: The result of the remote
+ *allocate_buckets* call.
+
+ :param IPassGroup pass_group: The passes which were used with the
+ remote call. A prefix of the passes in this group will be spent
+ based on the buckets which ``result`` indicates were actually
+ allocated.
+ """
+ alreadygot, bucketwriters = result
+ if alreadygot:
+ # Some passes don't need to be spent because the server already
+ # has some of the share data.
+ actual_size = allocated_size * len(bucketwriters)
+ actual_passes = required_passes(
+ self._pass_value,
+ [actual_size],
+ )
+ to_spend, to_reset = pass_group.split(range(actual_passes))
+ to_spend.mark_spent()
+ to_reset.reset()
+
@with_rref
def allocate_buckets(
self,
@@ -276,7 +335,7 @@ class ZKAPAuthorizerStorageClient(object):
canary,
):
num_passes = required_passes(self._pass_value, [allocated_size] * len(sharenums))
- return call_with_passes(
+ return call_with_passes_with_manual_spend(
lambda passes: rref.callRemote(
"allocate_buckets",
_encode_passes(passes),
@@ -289,6 +348,7 @@ class ZKAPAuthorizerStorageClient(object):
),
num_passes,
partial(self._get_passes, allocate_buckets_message(storage_index).encode("utf-8")),
+ partial(self._spend_for_allocate_buckets, allocated_size),
)
@with_rref
diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py
index bb89df0..e17207a 100644
--- a/src/_zkapauthorizer/_storage_server.py
+++ b/src/_zkapauthorizer/_storage_server.py
@@ -230,6 +230,27 @@ class ZKAPAuthorizerStorageServer(Referenceable):
passes,
self._signing_key,
)
+
+ # Note: The *allocate_buckets* protocol allows for some shares to
+ # already exist on the server. When this is the case, the cost of the
+ # operation is based only on the buckets which are really allocated
+ # here. It's not clear if we can allow the client to supply the
+ # reduced number of passes in the call but we can be sure to only mark
+ # as spent enough passes to cover the allocated buckets. The return
+ # value of the method will tell the client what the true cost was and
+ # they can update their books in the same way.
+ #
+ # "Spending" isn't implemented yet so there is no code here to deal
+ # with this fact (though the client does do the necessary bookkeeping
+ # already). See
+ # https://github.com/PrivateStorageio/ZKAPAuthorizer/issues/41.
+ #
+ # Note: The downside of this scheme is that the client has revealed
+ # some tokens to us. If we act in bad faith we can use this
+ # information to correlate this operation with a future operation
+ # where they are re-spent. We don't do this but it would be better if
+ # we fixed the protocol so it's not even possible. Probably should
+ # file a ticket for this.
check_pass_quantity_for_write(
self._pass_value,
validation,
diff --git a/src/_zkapauthorizer/tests/storage_common.py b/src/_zkapauthorizer/tests/storage_common.py
index ddc58f3..e0e2c1a 100644
--- a/src/_zkapauthorizer/tests/storage_common.py
+++ b/src/_zkapauthorizer/tests/storage_common.py
@@ -271,6 +271,13 @@ class _PassFactory(object):
self.in_use.update(passes)
return PassGroup(message, self, zip(passes, passes))
+ def _clear(self):
+ del self.returned[:]
+ self.in_use.clear()
+ self.invalid.clear()
+ self.spent.clear()
+ self.issued.clear()
+
def _mark_spent(self, passes):
for p in passes:
if p not in self.in_use:
diff --git a/src/_zkapauthorizer/tests/test_plugin.py b/src/_zkapauthorizer/tests/test_plugin.py
index ce04c94..403acd3 100644
--- a/src/_zkapauthorizer/tests/test_plugin.py
+++ b/src/_zkapauthorizer/tests/test_plugin.py
@@ -120,10 +120,6 @@ from ..controller import (
PaymentController,
DummyRedeemer,
)
-from ..storage_common import (
- required_passes,
- allocate_buckets_message,
-)
from .._storage_client import (
IncorrectStorageServerReference,
)
@@ -144,6 +140,7 @@ from .strategies import (
lease_cancel_secrets,
sharenum_sets,
sizes,
+ pass_counts,
)
from .matchers import (
Provides,
@@ -408,11 +405,7 @@ class ClientPluginTests(TestCase):
now=datetimes(),
announcement=announcements(),
voucher=vouchers(),
- storage_index=storage_indexes(),
- renew_secret=lease_renew_secrets(),
- cancel_secret=lease_cancel_secrets(),
- sharenums=sharenum_sets(),
- size=sizes(),
+ num_passes=pass_counts(),
)
@capture_logging(lambda self, logger: logger.validate())
def test_unblinded_tokens_spent(
@@ -422,11 +415,7 @@ class ClientPluginTests(TestCase):
now,
announcement,
voucher,
- storage_index,
- renew_secret,
- cancel_secret,
- sharenums,
- size,
+ num_passes,
):
"""
The ``ZKAPAuthorizerStorageServer`` returned by ``get_storage_client``
@@ -439,16 +428,12 @@ class ClientPluginTests(TestCase):
)
store = VoucherStore.from_node_config(node_config, lambda: now)
- # Give it enough for the allocate_buckets call below.
- expected_pass_cost = required_passes(store.pass_value, [size] * len(sharenums))
- # And few enough redemption groups given the number of tokens.
- num_redemption_groups = expected_pass_cost
controller = PaymentController(
store,
DummyRedeemer(),
- default_token_count=expected_pass_cost,
- num_redemption_groups=num_redemption_groups,
+ default_token_count=num_passes,
+ num_redemption_groups=1,
)
# Get a token inserted into the store.
redeeming = controller.redeem(voucher)
@@ -463,20 +448,17 @@ class ClientPluginTests(TestCase):
get_rref,
)
- # For now, merely making the call spends the passes - regardless of
- # the ultimate success or failure of the operation.
- storage_client.allocate_buckets(
- storage_index,
- renew_secret,
- cancel_secret,
- sharenums,
- size,
- LocalReferenceable(None),
- )
+ # None of the remote methods are implemented by our fake server and I
+ # would like to continue to avoid to have a real server in these
+ # tests, at least until creating a real server doesn't involve so much
+ # complex setup. So avoid using any of the client APIs that make a
+ # remote call ... which is all of them.
+ pass_group = storage_client._get_passes(u"request binding message", num_passes)
+ pass_group.mark_spent()
# There should be no unblinded tokens left to extract.
self.assertThat(
- lambda: store.get_unblinded_tokens(1),
+ lambda: storage_client._get_passes(u"request binding message", 1),
raises(NotEnoughTokens),
)
@@ -489,8 +471,8 @@ class ClientPluginTests(TestCase):
AfterPreprocessing(
lambda logged_message: logged_message.message,
ContainsDict({
- u"message": Equals(allocate_buckets_message(storage_index)),
- u"count": Equals(expected_pass_cost),
+ u"message": Equals(u"request binding message"),
+ u"count": Equals(num_passes),
}),
),
),
diff --git a/src/_zkapauthorizer/tests/test_storage_protocol.py b/src/_zkapauthorizer/tests/test_storage_protocol.py
index b649753..44afe13 100644
--- a/src/_zkapauthorizer/tests/test_storage_protocol.py
+++ b/src/_zkapauthorizer/tests/test_storage_protocol.py
@@ -32,6 +32,7 @@ from testtools.matchers import (
HasLength,
IsInstance,
AfterPreprocessing,
+ MatchesStructure,
raises,
)
from testtools.twistedsupport import (
@@ -353,6 +354,98 @@ class ShareTests(TestCase):
),
)
+ @given(
+ storage_index=storage_indexes(),
+ renew_secret=lease_renew_secrets(),
+ cancel_secret=lease_cancel_secrets(),
+ existing_sharenums=sharenum_sets(),
+ additional_sharenums=sharenum_sets(),
+ size=sizes(),
+ )
+ def test_shares_already_exist(
+ self,
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ existing_sharenums,
+ additional_sharenums,
+ size,
+ ):
+ """
+ When the remote *allocate_buckets* implementation reports that shares
+ already exist, passes are not spent for those shares.
+ """
+ # Hypothesis causes our storage server to be used many times. Clean
+ # up between iterations.
+ cleanup_storage_server(self.anonymous_storage_server)
+
+ # Oops our pass factory, too. :(
+ self.pass_factory._clear()
+
+ # A helper that only varies on sharenums.
+ def allocate_buckets(sharenums):
+ return self.client.allocate_buckets(
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ sharenums,
+ size,
+ canary=self.canary,
+ )
+
+ # Create some shares to alter the behavior of the next
+ # allocate_buckets.
+ write_toy_shares(
+ self.anonymous_storage_server,
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ existing_sharenums,
+ size,
+ canary=self.canary,
+ )
+
+ # Do a partial repeat of the operation. Shuffle around
+ # the shares in some random-ish way. If there is partial overlap
+ # there should be partial spending.
+ all_sharenums = existing_sharenums | additional_sharenums
+ self.assertThat(
+ allocate_buckets(all_sharenums),
+ succeeded(Always()),
+ )
+
+ # This is what the client should try to spend. This should also match
+ # the total number of passes issued during the test.
+ anticipated_passes = required_passes(
+ self.pass_value,
+ [size * len(all_sharenums)],
+ )
+
+ # The number of passes that will *actually* need to be spent depends
+ # on the size and number of shares that really need to be allocated.
+ expected_spent_passes = required_passes(
+ self.pass_value,
+ [size * len(all_sharenums - existing_sharenums)],
+ )
+
+ # The number of passes returned is just the difference between those
+ # two.
+ expected_returned_passes = anticipated_passes - expected_spent_passes
+
+ # Only enough passes for the not-already-uploaded sharenums should
+ # have been spent.
+ self.assertThat(
+ self.pass_factory,
+ MatchesStructure(
+ issued=HasLength(anticipated_passes),
+ spent=HasLength(expected_spent_passes),
+ returned=HasLength(expected_returned_passes),
+
+ in_use=HasLength(0),
+ invalid=HasLength(0),
+ ),
+ )
+
@given(
storage_index=storage_indexes(),
renew_secrets=tuples(lease_renew_secrets(), lease_renew_secrets()),
--
GitLab