babylonlabs-io / finality-provider

Other
6 stars 10 forks source link

SyncFinalityProviderStatus updates status for non-existent Finality Providers #73

Open gusin13 opened 1 month ago

gusin13 commented 1 month ago

Bug Description

SyncFinalityProviderStatus incorrectly updating the status of Finality Providers (FPs) that don't actually exist on the consumer chain.

https://github.com/babylonlabs-io/finality-provider/blob/627a8d0b64817f34789069abb5f34d6d3258afeb/finality-provider/service/app.go#L256

Current Behavior

  1. SyncFinalityProviderStatus queries voting power for all locally stored FPs without checking their existence on the chain.
  2. Some consumer implementations of QueryFinalityProviderVotingPower return a response (with zero power) even for non-existent FPs.
  3. This causes SyncFinalityProviderStatus to update statuses of FPs that aren't actually on the chain.

Expected Behavior

SyncFinalityProviderStatus should only update statuses for FPs that actually exist on the consumer chain.

Root Cause

  1. Lack of existence check in SyncFinalityProviderStatus before querying power.
  2. Inconsistent implementation of QueryFinalityProviderVotingPower across consumers.

Notes

The Babylon contract (used in babylon-sdk) currently returns a zero power response instead of an error for non-existent FPs:

root@2bb4c3b05f8d:/ibcsim-bcd# bcd q wasm contract-state smart bbnc1nc5tatafv6eyq7llkr2gv50ff9e22mnf70qgjlv737ktmt4eswrqgn0kq0 '{"finality_provider_info":{"btc_pk_hex": "018bd93a74d49a28dac942f90716fc715a6ca9dce6fcb527211c735f693fddc8"}}'
data:
  btc_pk_hex: 018bd93a74d49a28dac942f90716fc715a6ca9dce6fcb527211c735f693fddc8
  power: 0

this causes the SyncFinalityProviderStatus to mark the FP as REGISTERED even if it doesn't exist on the consumer chain

This problem doesn't occur for Babylon finality provider as Babylon grpc query returns err if FP doesn't exist

gusin13 commented 1 month ago

Fixed the query in babylon-contract https://github.com/babylonlabs-io/babylon-contract/pull/71

RafilxTenfen commented 1 month ago

Hey @gusin13, the idea of SyncFinalityProviderStatus is to mark it as registered if the FP exists on chain Should we use a query that verifies that the FP exists on the consumer chain?

gusin13 commented 3 weeks ago

@RafilxTenfen yes correct, we can first check if the FP exists on the consumer chain and then query VP.