lidofinance / lido-keys-api

Api for fetching node operators keys from modules
MIT License
18 stars 15 forks source link

feat: sanity checker for network config #279

Open AlexanderLukin opened 3 months ago

AlexanderLukin commented 3 months ago

Implement a sanity checker that checks that the information stored in the DB corresponds the chain for which the app is run. If the app is run for one chain, and the DB contains data for another chain, the sanity checker prevents the app from start and throws the error.

Amuhar commented 2 months ago

I think this solution is mostly good and valid. The code is well-structured and isolated. Changes in the sanity checker will not influence other parts of the code. It contains a lot of checks, but they are straightforward.

It looks valid to check for empty keys, operators, and module tables. Currently, we write data in the table in one transaction, but in future implementations, this can be changed, and checking only the ElMeta table will not be sufficient.

It is important to note that any of the tables can't be empty on Holesky or mainnet. But if we run Lido from scratch on a new network or on devnet, in the beginning, the keys API may have data only in the ElMeta table. In this case, if we stop the Keys API and run it again, the first check of the sanity checker will not be correct as it doesn't check the ElMeta table. It is proposed to add the ElMeta check too, or prioritize the AppInfo values check (will it be valid?). If we prioritize the AppInfo table check during the second run, we can check that the chainId and locator address were not changed.

Is it possible to have an empty AppInfo table and empty keys, operators, modules tables, and a non-empty ElMeta? Only if we run an old release for new Lido deployment.

Also, we need to pay attention to the test coverage of the sanity checker. Are all possible cases well covered?

Why do we need such a difficult solution? If the database already has data, we somehow need to check if it is consistent with the chain we want to run the Keys API. For this case, we check the moduleAddress in the 'key' table. Also, this solution checks consistency for the case where the database was partially cleaned accidentally.

AlexanderLukin commented 2 months ago

Hey-hey! Thank you for your strong attention to this sanity checker and such a detailed and comprehensive review! I believe it's very important for the good product quality.

I agree that in case of the scratch deployment of the Lido protocol on a devnet, it is possible that the keys, operators, and modules table might be empty, but the ElMeta table might not. In this case, the sanity checker may not work correctly and this case should be covered.

I also agree with your point regarding the test coverage. Currently, all cases of the sanity checker code are covered by unit tests. But new implementation with the additional ElMeta emptiness checking must also be covered by new tests.

Also, this solution checks consistency for the case where the database was partially cleaned accidentally.

Just to be clear here. This sanity checker is not dedicated to handling cases when the DB is corrupted by side interventions (i. e. when other applications and processes, not Keys API, corrupt data in the DB somehow). You're correct that the sanity checker in its current form is able to prevent some cases of such corruption, but there are way more edge cases, that are not covered (and the code is not designed to cover them).