nspcc-dev / neo-go

Go Node and SDK for the Neo blockchain
MIT License
123 stars 79 forks source link

Rework Timestamp attribute of block objects in NeoFS block storage #3654

Open AnnaShaleva opened 2 weeks ago

AnnaShaleva commented 2 weeks ago

Is your feature request related to a problem? Please describe.

Block objects stored in the NeoFS block storage have Timestamp attribute which is currently the block timestamp in milliseconds. This behaviour results in the following:

$ ./bin/neofs-cli object head -r minerva.nspcc.ru:8080 --cid VCVr6RKg1hfJR7v36xvjbaXTncj1waPi94u9EFyEttW --oid 4vMdkXbQEgnSSiEi7xsywwoqpbdS6mgqPFSSouyqXAEj --ttl 1
ID: 4vMdkXbQEgnSSiEi7xsywwoqpbdS6mgqPFSSouyqXAEj
CID: VCVr6RKg1hfJR7v36xvjbaXTncj1waPi94u9EFyEttW
Owner: NhNT6EFqrDu7hAsv2V1J3B7J1DKLvkNGRg
CreatedAt: 1
Size: 697
HomoHash: 13e0148adc711a768d37a14434fac38f48877691f17e4b312b107a4cdbce3427322524d59acd97bcff1a9f554db3348277cff084aa6ff9ab573787bf605b61c2
Checksum: 6ac6d21575d25263b31194f5f81498b9971d1c21367849fcb325d792c8f0452e
Type: REGULAR
Attributes:
  Block=1976
  Primary=2
  Hash=7c27f25d29092d63ad0ec8b0aa284903be12610ac581a90c6ddceb47c514d5c9
  PrevHash=d2d12394af799d97512d715375a72ddea29fb64824797d1e52a04bc672a4e31b
  Timestamp=1628131414239 (53563-06-07 17:10:39 +0300 MSK)
ID signature:
  public key: 02d89e1e06504a2ec3410a9ea801acfa3e45ea5d9ad3fa56ea179638a6dd72b7d2
  signature: 51d71990d5cce28e10d07a2e7c6038da1158e26e7b7d37b6ae78bd5094b88ec1462d17be6c2adb18141f9330b4a6102063ffb0e859fe6c8f9a5f410517959344

Here we see this strange Timestamp=1628131414239 (53563-06-07 17:10:39 +0300 MSK) record because NeoFS API treats Timestamp attribute as Unix time of object creation: https://github.com/nspcc-dev/neofs-api/blob/d7c033317a66aff467728f4ca5a86879edb66e54/object/types.proto#L162.

Describe the solution you'd like

Rename Timestamp to TimestampMilli and don't set Timestamp attribute at all.

Describe alternatives you've considered

Convert Timestamp attribute from milliseconds-precision to Unix timestamp. But the meaning of this field is still not "time of object creation" anyway. It's the time of block creation.

Additional context

Please, vote: 👍 for renaming 👎 for conversion to Unix timestamp

It's a breaking change, hence it must be handled with care because we already have existing block objects uploaded using the old format.

AliceInHunterland commented 2 weeks ago

Rename Timestamp to TimestampMilli, remove Timestamp attribute at all.

We will have more duplicates if the Timestamp won't block creation time. (we won't be afraid only after https://github.com/nspcc-dev/neo-go/issues/3652 )

AnnaShaleva commented 2 weeks ago

We will have more duplicates if the Timestamp won't block creation time

That's why I propose to explicitly remove Timestamp attribute.

we won't be afraid only after https://github.com/nspcc-dev/neo-go/issues/3652

Exactly.

AliceInHunterland commented 2 weeks ago

Why TimestampMilli? Maybe BlockTimestamp or something block related?

AnnaShaleva commented 2 weeks ago

Maybe BlockTimestamp or something block related?

All block object attributes are block-related, hence I consider any Block* prefix redundant.

AliceInHunterland commented 2 weeks ago

All block object attributes are block-related, hence I consider any Block* prefix redundant.

If we rename it, we will have a Timestamp attribute - the default one from NeoFS and our block-related timestamp. In the case Timestamp + TimestampMilli it's not obvious which one block-related. Also instead of Index of the block, we have Block as a block-related attribute to identify it, so the Block* prefix could be helpful.

AnnaShaleva commented 2 weeks ago

the default one from NeoFS

This default is not written in stones, it may easily be removed, it's a matter of an additional method in SDK.

Also instead of Index of the block, we have Block as a block-related attribute to identify it,

Block index is a special one, it combines the block object identifier with the index. We anyway have to include block object identifier into the list of attributes, and it's reused for index as well in order not to waste space for a separate Index attribute.

so the Block* prefix could be helpful.

So I'm still not convinced.