Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BM-60: Database schema ER diagram #45

Merged
merged 16 commits into from
Jul 9, 2022
Merged

BM-60: Database schema ER diagram #45

merged 16 commits into from
Jul 9, 2022

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Jul 4, 2022

Created an Entity Relationship diagram for our modest database schema.

The colours were generated by https://coolors.co/79addc-ffc09f-ffee93-fcf5c7-adf7b6

* outcome: success or failure
* events: emitted by staking execution
* logs: emitted by staking execution
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would the epoching module store the events and logs emitted by the staking module in DB, rather than returning it to the user and other modules directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so this is one of the instances where this logical model might be different from the physical one. I will try to add a note.

I am actually a bit confused as to where these are physically stored. Events say they do not impact consensus - I thought that was only true about Logs. Although Header contains LastResultHash, and it indeed says these events are ignored. Not sure how you can build an IBC solution on subscribing to events then.

Anyway, they must be stored somewhere. I just wanted to highlight that we leave some sort of tombstone behind about the execution, eventually.

Copy link
Contributor

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool ER diagram, thanks @aakoshh! I left several comments mainly surrounding the checkpointing module. Please check.

}


package "checkpointing" #FFEE93 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we also need an entity for LastCommitHash which is drawn from Tendermint Block.

We need this to create bls-sig transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added

raw_ckpt |o..|| block : commit hash from
to show this relationship, that it's coming from the Tendermint block.

I don't know the best way to implement this, but I commented about doing aggregation incrementally, why would allow the creation of a raw checkpoint with 0 signatures, aggregate them until the threshold is reached, then freeze. If we did that, then the LastCommitHash can be part of the raw checkpoint from the beginning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I agree. I will do that in future PRs. Thanks for the reminder!

| SIGNED: has +1/3 sigs, await submit
| SUBMITTED: included on BTC
| CONFIRMED: k-deep on BTC
* effective_timestamp: BTC timestamp of k-deep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we decided to add effective_timestamp in the Checkpointing module? @fishermanymc What's it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure about which timestamps (if any) need to be copied here, but they could be useful for someone to see the lifecycle of the checkpoint: when was it included first on BTC, when did we consider it confirmed, etc. But now that we don't allow multiple checkpoints and we don't even allow submissions without a prior checkpoint, the effective timestamp is not needed for example to order checkpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed these for now, but you have use for it, let me know.

* PoP: Proof-of-Possession
}

entity "BLS Signature" as bls_sig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is like a logical model. We don't need this to be stored because it is processed right away and accumulated as aggr_bls_sig, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I added this as an illustration to show that there are multiple signatures, and what relationships they have. I think these are currently explicitly stored, but @gitferry said he might remove them in a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks for the reminder!

| ACCUMULATING: await sigs
| SIGNED: has +1/3 sigs, await submit
| SUBMITTED: included on BTC
| CONFIRMED: k-deep on BTC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought CONFIRMED is w-deep rather than k-deep and w>k?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm not actually sure, I haven't internalised that model. I thought w is something like a timeout, and if a checkpoint doesn't appear within w it means Babylon has potential issues with censoring. But I'm not sure how it affects confirmation.

A k-deep checkpoint surely achieved something in that it won't be rolled back from Bitcoin. At best there can be an alternative submission to show up, but that just means someone else should have received the reward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either. Do @fishermanymc and @KonradStaniec have any opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left this at k-deep and added a FINALIZED state with w-deep.

Removed the timestamps as it's not clear exactly what timestamp that should be, since the last block is still volatile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. I guess I should update the status in the Checkpointing module accordingly. Will do in a future PR.

docs/diagrams/database_schema.puml Outdated Show resolved Hide resolved
* effective_timestamp: BTC timestamp of k-deep
}

entity "BLS Key" as bls_key {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to add MsgCreateValidator in this entity since the message is stored and forwarded to the staking module at the end of the epoch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I added a MsgWrappedCreateValidator and MsgWrappedStaking messages with some loose association to BLS key and the queued message to show that they create these, and move PoP over as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the checkpointing module does not need to store MsgWrappedCreateValidator messages, but forward them to the epoching module. So, I would say that they are here as a logical model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are totally here as logical entries. I used enum for them instead of entity and dashed lines to associate them with the others on the diagram to differentiate.

docs/diagrams/database_schema.puml Outdated Show resolved Hide resolved
Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome diagram! Added some minor comments about the btclightclient module.

docs/diagrams/database_schema.puml Outdated Show resolved Hide resolved
entity "Tip" as btc_tip {
<<singleton>>
--
* hash <<FK>>: block hash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we store the entire header bytes in order to simplify retrieval. By storing the hash, in order to get the header bytes we would need to retrieve the height using the hash->height storage and then retrieve the header bytes using the (height, hash) -> header storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think that falls under the difference between a logical and the physical schema. The point I was trying to make here is that every tip is also header, but at most one header is a tip.

We could attempt to do some other diagram that fully captures the KV store indexes we use.

docs/diagrams/database_schema.puml Show resolved Hide resolved
@aakoshh
Copy link
Contributor Author

aakoshh commented Jul 8, 2022

Are there any more comments on this? Could somebody approve, if not?

@SebastianElvis
Copy link
Member

Are there any more comments on this? Could somebody approve, if not?

Perhaps adding the DB schema for the validator set and the slashed validator set in the epoching module, which are introduced in #35?

@aakoshh
Copy link
Contributor Author

aakoshh commented Jul 8, 2022

@SebastianElvis thanks, added those 👍

Copy link
Contributor

@gitferry gitferry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for multiple rounds of modification. No blocker from my side.

@aakoshh aakoshh merged commit 8a003fa into main Jul 9, 2022
@aakoshh aakoshh deleted the bm-60-er-diagram branch July 9, 2022 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants