api3dao / airseeker

A service powering the dAPIs
MIT License
2 stars 5 forks source link

Update individual Beacons if they deviate too much, independent from the aggregation #329

Closed bbenligiray closed 1 month ago

bbenligiray commented 2 months ago

Api3ServerV1 expects individual Beacons comprising Beacon sets to be kept up to date independent from the aggregation. For example, imagine three Beacons with the following signed data

t=0 100, 100, 100
t=1 100, 50, 100
t=2 100, 100, 100

Here, the aggregation of the Beacon sets doesn't change, so Airseeker will not update. However, this causes the data point 50 to remain usable in any following update, which is not good. Instead, Airseeker should update the second Beacon from 100 to 50 (at t=1), back to 100 again (at t=2), which renders the 50 unusable in the following update.

Few points:

Siegrift commented 2 months ago

However, this causes the data point 50 to remain usable in any following update, which is not good.

But Airseeker won't change the value from 100 to 50. So this value will never end up on-chain.

We do some redundant Beacon updates in return for resiliency against unavailability

Currently, we update all beacons when the data feed exceeds the deviations and also when we do heartbeat updates. I see that what you suggest is deviation based update, but wonder if this time-based guarantee isn't enough.

Airseeker should update the second Beacon from 100 to 50 (at t=1), back to 100 again (at t=2), which renders the 50 unusable in the following update.

It's not clear to me how this should work in practice. Currently, we update all beacons if the aggregation causes the feed change. The problem is that changing only some of the beacons might, change the feed value. Imagine these beacon values and config treshold set to 10%:

t0: 110, 120, 130 , 140, 150
t1: 110, 130, 130, 130, 120

The first and third beacon values are the same. Second, fourth and fifth have deviation ~8.3%, ~7.2% and 20%. Based on the config, we should only update the fifth beacon, but by doing that we end up with beacon values 110, 120, 120, 130, 140 which changes the aggregated value (and anyone can call updateBeaconSetWithBeacons).

To prevent the scenario above, I think we need to do the following:

  1. Check the feed update conditions. If we need to do an update, update all beacons. Otherwise:
  2. Check each beacon deviation based on the config. Determine, if updating only these beacons changes the feed value. If yes - update all beacons. Otherwise only update the beacons with deviation.
bbenligiray commented 2 months ago

But Airseeker won't change the value from 100 to 50. So this value will never end up on-chain.

That's not good enough, someone else can still use it

Currently, we update all beacons when the data feed exceeds the deviations and also when we do heartbeat updates. I see that what you suggest is deviation based update, but wonder if this time-based guarantee isn't enough.

It's not

by doing that we end up with beacon values 110, 120, 120, 130, 140 which changes the aggregated value (and anyone can call updateBeaconSetWithBeacons).

Anyone can already only update that individual Beacon and update the Beacon set based on that. This is not an argument against this change.

Siegrift commented 2 months ago

Anyone can already only update that individual Beacon and update the Beacon set based on that. This is not an argument against this change.

Right. I forgot the assumption that Signed API is public.

I think we need to do the following:

I still think the implementation should work like that though.

bbenligiray commented 2 months ago

I still think the implementation should work like that though.

I think that also implies that whenever we can update Beacon sets without updating Beacons we should. Yet that is bad because it allows a third-party to do push minute changes to the median value to force us to update the Beacon set, which would drain our wallet. If we're not going to do this (for this reason), then neither should we care about updating the Beacon set within the context of this issue.

bbenligiray commented 1 month ago

@bdrhn9 is there an issue tracking when this goes live in production?

bdrhn9 commented 1 month ago

@bdrhn9 is there an issue tracking when this goes live in production?

No. I created https://github.com/api3dao/data-feeds/issues/764. I'm making deployments according to that now.

bdrhn9 commented 1 month ago

@Siegrift when you feel free, can you make release v3.4.0?

Siegrift commented 1 month ago

Done.