|
| 1 | +# Guidelines for documenting low-level primitives for persistent storage |
| 2 | + |
| 3 | +The folder `storage/operation` contains low-level primitives for persistent storage and retrieval of data structures from a database. |
| 4 | +We accept that these functions have to be used _carefully_ by engineers that are knowledgeable about the |
| 5 | +safety limitations of these functions to avoid data corruption . In order to facilitate correct usage, we need to diligently document |
| 6 | +what aspects have to be paid attention to when calling these functions. |
| 7 | + |
| 8 | +Proceed as follows |
| 9 | +1. look at one file in `storage/operation` at a time (skip test files for now) |
| 10 | +2. Go over the functions contained in the file one by one and for each function decide whether it is for writing or reading data. |
| 11 | +3. For each function, provide a concise yet precise documentation covering |
| 12 | + - what this function is for |
| 13 | + - the assumptions this function makes about its inputs |
| 14 | + - what has to be payed attention to when calling this function |
| 15 | + - expected error returns during normal operations |
| 16 | + - follow our godocs policy `docs/agents/GoDocs.md` |
| 17 | + |
| 18 | +Guidelines: |
| 19 | +- Tune your documentation on a case by case basis to reflect the function's specific detail. |
| 20 | +- Avoid overly generic documentation. |
| 21 | +- Stick to a uniform framing and wording. |
| 22 | +- Be very concise and precise. |
| 23 | +- Analyze the implementation to make the correct statements! |
| 24 | +- Double check your work. |
| 25 | + |
| 26 | +## High level structure |
| 27 | + |
| 28 | +On the highest level, there are function for storing data and other functions for retrieving data. The naming indicate which class |
| 29 | +a function belongs to, though there is no absolutely uniform convention. For example, some function for loading data start with `Retrieve`, |
| 30 | +while others start with `Lookup`, and additional names might be used as well. So pay close attention to the naming of the function. |
| 31 | + |
| 32 | +Conceptually, we have data structures that contain certain fields. Furthermore, most data structures we deal with provide the functionality |
| 33 | +to compute a cryptographic hash of their contents, which is typically referred to as "ID". We store data as key-value pairs. |
| 34 | +(i) keys are either: the cryptographic hashes of the data structures. |
| 35 | +(ii) Frequently, we break up the storage of compound objects, storing their sub-data structures individually. For example, a block contains the payload, the payload contains Seals. Frequently, we create mappings from the ID of the high-level data structure (e.g. block ID) to the IDs of the lower-level objects it contains (e.g. Seals). For example, `operation.IndexPayloadSeals`. |
| 36 | + |
| 37 | +(i) and (ii) are fundamentally different: In case (i) the key is derived from the value in a collision-resistant manner (via cryptographic hash). |
| 38 | +Meaning, if we change the value, the key should also change. Hence, unchecked overwrites pose no risk of data corruption, because for the same key, |
| 39 | +we expect the same value. In comparison, for case (ii) we derive the key from the _context_ of the value. Note that the Flow protocol mandates that |
| 40 | +for a previously persisted key, the data is never changed to a different value. Changing data could cause the node to publish inconsistent data and |
| 41 | +to be slashed, or the protocol to be compromised as a whole. In many cases, the caller has to be cautious about avoiding usages causing data |
| 42 | +corruption. This is because we don't wan't to implement override protections in all low-level storage functions of type (ii) for performance |
| 43 | +reasons. Rather, we delegate the responsibility for cohesive checks to the caller, which must be clearly documented. |
| 44 | + |
| 45 | + |
| 46 | +### Functions for reading data |
| 47 | + |
| 48 | +When generating documentation for functions that read data, carefully differentiate between functions of type (i) and (ii). |
| 49 | + |
| 50 | +#### Type (i) functions for reading data |
| 51 | + |
| 52 | +As an example for functions of type (i), consider `operation.RetrieveSeal`: |
| 53 | +```golang |
| 54 | +// RetrieveSeal retrieves [flow.Seal] by its ID. |
| 55 | +// Expected error returns during normal operations: |
| 56 | +// - [storage.ErrNotFound] if no seal with the specified `sealID` is known. |
| 57 | +func RetrieveSeal(r storage.Reader, sealID flow.Identifier, seal *flow.Seal) error |
| 58 | +``` |
| 59 | +* We document the struct type that is retrieved, here flow.Seal. Be mindful whether we are retrieving an individual struct or a slice. |
| 60 | +* We document that the key we look up is the struct's own ID. |
| 61 | +* We document the "Expected errors during normal operations:" (use this phrase) |
| 62 | + - in all cases, this will be the error storage.ErrNotFound, followed by a short description that no object with the specified ID is known. |
| 63 | + |
| 64 | +#### Type (ii) functions for reading data |
| 65 | + |
| 66 | +As an example for functions of type (ii), consider `operation.LookupPayloadSeals `: |
| 67 | +```golang |
| 68 | +// LookupPayloadSeals retrieves the list of Seals that were included in the payload |
| 69 | +// of the specified block. For every known block, this index should be populated (at or above the root block). |
| 70 | +// Expected error returns during normal operations: |
| 71 | +// - [storage.ErrNotFound] if `blockID` does not refer to a known block |
| 72 | +func LookupPayloadSeals(r storage.Reader, blockID flow.Identifier, sealIDs *[]flow.Identifier) error error |
| 73 | +``` |
| 74 | +* We document the struct type that is retrieved, here list of Seals. Be mindful whether we are retrieving an individual struct or a slice. |
| 75 | +* Document that the lookup key is the ID of the bock containing the data structure. You can use our standard shorthand in this case, and just write that we are looking up X by Y containing X. |
| 76 | +* We state if the index is populated for every known struct (which is typically the case). Consult the places in the code, where the corresponding index is written, to determine when the index is populated. |
| 77 | +* We document the "Expected errors during normal operations" (use this phrase). Typically, the error explanation is that no struct Y is known, which contains X. |
| 78 | + |
| 79 | + |
| 80 | +### Functions for writing data |
| 81 | + |
| 82 | +When generating documentation for functions that write data, carefully differentiate between functions of type (i) and (ii). |
| 83 | +For type (i), you need to carefully differentiate two sub-cases (i.a) and (i.b). Analogously, for type (ii), |
| 84 | +you need to carefully differentiate two sub-cases (ii.a) and (ii.b) |
| 85 | + |
| 86 | +#### Type (i.a) functions for writing data |
| 87 | + |
| 88 | +As an example for functions of type (i.a), consider `operation.UpsertCollection`: |
| 89 | +```golang |
| 90 | +// UpsertCollection inserts a light collection into the storage, keyed by its ID. |
| 91 | +// |
| 92 | +// If the collection already exists, it will be overwritten. Note that here, the key (collection ID) is derived |
| 93 | +// from the value (collection) via a collision-resistant hash function. Hence, unchecked overwrites pose no risk |
| 94 | +// of data corruption, because for the same key, we expect the same value. |
| 95 | +// |
| 96 | +// No error returns are expected during normal operation. |
| 97 | +func UpsertCollection(w storage.Writer, collection *flow.LightCollection) error { |
| 98 | + return UpsertByKey(w, MakePrefix(codeCollection, collection.ID()), collection) |
| 99 | +} |
| 100 | +``` |
| 101 | +Analyze the implementation! Here, the method itself computes the ID (i.e. cryptographic hash). |
| 102 | +In this case, the function contains internal protections against the caller accidentally corrupting data. |
| 103 | +Only functions that store the struct by its own ID _and_ contain internal safeguards against accidentally corrupting data are of type (i.a)! |
| 104 | + |
| 105 | +* We document the struct type that is stored, here light collection. Be mindful whether we are storing an individual struct or a slice. |
| 106 | +* We state whether the method will overwrite existing data. And then explain why this is safe. |
| 107 | +* We state which errors are expected during normal operations (here none). |
| 108 | + |
| 109 | +#### Type (i.b) functions for writing data |
| 110 | + |
| 111 | +As an example for functions of type (i.b), consider `operation.InsertSeal`: |
| 112 | + |
| 113 | +```golang |
| 114 | +// InsertSeal inserts a [flow.Seal] into the database, keyed by its ID. |
| 115 | +// |
| 116 | +// CAUTION: The caller must ensure sealID is a collision-resistant hash of the provided seal! |
| 117 | +// This method silently overrides existing data, which is safe only if for the same key, we |
| 118 | +// always write the same value. |
| 119 | +// |
| 120 | +// No error returns are expected during normal operation. |
| 121 | +func InsertSeal(w storage.Writer, sealID flow.Identifier, seal *flow.Seal) error { |
| 122 | + return UpsertByKey(w, MakePrefix(codeSeal, sealID), seal) |
| 123 | +} |
| 124 | +``` |
| 125 | +Analyze the implementation! Here, the method itself receives the ID (i.e. the cryptographic hash) if the object it is storing as an input. In this case, the function requires the caller to precompute the ID of the struct and provide it as an input. Only functions that store the struct by its own ID _but_ require the caller to provide this ID are of type (i.b)! |
| 126 | + |
| 127 | +* We document the struct type that is stored, here flow.Seal. Be mindful whether we are storing an individual struct or a slice. |
| 128 | +* We document that the key which we use (here "its ID"). |
| 129 | +* With a "CAUTION" statement, we document the requirement that the caller must ensure that the key is a collision-resistant hash of the provided data struct. |
| 130 | +* We state which errors are expected during normal operations (here none). |
| 131 | + |
| 132 | +#### Type (ii.a) functions for writing data |
| 133 | + |
| 134 | +As an example for functions of type (ii.a), consider `operation.IndexStateCommitment`: |
| 135 | + |
| 136 | +```golang |
| 137 | +// IndexStateCommitment indexes a state commitment by the block ID whose execution results in that state. |
| 138 | +// The function ensures data integrity by first checking if a commitment already exists for the given block |
| 139 | +// and rejecting overwrites with different values. This function is idempotent, i.e. repeated calls with the |
| 140 | +// *initially* indexed value are no-ops. |
| 141 | +// |
| 142 | +// CAUTION: |
| 143 | +// - Confirming that no value is already stored and the subsequent write must be atomic to prevent data corruption. |
| 144 | +// The caller must acquire the [storage.LockInsertOwnReceipt] and hold it until the database write has been committed. |
| 145 | +// |
| 146 | +// Expected error returns during normal operations: |
| 147 | +// - [storage.ErrDataMismatch] if a *different* state commitment is already indexed for the same block ID |
| 148 | +func IndexStateCommitment(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, commit flow.StateCommitment) error { |
| 149 | + if !lctx.HoldsLock(storage.LockInsertOwnReceipt) { |
| 150 | + return fmt.Errorf("cannot index state commitment without holding lock %s", storage.LockInsertOwnReceipt) |
| 151 | + } |
| 152 | + |
| 153 | + var existingCommit flow.StateCommitment |
| 154 | + err := LookupStateCommitment(rw.GlobalReader(), blockID, &existingCommit) // on happy path, i.e. nothing stored yet, we expect `storage.ErrNotFound` |
| 155 | + if err == nil { // Value for this key already exists! Need to check for data mismatch: |
| 156 | + if existingCommit == commit { |
| 157 | + return nil // The commit already exists, no need to index again |
| 158 | + } |
| 159 | + return fmt.Errorf("commit for block %v already exists with different value, (existing: %v, new: %v), %w", blockID, existingCommit, commit, storage.ErrDataMismatch) |
| 160 | + } else if !errors.Is(err, storage.ErrNotFound) { |
| 161 | + return fmt.Errorf("could not check existing state commitment: %w", err) |
| 162 | + } |
| 163 | + |
| 164 | + return UpsertByKey(rw.Writer(), MakePrefix(codeCommit, blockID), commit) |
| 165 | +} |
| 166 | +``` |
| 167 | +Analyze the implementation! Only functions that internally implement safeguards against overwriting a key-value pair with _different_ data for the same key are of type (ii.a)! |
| 168 | + |
| 169 | +* We document the struct type that is stored, here `flow.StateCommitment`. If applicable, we also document additional key-value pairs that are persisted as part of this function (here, none). Analyze the implementation. |
| 170 | +* We concisely document by which means the implementation ensures data integrity. For functions of type (ii.a), we typically just attempt to read the value for the respective key. You may adapt the explanation from this example to reflect the specifics of the implementation. Note that the behaviour might be different if a value has previously been stored. Analyze the implementation. |
| 171 | +* With a "CAUTION" statement, we concisely document the requirement that the read for the data integrity check and the subsequent write must happen atomically. This requires synchronization, and hence locking. We document which locks are required to be held by the caller. |
| 172 | +* Analyze the implementation to decide whether additional cautionary statements are required to reduce the probability of accidental bugs. |
| 173 | +* We state which errors are expected during normal operations (here `storage.ErrDataMismatch`) and the condition under which they occur. Analyze the implementation to make the correct statements! |
| 174 | + |
| 175 | + |
| 176 | + |
| 177 | +#### Type (ii.b) functions for writing data |
| 178 | + |
| 179 | +As an example for functions of type (ii.b), consider `operation.IndexPayloadSeals`: |
| 180 | + |
| 181 | +```golang |
| 182 | +// IndexPayloadSeals indexes the given Seal IDs by the block ID. |
| 183 | +// |
| 184 | +// CAUTION: |
| 185 | +// - The caller must acquire the [storage.LockInsertBlock] and hold it until the database write has been committed. |
| 186 | +// - OVERWRITES existing data (potential for data corruption): |
| 187 | +// This method silently overrides existing data without any sanity checks whether data for the same key already exits. |
| 188 | +// Note that the Flow protocol mandates that for a previously persisted key, the data is never changed to a different |
| 189 | +// value. Changing data could cause the node to publish inconsistent data and to be slashed, or the protocol to be |
| 190 | +// compromised as a whole. This method does not contain any safeguards to prevent such data corruption. The lock proof |
| 191 | +// serves as a reminder that the CALLER is responsible to ensure that the DEDUPLICATION CHECK is done elsewhere |
| 192 | +// ATOMICALLY with this write operation. |
| 193 | +// |
| 194 | +// No error returns are expected during normal operation. |
| 195 | +func IndexPayloadSeals(lctx lockctx.Proof, w storage.Writer, blockID flow.Identifier, sealIDs []flow.Identifier) error { |
| 196 | + if !lctx.HoldsLock(storage.LockInsertBlock) { |
| 197 | + return fmt.Errorf("cannot index seal for blockID %v without holding lock %s", |
| 198 | + blockID, storage.LockInsertBlock) |
| 199 | + } |
| 200 | + return UpsertByKey(w, MakePrefix(codePayloadSeals, blockID), sealIDs) |
| 201 | +} |
| 202 | +``` |
| 203 | + |
| 204 | +Analyze the implementation! Only functions are of type (ii.b) that delegate the check whether an entry with the specified key already exists to the caller! |
| 205 | + |
| 206 | +* We document the struct type that is stored, here "the given Seal". If applicable, we also document additional key-value pairs that are persisted as part of this function (here none). Analyze the implementation. |
| 207 | +* With a "CAUTION" statement, we document that the caller must provide protections against accidental overrides. Typically, those protections require reads happening in one atomic operation with the writes. To perform those reads atomically with the writes, the caller is intended to hold the specified locks and only release them after the database writes have been committed. |
| 208 | + - The first bullet point in the CAUTION statement specifies which locks the caller must hold and that those locks are to be held until the writes have been committed. |
| 209 | + - The second bullet point in the CAUTION statement emphasizes that the caller must provide protections against accidental overrides with different data. You may copy the wording of the second bullet point. It is generic enough, so it should apply in the majority of cases. |
| 210 | +* We state which errors are expected during normal operations (here none) and the condition under which they occur. Analyze the implementation to make the correct statements! |
| 211 | + |
| 212 | + |
0 commit comments