search

NethermindEth / near-sffl

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

Validate params in RPC server #268

Closed emlautarom1 closed 3 months ago

emlautarom1 commented 3 months ago

Current Behavior

The current RPC server does not validate the parameters on each method, opening up the possibility of errors due to nil dereference panics or unsatisfied invariants.

New Behavior

Each method on the RPC server validates the parameters returning an error in case of failure before doing any actual work.

Breaking Changes

There should not be any breaking changes.

Observations

Currently we are only checking for hidden nil values.

emlautarom1 commented 3 months ago

@Hyodar added range check to RPC

Hyodar commented 3 months ago

Int test failed with the timestamp range check because the first checkpoint is actually 0 -> current. We could change this without much of an issue so the first checkpoint is toTimestamp - interval -> toTimestamp (feel free to do that), but this reminds me we still then need to chunk GetAggregatedCheckpointMessages calls on the operators if this is ever needed.

emlautarom1 commented 3 months ago

We could change this without much of an issue so the first checkpoint is toTimestamp - interval -> toTimestamp

Could you point me out to where this would need to be changed?

Hyodar commented 3 months ago

Could you point me out to where this would need to be changed?

In sendNewCheckpointTask, you can first get the toTimestamp and, if the last timestamp is 0, just do fromTimestamp = toTimestamp - agg.checkpointInterval or such.

emlautarom1 commented 3 months ago

There still seems to be an issue here. I've changed the sendNewCheckpointTask interval as follows:

    toTimestamp := block.Time()
    fromTimestamp := lastCheckpointToTimestamp + 1
    if lastCheckpointToTimestamp == 0 {
        fromTimestamp = toTimestamp - uint64(agg.checkpointInterval)
    }

yet the integration test continues to fail.

Hyodar commented 3 months ago

I think you need to get the seconds only, seems to be underflowing. Here's the error:

{"level":"info","ts":1720032516.9505258,"caller":"aggregator/aggregator.go:397","msg":"Aggregator sending new task","fromTimestamp":18446744035429584175,"toTimestamp":1720032559}
{"level":"error","ts":1720032516.9521868,"caller":"chainio/avs_writer.go:88","msg":"Error assembling CreateCheckpointTask tx","stacktrace":"github.com/NethermindEth/near-sffl/core/chainio.(*AvsWriter).SendNewCheckpointTask\n\t/home/runner/work/near-sffl/near-sffl/core/chainio/avs_writer.go:88\ngithub.com/NethermindEth/near-sffl/aggregator.(*Aggregator).sendNewCheckpointTask\n\t/home/runner/work/near-sffl/near-sffl/aggregator/aggregator.go:399"}
{"level":"error","ts":1720032516.952221,"caller":"aggregator/aggregator.go:401","msg":"Aggregator failed to send checkpoint","err":"execution reverted: revert: fromTimestamp greater than toTimestamp","stacktrace":"github.com/NethermindEth/near-sffl/aggregator.(*Aggregator).sendNewCheckpointTask\n\t/home/runner/work/near-sffl/near-sffl/aggregator/aggregator.go:401"}
emlautarom1 commented 3 months ago

I think you need to get the seconds only, seems to be underflowing

Good catch, this is not the first time that I've had the same issue.