Add simple monitor module

[KonradStaniec]

Add module which would keep track of different values required by monitor. For now only btc light client height at each epoch end.

Advantages of this approach:

• we can keep monitor queries/data separated from other modules.
• if we will need to add some more complicated queries, it will be easier evolve data model
• we do not affect existing modules with data which is not their core responsiblity

Disadvantages:

• A lot of code for such simple functionality. This is mainly due to protobufs and whole cosmos-sdk ceremony. In reality, this is just separate column in database for additional data, one endpoint, and class subscribing to some additional callbacks.

Personally I prefer having all related additional monitor data under one module rather than scattering it between different modules (epoching, btchechkpoint) especially as this data does not neatly fit any one of those modules. But it would be good to get others opinion.

[SebastianElvis]

Hey Konrad, while the module looks well-implemented, I feel that the approach of including all monitor-related APIs in a new module is a bit over-complicated.

My understanding of Cosmos SDK is that a new module aims at implementing a certain part of the state machine, and all APIs related to it. Such a new module does not affect other modules in terms of the state machine, instead just replicates stuff of other modules. This comes with extra code, maintenance effort, and storage overhead.

In terms of the KV store epoch -> BTC header, my proposal was to include it in the epoch metadata, and add a function for recording the BTC height here. This requires few code changes and no API change. The DB schema also makes sense since the BTC height can be considered as a BTC timestamp of this epoch. Wdyt?

[KonradStaniec]

So, in general I am not super attached to approach in this pr and this pr is draft precisely to discuss stuff.

My understanding of Cosmos SDK is that a new module aims at implementing a certain part of the state machine, and all APIs related to it. Such a new module does not affect other modules in terms of the state machine, instead just replicates stuff of other modules. This comes with extra code, maintenance effort, and storage overhead.

I agree that having this as separate module adds a bit code overhead, but as mentioned in intro comment mainly due to protobuffs and cosmos way of doing things and not really due to some complexity

In terms of the KV store epoch -> BTC header, my proposal was to include it in the epoch metadata, and add a function for recording the BTC height here. This requires few code changes and no API change. The DB schema also makes sense since the BTC height can be considered as a BTC timestamp of this epoch

This is the thing I am not fan of. If you squint hard enough yea you can say The DB schema also makes sense since the BTC height can be considered as a BTC timestamp of this epoch But does epoching module do something with this data ? Not really. It is there only for monitor purposes. Does monitor care about any other data from https://github.com/babylonchain/babylon/blob/dev/proto/babylon/epoching/v1/epoching.proto#L11-L25 ? Again not really. Does epochs are somehow connected to btc height ? Only transitively through the fact that btcheckpoint finalizes epochs based on checkpoint which are at certain btc heights, so the btc client needs to grow.

So in reality what we would be doing is, we would be adding a bit of unrelated data in into core module data model, not for any logic of the module, but for the sake of external process. And it is the same case for btccheckpoint. So in perpetuity those two modules would need to support this piece of data in its kv stores. The data which they do nothing with, it just there for reporting purposes.

This for me is a bit of a smell as then suddenly both epoching module and btcheckpointing module are a bit of hostages of the monitor i.e if monitor would need another piece of data, we would just be pushing it into those modules.

Thats why, imo having separate kv store and separate namespace for monitor queries makes sense. If there were a way to make it without adding whole module I would be all for it.

Another alternative approach for which I would be all for is to instead of sticking this data into KV stores and polluting data model, is to put it into events. Have events like EpochEnded(btcLightClientHeigh: XXX) and CheckpointSubmitted(epoch: XXX, btcLightClientHeigh: XXX) this changes a bit design of monitor, as it is not longer pulling for data, but needs to subscribe to events. This approach is a bit more architecturally sound, as events are by design intended for external processes.

[SebastianElvis]

So in reality what we would be doing is, we would be adding a bit of unrelated data in into core module data model, not for any logic of the module, but for the sake of external process. And it is the same case for btccheckpoint. So in perpetuity those two modules would need to support this piece of data in its kv stores. The data which they do nothing with, it just there for reporting purposes.

I see your point. Actually I was not considering the monitor as an "external" process, but rather an integral part for securing Babylon. The ultimate form of Babylon will be using such new KV stores for verifying evidences of dishonest majority attacks submitted by monitors.

For example, assuming the dishonest majority Babylon is censoring BTC headers for BTC light client, then the monitor will submit an evidence of the stalling as a message, where the evidence includes 100 BTC blocks censored. Then, Babylon will see whether at the end of an epoch the BTC light client indeed fell behind 100 BTC blocks. This will require using the new KV pairs.

Another alternative approach for which I would be all for is to instead of sticking this data into KV stores and polluting data model, is to put it into events. Have events like EpochEnded(btcLightClientHeigh: XXX) and CheckpointSubmitted(epoch: XXX, btcLightClientHeigh: XXX) this changes a bit design of monitor, as it is not longer pulling for data, but needs to subscribe to events. This approach is a bit more architecturally sound, as events are by design intended for external processes.

Yep I think events are also a viable approach. The thing is that it does not allow verifying evidences of safety/liveness violations suggested above. If we don't need such evidences, then it should be fine. Otherwise we will need to touch the KVStores anyway.

[KonradStaniec]

I see your point. Actually I was not considering the monitor as an "external" process, but rather an integral part for securing Babylon. The ultimate form of Babylon will be using such new KV stores for verifying evidences of dishonest majority attacks submitted by monitors.

Hmm tbh If we are considering monitor to submit evidence of misbehaviour, then imo it makes even more sense for all this functionality to be encapsulated into separate module in which we could add separate messages for this misbehaviour submissions (without polluting other modules). And yes, this evidence handling excludes usage of events.

Actually I was not considering the monitor as an "external" process, but rather an integral part for securing Babylon. The ultimate form of Babylon will be using such new KV stores for verifying evidences of dishonest majority attacks submitted by monitors.

I sort of agree with that view. I.e I see monitor as integral part of Babylon when viewed from point of view of whole Babylon system, but still when viewed from point of view of Babylon node it is another external process interacting with the node. So we should expose proper api-s for those interactions, but we should not break our architecture and module separation due to those interactions.

[vitsalis]

I think both options have their merits. To summarize what I understand from the above discussion:

Adding a new module:

• Does not add stuff to core modules of Babylon just to support features related to external processes unrelated to the proper functioning of the node.
  • However, it adds a new module which can be considered a big addition to Babylon core just to support a single query (or even other queries in the future). Other than all the boilerplate code this module needs to be documented etc. and we also have to explain to users why this module exists. Finally, with the addition of this, one might reasonably ask, why not move other queries that require the user to compose data into this module (e.g. get the checkpoint for an epoch)
  • If we have a specific module for handling monitoring stuff, then it is clear who is going to handle the evidence for a liveness attack.

Adding a KVStore on epoching:

• A simple and not invasive solution that uses the existing functionalities.
  • However, this adds a connection between epoching and BTC. In my mind, epoching should know nothing about BTC as it is not its concern. Further, the BTC modules should know nothing about epoching as well. Overall, this is a kinda hacky solution which creates a connection between nodes, again just to support a query.
  • BTC modules could handle the evidence for a liveness attack.

Just rephrasing the issue here: We want to be able to view the state of the btclightclient module (i.e. the tip of the BTC chain) at a particular point in the past (i.e. at the end of a particular epoch). So, if we had a way to query the store at a point in the past, then our problems would be solved. I don't fully understand the code and it's been a while since I took a look at it, but isn't this what the modified staking of the epoching module do (cc @SebastianElvis). If that is possible, another option would be to

• Add a new query to the btclightclient module that queries the tip of the chain at the end of a particular epoch (epoch provided by the block number).
  • However, as @KonradStaniec pointed out in an offline chat, this would require that the node that the monitor connects to, is not an "archive node" (i.e. a node that does not prune previous block data). In my mind, this is an OK requirement to have for running a monitor as this does not take away from the security of Babylon and storage is cheap to not be a very big barrier of entry for users. Also, we do not expect everyone to run a monitor.
  • BTC modules could handle the evidence for a liveness attack.

Overall, if it is possible to go with the third option (i.e. querying past states), I would go with it as it is the least minimally invasive, and it makes sense that just the btclightclient module handles those queries.

Edit: @KonradStaniec mentioned offline that we still have to support evidence for all kinds of nodes. This means that if a node depends on archived data, then nodes that are doing pruning won't be able to verify the evidence. (side note: does this mean that the modified staking that I linked above will have issues with pruned nodes @SebastianElvis ?)

[SebastianElvis]

So, if we had a way to query the store at a point in the past, then our problems would be solved. I don't fully understand the code and it's been a while since I took a look at it, but isn't this what the modified staking of the epoching module do (cc @SebastianElvis).

Modified staking uses a previous header in ctx for a different reason, namely finishing the unbonding process of all validators/delegations before this header's timestamp.

It seems that using a historical header does not give us the ability of querying the KVStore from the past -- KVStore uses the ctx only for accessing the gas meter.

This means that if a node depends on archived data, then nodes that are doing pruning won't be able to verify the evidence.

My feeling is that as long as we don't prune too much (say, keeping most recent blocks in the time range of past W BTC blocks), then a synced node can still verify incoming evidences, since attacks can only happen at this time interval. So probably fine?

[gitferry]

Awesome!

[gitferry]

Suggested change

	func (k Keeper) LighclientHeightAtEpochEnd(ctx sdk.Context, epoch uint64) (uint64, error) {
	func (k Keeper) LightclientHeightAtEpochEnd(ctx sdk.Context, epoch uint64) (uint64, error) {