Remove expiration concept from data streaming #3680
Conversation
pmikolajczyk41
force-pushed
the
pmikolajczyk/serialize-data-in-data-streaming
branch
from
8ab5c7f to
d11f2e1
Compare
pmikolajczyk41
changed the title
There was a problem hiding this comment.
Regarding the backwards compatibility comments: we have to be really careful with any changes to the existing AnyTrust API (dasRpcServer). Regarding the daprovider API we can play a lot more fast and loose because it's not widely deployed and we are making major changes to it with the custom-da work.
| // lint:require-exhaustive-initialization | ||
| type StoreResult struct { | ||
| DataHash hexutil.Bytes `json:"dataHash,omitempty"` | ||
| Timeout hexutil.Uint64 `json:"timeout,omitempty"` |
There was a problem hiding this comment.
I think this isn't backwards compatible, we might have to leave it called "Timeout" in the Store das RPC API.
| Expiry uint64 | ||
| } | ||
|
|
||
| func (s *DASRPCServer) StartChunkedStore(ctx context.Context, timestamp, nChunks, chunkSize, totalSize hexutil.Uint64, sig hexutil.Bytes) (*data_streaming.StartStreamingResult, error) { |
There was a problem hiding this comment.
I like the overall idea of this change, ie moving extra fields like expiry into the payload, but again I think this isn't backwards compatible for existing anytrust deployments and that's a problem given that client and server will have different upgrade schedules. If we have to keep a vestigial parameter on the streaming interface for now it's not the end of the world for now, then we can do a V2 of the RPC API without it to give a migration path.
| dasWriter DASWriter | ||
| } | ||
|
|
||
| func (d *writerForDAS) Store(ctx context.Context, message []byte, timeout uint64, disableFallbackStoreDataOnChain bool) ([]byte, error) { |
There was a problem hiding this comment.
IIRC this adapts from daprovider.Writer to the dasWriter. I think we should be able to remove the expiry parameter from daprovider.Writer and just have this adapter add the expiry. It is currently calculated as uint64(time.Now().Add(config.DASRetentionPeriod).Unix()) in the batch poster, that could just be moved into this adapter.
|
This one too will cause a lot of conflicts with #3237. It might be better to make these changes against that branch. |
pmikolajczyk41
deleted the
pmikolajczyk/serialize-data-in-data-streaming
branch
2 participants
Data streaming protocol should be used to send pure bytes, without any other parameters. Therefore:
timeoutparameter was removed from the protocol levelThis opens a clear path for e.g. including other data like on-chain storage fallback flag in other usecases.
We also renamed 'timeout' to 'expiry', which is more accurate in the DA world.