Leases for mutable files are handled incorrectly in `slot_testv_and_readv_and_writev`

the writev could indicate that a new sharefile should be created on this server. In this case, the sharefile will not have a lease applied to it, so the file could be collected immediately.

I can't see this. For the starting condition where no share files exist at a storage index and a request that includes writev elements that allocate space:

The client calls ZKAPAuthorizerStorageServer.remote_slot_testv_and_readv_and_writev
ZKAPAuthorizerStorageServer.remote_slot_testv_and_readv_and_writev calls self._slot_testv_and_readv_and_writev
_slot_testv_and_readv_and_writev finds the space-allocating operation with has_writes
_slot_testv_and_readv_and_writev finds no active leases with has_active_leases
renew_leases is set to True and current size to {}
_slot_testv_and_readv_and_writev computes required passes for the new size with get_required_new_passes_for_mutable_write (this might be vulnerable to #222 (closed)) and verifies enough passes have been supplied
_slot_testv_and_readv_and_writev calls StorageServer.slot_testv_and_readv_and_writev and passes True for renew_leases
If testv_is_good then StorageServer.slot_testv_and_readv_and_writev applies the write vectors and calls _add_or_renew_leases because renew_leases is True
_add_or_renew_leases renews the leases on all shares that still exist in the slot

Did I get part of that wrong?

For the starting conditions where some other shares exist I think the outcome is the same for the newly created share file - though it is true that any other existing shares that weren't touched will also have their leases renewed, which is another way for a clever operator to pick up some storage-time for free.

the first share found doesn't have a lease (probably from the first case). In this case, we will treat things as if no sharefiles have a lease[1], so pay for the entire existing size of the file, and extend the lease, losing the value of any existing leases.

I think this is true except that I don't know of a way to end up with share files belonging to the same slot but having different leases. I think that all of the lease manipulations always operate the same way on all leases in a slot (or bucket). Sadly this is hard to prove since the on-disk representation certainly allows for them to differ.

Otherwise, we pay for the difference in size between the current and new sizes, but don't change the lease time, losing the paid value beyond the current lease time.

Because of my understanding of the first point, I think it resolves the other way around (if I'm wrong there I'm probably wrong here). If I pay 1 ZKAP to store 1MiB on Day 0 and then I pay 1 more ZKAP to expand the share to 2 MiB on Day 5 then the 2nd operation extends the lease expiration time to Day (5+31) for the whole share - giving me (2 MiB × 31 days) (or 1 MiB × 62 days) of storage-time for 1 ZKAP while throwing away the value of (1 MiB × (31-5) days) remaining on the existing lease, for a net of (1 MiB × (62-(31-5) days). In other words, I get what I paid for plus the already-used value on the lease being renewed. This makes sense. All of the already-stored data is having its lease expiration time bumped forward by one period for free.

Created by: tomprince

the writev could indicate that a new sharefile should be created on this server. In this case, the sharefile will not have a lease applied to it, so the file could be collected immediately.

I can't see this. For the starting condition where no share files exist at a storage index and a request that includes writev elements that allocate space:
* The client calls `ZKAPAuthorizerStorageServer.remote_slot_testv_and_readv_and_writev`

* `ZKAPAuthorizerStorageServer.remote_slot_testv_and_readv_and_writev` calls `self._slot_testv_and_readv_and_writev`

* `_slot_testv_and_readv_and_writev` finds the space-allocating operation with `has_writes`

* `_slot_testv_and_readv_and_writev` finds no active leases with `has_active_leases`
* [...]

The missing piece here, in my issue description is that leases are per-sharefile, not per-storage index. However, has_active_leases just checks if there is any lease on the first share file it finds. So, has_active_leases can return True, even if one of the writes is to a brand new share file (which is created here. This isn't an issue upstream, since the leases on all share files are unconditionally extended at the end of StorageServer.slot_testv_and_readv_and_writev.

I think the following sequence of tw_vectors demonstrates exhibits the behavior of 1 and 2 (assuming that the os.listdirin StorageServer._get_bucket_shares returns things in sorted order, rather than creation order):

{2: ([], [(0, 'x')], None): This creates a sharefile 2 and is handled properly
{1: ([], [(0, 'x')], None): This creates a sharefile 1 without a lease, since has_active_leases would return True
{2: ([], [(1, 'x')], None): Since has_active_leases returns False here (since sharefile 1 doesn't have a lease), we are charged for the full size of all the share files (i.e. 3) instead of the increase of this write (i.e. 1)

(assume for the example that passes are for 1 B, or the data is correspondingly bigger)

Otherwise, we pay for the difference in size between the current and new sizes, but don't change the lease time, losing the paid value beyond the current lease time.

Because of my understanding of the first point, I think it resolves the other way around (if I'm wrong there I'm probably wrong here). If I pay 1 ZKAP to store 1MiB on Day 0 and then I pay 1 more ZKAP to expand the share to 2 MiB on Day 5 then the 2nd operation extends the lease expiration time to Day (5+31) for the whole share - giving me (2 MiB × 31 days) (or 1 MiB × 62 days) of storage-time for 1 ZKAP while throwing away the value of (1 MiB × (31-5) days) remaining on the existing lease, for a net of (1 MiB × (62-(31-5) days). In other words, I get what I paid for plus the already-used value on the lease being renewed. This makes sense. All of the already-stored data is having its lease expiration time bumped forward by one period for free.

I don't think this is tied to the understanding of the first point. You say "the 2nd operation extends the lease expiration time", however, if has_active_leases returns True, then renew_leases is left as `False, so it would not extend the lease expiration time.

I think the following sequence of tw_vectors demonstrates exhibits the behavior of 1 and 2 (assuming that the os.listdirin StorageServer._get_bucket_shares returns things in sorted order, rather than creation order):

Aha. Okay. This sequence does look to me like it will be handled improperly.

{2: ([], [(0, 'x')], None): This creates a sharefile 2 and is handled properly

To elaborate on "properly" - share file 2 is created with size 1, the cost is 1 ZKAP, and a lease is acquired.

{1: ([], [(0, 'x')], None): This creates a sharefile 1 without a lease, since has_active_leases would return True

Here things begin to go off the rails. Share file 1 is created with size 1, the cost is 1 ZKAP, and (as you say) no lease is acquired. This share should have a lease. The storage-time has been paid for (the 1 ZKAP spent here).

{2: ([], [(1, 'x')], None): Since has_active_leases returns False here (since sharefile 1 doesn't have a lease), we are charged for the full size of all the share files (i.e. 3) instead of the increase of this write (i.e. 1)

If share file 1 had been granted a lease then this would go differently.... but I have a feeling that there are other ways to arrive at either this or a similarly incorrect outcome.

I'm thinking about whether the protocol can be adapted to deal with these per-share issues now...

Leases for mutable files are handled incorrectly in `slot_testv_and_readv_and_writev`

Designs

Child items ...

Activity