Feat/stackerdb chunk db #3558
Feat/stackerdb chunk db #3558
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3558 +/- ##
===========================================
- Coverage 0.16% 0.16% -0.01%
===========================================
Files 305 315 +10
Lines 280694 282789 +2095
===========================================
Hits 469 469
- Misses 280225 282320 +2095
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
I have a couple of comments and requests that should be addressed:
- Dynamic table names are pretty close to a sql anti-pattern and can lead to issues down the road (as well as requiring contract name sanitization), and so I think unless there's a very compelling reason to do this, it should be avoided.
StackerDBrefers to two different things in the codebase (and the docs) and I think it's making the codebase confusing (sometimes "stackerdb" is a contract identifier, other times its the database storing multiple "stackerdb"s), and there's no better time than now to try to fix this (if it isn't fixed now, the codebase will probably have to live with this confusion forever)
src/net/stackerdb/bits.rs
Outdated
|
|
||
| /// This module contains methods for interacting with the data contained within the messages | ||
| use crate::net::{ | ||
| Error as net_error, StackerDBChunkData, StackerDBChunkInvData, StackerDBGetChunkData, |
There was a problem hiding this comment.
I think StackerDBChunkData should be in the net::stackerdb module rather than in net
There was a problem hiding this comment.
This and the other StackerDB* structs are new protocol messages, whose definitions IMHO belong with the other message definitions.
src/net/stackerdb/db.rs
Outdated
| format!("stackerdb_{}", normalized_name) | ||
| } | ||
|
|
||
| /// Load up chunk metadata from the database, given the primary key. |
There was a problem hiding this comment.
| /// Load up chunk metadata from the database, given the primary key. | |
| /// Load a chunk's metadata from the database, keyed by the chunk's smart contract, the reward cycle consensus hash, and the chunk identifier. |
Looking at this PR after #3551 and #3552, I think there's a naming conflict with "StackerDB" -- you refer to the local database as "the StackerDB", but also, throughout the code, different smart contracts' associated chunk storage is each called "a StackerDB". This is already kind of confusing to me, and this is pretty early on in the life of this code, so I think we'd probably be better served with a different way to talk about these things.
There was a problem hiding this comment.
I'm having a hard time understanding what the ask here is. The "smart contracts' associated chunk storage" is a stacker DB. The node maintains a local replica of that state for each stacker DB it subscribes to. But, I don't see the value in calling the local replica by a different name, since I think that's already understood simply because a single process cannot be a whole p2p network.
There was a problem hiding this comment.
I think you're referring to the fact that StackerDB the struct currently stores all stacker DB chunks for all subscribed stacker DBs? If so, then I'll rename StackerDB to StackerDBSet to disambiguate it.
src/net/stackerdb/db.rs
Outdated
| /// Set up a database's storage slots. | ||
| /// The slots must be in a deterministic order, since they are used to determine the chunk ID | ||
| /// (and thus the key used to authenticate them) |
There was a problem hiding this comment.
What is a storage slot? It seems like this method is just writing empty data into the table. Why is this necessary?
There was a problem hiding this comment.
It's done this way because the follow-on StackerDB state sync PR needs to be able to extract the version vector for all of the DB's slots. But in order to do that, those slots either have to be instantiated, or we need some kind of sparse version vector calculation. The latter requires one SQL query per slot (since if slot data does not exist, we'd need to synthesize it on the fly), whereas this only requires a single query which can use an index.
…arious DB failures; drop rc_consensus_hash
… all chunks in one table and bind them to a database name entry
jcnelson
requested review from
kantai and
obycode
and removed request for
donpdonp
|
Hey @kantai, this is ready for re-review when you get the chance. Thanks! |
|
@kantai I addressed all your feedback in |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This implements the Stacker DB chunk storage database, as part of breaking apart #3534.