search

NethermindEth / near-sffl

https://nffl.nethermind.io/
MIT License
6 stars 3 forks source link

Server parameter validation #250

Closed emlautarom1 closed 3 weeks ago

emlautarom1 commented 3 weeks ago

Current Behavior

Partially solves https://github.com/NethermindEth/near-sffl/issues/239 by introducing additional bounds checks to the checkpoint messages fetching. It also adds additional tests for parameter deserializing.

New Behavior

Ensures that ranges from - to are valid, that is from < to is always satisfied.

Breaking Changes

Certain queries that would previously return an empty result set now return an error message indicating that the range provided is invalid.

Observations

There is quite a complicated issue with the storage of BlockNumber and Timestamp as defined here:

https://github.com/NethermindEth/near-sffl/blob/18544a6716cb6fe0cb91f928ba1f476cc7bcaa5f/core/types/fields.go#L14-L15

I'm assuming that this timestamp is representing a Unix timestamp

These types are used in StateRootUpdateMessage

https://github.com/NethermindEth/near-sffl/blob/18544a6716cb6fe0cb91f928ba1f476cc7bcaa5f/core/types/messages/state_root_update.go#L18-L19

This message type then gets converted to a specific model when interacting with the DB:

https://github.com/NethermindEth/near-sffl/blob/18544a6716cb6fe0cb91f928ba1f476cc7bcaa5f/aggregator/database/models/state_root_update.go#L12-L13

As noted, there is a pending range check to perform. SQLite by default does not support uint64 values (and neither other DBs like Postgres), allowing only int64 values. For both fields, if we try to store math.MaxUint64 we'll get the following SQL error:

uint64 values with high bit set are not supported

I'm not 100% sure who is to blame here (gorm, SQLite), but this means that in the future, if we have either a BlockHeight or a Timestamp greater than math.MaxInt64 then we'll get an error.

Possible actions:

  1. Do nothing since we can guess that such high values will not show up in the near future.
  2. Add a check to prevent inserting these values in the DB. This is not that different from 1 but at least we control what to do on errors
  3. Change the DB schema to store these values, ex. store them as text. Curiously we get the same error on BlockHeight despite technically using text as a type which makes things a bit confusing. Either way, changing the schema would mean a breaking change which we want to avoid at least for the time being.
emlautarom1 commented 3 weeks ago

We'll go with option 1. for now (Do nothing since we can guess that such high values will not show up in the near future)