babylonchain / babylon-finality-gadget

3 stars 3 forks source link

fix: `QueryIsBlockBabylonFinalized` return `true` when no FP has voting power at a height #38

Closed lesterli closed 4 months ago

lesterli commented 4 months ago

This PR fixes the issue of QueryIsBlockBabylonFinalized, it should return true when no FP has voting power at a height.

bap2pecs commented 4 months ago

any L2 block can be finalised only when all ancestor blocks (since BTC staking activation) are finalised

makes sense. I think the current logic already guarantees that: https://github.com/babylonchain/optimism/blob/feat/babylon-rfc/op-node/rollup/finality/finalizer.go#L235-L249

let's imagine when the derivation pipeline receive block N and it's the first one that has any voting power. then op-node will only mark it as finalized once it has 2/3 votes

now for N+1, since it should have voting power, op-node will require it to also have enough votes to proceed to FCU the "finalized" block head.

but one extreme edge case is all FPs got slashed/unbounded at block N+1 so nobody has voting power. then this can be bypassed. not sure if we should worry about this one

this also implies that under normal cases, if block N+1 is finalized by Etheruem L1 but not having enough voting power, we still mark it as not finalized due to this logic. this ensures the system is secure and there is no way to cheat by sending a forked block to FP network but secretly submitting another block in the L1 batch


I also checked our current implementation of QueryLatestFinalizedBlock() here and it's reading from the configured L2 node RPC

so as long as the FP is connected to a trusted L2 node's RPC, this rule will be respected.

cc @gitferry @SebastianElvis

SebastianElvis commented 4 months ago

makes sense. I think the current logic already guarantees that: babylonchain/optimism@feat/babylon-rfc/op-node/rollup/finality/finalizer.go#L235-L249

I see, OP node basically achieves this by imposing the finality provider quorum requirement on finalised blocks. Do you think this is enough or do we need to impose the consecutiveness of finalised blocks in the Babylon DA SDK as well? The consecutiveness is more like a design needed in BTC staking rather than the OP node that benefits from BTC staking.

Alternatively, if we don't enforce consecutiveness in Babylon DA SDK, maybe we need to avoid mentioning "finalised" in this code base. For example, QueryIsBlockBabylonFinalized can be called QueryBlockQuorum as it only checks whether the block has quorum, which is only one of the requirement to be finalised

bap2pecs commented 4 months ago

Do you think this is enough or do we need to impose the consecutiveness of finalised blocks in the Babylon DA SDK as well

the SDK is a lightweight stateless library so we can't enforce consecutiveness here. from the system security perspective, op nodes don't need it b/c they keep their own states about finalized block heads in op-geth.

but for end users, they can either query finalized block head from a trusted RPC or call QueryBlockQuorum() themselves

@SebastianElvis @gitferry do you think it's enough for users to call QueryBlockQuorum() and fast accept a tx? or should they maintain their own database like op nodes did

bap2pecs commented 4 months ago

resolved Runchao's comment. I also enabled govet linters in this PR and fixed a few to get down the lint errors to 0

SebastianElvis commented 4 months ago

do you think it's enough for users to call QueryBlockQuorum() and fast accept a tx? or should they maintain their own database like op nodes did

If the user relies on a OP node to verify consecutive quorums, does it need to wait for ~5min such that the block can be derived from L1 before being convinced that the block follows consecutive quorums? From the current implementation (i.e., a block is Babylon finalised only when it is derived from L1 and it receives enough finality signatures) this seems to be the case.

If the above is the case, then that means users could not get fast finality from BTC staking. There are 2 directions to fix this

What do you think?

bap2pecs commented 4 months ago

i.e., a block is Babylon finalised only when it is derived from L1 and it receives enough finality signatures

hmm I think that's not true. it doesn't need to be derived from L1. it can be a purely offline block but still considered to be Babylon finalized, as long as there is enough votes and the block follows consecutive quorums

bap2pecs commented 4 months ago

cc @SebastianElvis to follow up on the discussion

SebastianElvis commented 4 months ago

i.e., a block is Babylon finalised only when it is derived from L1 and it receives enough finality signatures

hmm I think that's not true. it doesn't need to be derived from L1. it can be a purely offline block but still considered to be Babylon finalized, as long as there is enough votes and the block follows consecutive quorums

But tryFinalize is triggered only on finalised blocks that are derived from L1, no? https://github.com/babylonchain/optimism/blob/a1dfbac5b1d98a51b707f30013a0077c0b96ff79/op-node/rollup/finality/finalizer.go#L236-L237 so if users rely on OP node for BTC staking finalised blocks the latency will be several minutes

bap2pecs commented 4 months ago

But tryFinalize is triggered only on finalised blocks that are derived from L1, no? https://github.com/babylonchain/optimism/blob/a1dfbac5b1d98a51b707f30013a0077c0b96ff79/op-node/rollup/finality/finalizer.go#L236-L237 so if users rely on OP node for BTC staking finalised blocks the latency will be several minutes

yes it is triggered on L1 finalised signal. but in our system, users won't need to wait for that to accept a tx. I think we discussed that in the past.

the biggest trust assumption is that the users know the block will eventually be finalised by the honest nodes

bap2pecs commented 4 months ago

discussed w @SebastianElvis offline. it's still better to have users track consecutive quorums so they can detect fork early

so we will add a verifier program for that. users will start to track blocks when they first start the verifier. but there is no need to catch up with all the history ones

bap2pecs commented 4 months ago

I just realized an important issue here. imagine all FPs go rogue at a point and got slashed. after that, none of them have any voting power.

in that case we shouldn't mark the block w zero VP finalized. the right thing to do is to let the derivation pipeline stuck at the height.

created https://github.com/babylonchain/babylon-finality-gadget/issues/59

SebastianElvis commented 4 months ago

Hmm good point. In fact this is very possible as there might be only 1 FP operated by the sequencer itself.