fix: making zoneconcierge resilient against long BTC reorg (again) #413
Conversation
| if initHeader == nil { | ||
| // if initHeader is nil, then this means a large reorg happens such that all headers | ||
| // in the last segment are reverted. In this case, send the last w+1 BTC headers | ||
| return k.getDeepEnoughBTCHeaders(ctx) |
There was a problem hiding this comment.
does the smart contract do btc fork choice or does it just accepts whatever Babylon will send it ? becouse from what I understand with this apprach we will send competing fork to smart contract ?
Also doesn't this approach fail if there is no checkpoints finalized for over W blocks as then we will have gap in blocks we will send to smart contract ?
My current thinking for proper fix is:
- store last W+1 blocks known by smart contract
- with each finalised checkpoint, find lowest common ancestor between smart contract chain and current babylon tip.
- send to smart contract chain extension from found lowest common ancestor to current babylon tip
- update last W+1 blocks known by smart contract
This way practically do fork choice for smart contract, and even if there is no finalized checkpoints for long time, we can always extend smart contract chain. (as long as there is no rollback larger than W)
There was a problem hiding this comment.
does the smart contract do btc fork choice or does it just accepts whatever Babylon will send it ? becouse from what I understand with this apprach we will send competing fork to smart contract ?
So Babylon contract always follows the fork received from Babylon. This ensures consistency of BTC light client between Babylon and Babylon contract, and is secure as long as Babylon is secure. In the future it will support fork choice in order to allow any user to submit BTC headers to Babylon contract.
Also doesn't this approach fail if there is no checkpoints finalized for over W blocks as then we will have gap in blocks we will send to smart contract ?
No, because as long as one block in lastSentSegment is still canonical, Babylon will send all BTC headers since that canonical block in lastSentSegment, even if the number of BTC headers is larger than w.
Also in the case where Babylon does not have checkpoints over w BTC blocks, doesn't Babylon lose liveness and vigilante monitor will raise alarms? Babylon (and phase2 integration) has liveness only when there is a checkpoint in every w BTC blocks. No checkpoint in w consecutive BTC blocks implies that Babylon is censoring BTC checkpoints.
with each finalised checkpoint, find lowest common ancestor between smart contract chain and current babylon tip.
This was the exact design of phase2 integration. However, in the new data model of BTC light client, finding lowest common ancestor of two forks seems impossible, because every header that is not canonical is removed.
store last W+1 blocks known by smart contract
This will make the zoneconcierge stateful in the sense that it rememebers the state of each Babylon contract. Given that Babylon contract will allow anyone to submit BTC headers, Babylon won't precisely know the state of the Babylon contract. This will also introduce the O(w * n) storage overhead where n is the number of chains doing phase-2 integration.
In fact, Babylon will send exactly the same BTC segment to each chain with phase-2 inetrgation, so this mechanism effectively falls back to storing the last w+1 BTC headers in Babylon's BTC light client. The current implementation can be considered as an "optimisation" to this approach, i.e., if the last BTC segment is not entirely reverted then Babylon sends much fewer BTC headers to Babylon contract than w+1.
Overall, I think we can live with having the w assumption (which is already the case) in this part of the code for now, and add back the functionality to preserve forks and find common ancestors if there is a compelling use case. Wdyt?
There was a problem hiding this comment.
This will make the zoneconcierge stateful in the sense that it rememebers the state of each Babylon contract. Given that Babylon contract will allow anyone to submit BTC headers, Babylon won't precisely know the state of the Babylon contract. This will also introduce the O(w * n) storage overhead where n is the number of chains doing phase-2 integration.
Doesn't this whole logic go away when btc lc smart contract will be allow anybody to submit headers ? Then it true that every chain can have btc lc in different state, so that chain submitted by Babylon may be not the best i.e
babylon finalized checkpoint with btc chain with length - 10, but in the mean time chain has beed extended to 11, so whatever Babylon will send will be invalid.
Overall, I think we can live with having the w assumption (which is already the case) in this part of the code for now, and add back the functionality to preserve forks and find common ancestors if there is a compelling use case. Wdyt?
Yup, I think we need to live with W assumption as this is perversive in many places.
There was a problem hiding this comment.
Doesn't this whole logic go away when btc lc smart contract will be allow anybody to submit headers ?
Ideally, if there 1 honest guy who keeps sending new BTC headers to Babylon contract, then yes Babylon does not need to send it over again. Thing is that we cannot rely on random users for Babylon contract's liveness. Thus Babylon still needs to send over BTC headers. Some of those headers might be duplicated, so Babylon contract is implemented in a way that it ignores (but does not reject) duplicated BTC headers.
There was a problem hiding this comment.
Ideally, if there 1 honest guy who keeps sending new BTC headers to Babylon contract, then yes Babylon does not need to send it over again. Thing is that we cannot rely on random users for Babylon contract's liveness. Thus Babylon still needs to send over BTC headers. Some of those headers might be duplicated, so Babylon contract is implemented in a way that it ignores (but does not reject) duplicated BTC headers.
So this is a choice in the sense that:
- either we leave Babylon sending those headers
- or we introduce assumption in smart contract that there is at least one header reporter for btc headers (similar assumption as in Babylon)
There was a problem hiding this comment.
We could have have both of them, namely Babylon and a vigilante program submit BTC headers to Babylon contract simultaneously. This is to answer the concerns about censorship resistance of Babylon contract -- even Babylon is censoring BTC headers, Babylon contract can still receive BTC headers, and vigilante monitor on phase2 integration will panick if Babylon censors BTC timestamps as well.
| @@ -182,6 +195,12 @@ func (k Keeper) getHeadersToBroadcast(ctx context.Context) []*btclctypes.BTCHead | |||
| } | |||
| } | |||
|
|
|||
| if initHeader == nil { | |||
| // if initHeader is nil, then this means a large reorg happens such that all headers | |||
There was a problem hiding this comment.
then this means a large reorg it not necessarily large reorg. In case when segment had length 1 which is possible when checkpoints finalizing different epoch are in adjacent btc blocks, this could be small re-org
There was a mistake in the assertions, making the test flaky. Fixed and fuzzed the test locally |
(Migrated from the private repo)
The new BTC data model changes the algorithm of finding BTC headers to send upon a finalised epoch in the sense that it no longer resilient against long reorg in Bitcoin -- if the last segment is entirely reverted due to the reorg,
initHeaderwill beniland the chain will panic (stack trace attached below).This PR makes zoneconcierge resilient against long BTC reorg again. In particular, if zoneconcierge finds the last sent segment is reverted entirely, then it will send the last
w+1BTC headers, such that the 1st BTC header among them isw-deepand must be canonical assuming w-long reorg never happens.In addition, this PR also restores the design that upon initialising a Babylon contract, the IBC packet will only include the last
w+1BTC headers rather than all the way down to genesis, such that the 1st BTC header among them isw-deepand will be the base header of Babylon contract.This PR also introduces assertions in BTC light client about the last reorg point storage, and adds a fuzz test on
getHeadersToBroadcast.Appendix: stack trace upon a reorg reverting the entire
LastSentSegmentTo reproduce, comment out this in this PR and run the new fuzz test.