decred / dcrtime

Decred anchored timestamp client, proxy, and server.
ISC License
28 stars 23 forks source link

multi: reject digest if exists in previous not anchored dir #68

Closed amass01 closed 4 years ago

amass01 commented 4 years ago

This diff ensures uploaded digest aren't part of previous not anchored yet dir(s) and would reject existing digests.

Before this diff on upload we compared uploaded digests against current timestamp data dir & against global flushed db, and ignored other previous not anchored yet data dirs, this why duplicated digest were accepted.

Also, this commit includes a small fix in flusher cron task, which used to count dirs as flushed even if anchoring failed for any reason, digest would still exists as not anchored but logs would print wrong flushed dirs count.

Closes: #42

amass01 commented 4 years ago

Still broken, but almost there!

amass01 commented 4 years ago

investigation twist: it's not that we mark digests as flushed without broadcasting tx, it's more upload issue, when collecting a digest we check if digest is already flushed and in global db or if it's part of the current not anchored cycle (all cycle's digests saved under one dir). that's not enough, and we should check if it's also part on any previous not anchored yet cycles, and if yes, reject it as duplicate, as it gonna be anchored with next successful anchoring cycle.

Say we have hourly cycles and we anchor on each 10th minute of a given hour:

  1. user submit a digest x at 15:01 => accepted and saved to dcrtimed/data/15:00
  2. user submit same digest x at 15:** => rejected as it part of currently cycle dir
  3. user submits same digest x at 16:01 (note that anchoring will occur for all not flushed dirs at 16:10), backend checks if digest exists in global db(flushed) or if it's in currently cycle data: dcrtimed/data/16:00, doesn't find it as it in the previous not anchored yet dir (15:00), digest x accepted again!!

as i mentioned above we should check if digest is also part of older dirs and if it's there we should reject it and return: digest x exists but not anchored

2020-06-13 16:33:13.953 [INF] DCRT: /v2/timestamp Timestamp [::1]:64631: rejected 20200613.143300 e3d89c3f6ca2b72aac5414b5b1860f7563360d41fbfd01ccd05ca66c68121a97
2020-06-13 16:33:46.731 [INF] DCRT: /v2/timestamp Timestamp [::1]:64648: rejected 20200613.143300 e3d89c3f6ca2b72aac5414b5b1860f7563360d41fbfd01ccd05ca66c68121a97
2020-06-13 16:33:53.255 [INF] DCRT: /v2/timestamp Timestamp [::1]:64653: rejected 20200613.143300 e3d89c3f6ca2b72aac5414b5b1860f7563360d41fbfd01ccd05ca66c68121a97
2020-06-13 16:33:56.090 [INF] DCRT: /v2/timestamp Timestamp [::1]:64658: rejected 20200613.143300 e3d89c3f6ca2b72aac5414b5b1860f7563360d41fbfd01ccd05ca66c68121a97
2020-06-13 16:34:07.743 [INF] DCRT: /v2/timestamp Timestamp [::1]:64666: accepted 20200613.143400 e3d89c3f6ca2b72aac5414b5b1860f7563360d41fbfd01ccd05ca66c68121a97

^ here i have minutes anchoring cycle: and as you can see, same digest accepted twice for different cycles once for 16:33:13 and again for 16:34:07 - next anchoring was suppose to happen on 16:34:10 https://github.com/decred/dcrtime/blob/master/dcrtimed/backend/filesystem/filesystem.go#L708

^ here, for each uploaded hash we: // Lookup in current timestamp database // Lookup in global database if there are dups.

I added another chuck of logic to check if digest being uploaded doesn't exist in previous cycles not anchored yet dirs

amass01 commented 4 years ago

before 68:

anchoring time -> construct transaction for cached records -> anchoring error -> fail silently -> re-stamp old digest -> digest accepted -> anchoring time -> anchoring error -> resolve error -> digest anchored multiple times!

after 68: anchoring time -> construct transaction for cached records -> anchoring error -> fail silently -> re-stamp old digest -> digest rejected (digest found in old dir and not anchored, return exists!) -> anchoring time -> anchoring error -> resolve error -> anchoring time -> digest anchored one time without duplication -> HAPPY END.

marcopeereboom commented 4 years ago

This is an outstanding find. Good job. What is missing in this PR is test code that verifies these claims. Please add unit tests that verify these conditions or we'll continue to chase these forevermore.

amass01 commented 4 years ago

@marcopeereboom Sure thing, adding unit tests. Thanks for the review.

amass01 commented 4 years ago

waiting on: #70 & then #67

amass01 commented 4 years ago

fyi - as test i ran the new unit test against master. Put func returned OK and accepted a duplication and the test failed.