Add functions related to consensus#109
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds new functions to support consensus-related operations by managing and analyzing bead relationships.
- Added public function load_beads_in_memory to ingest beads.
- Introduced private helper methods: get_parents, get_children, get_geneses, and get_tips to compute bead relationships.
- Added tests to validate the new functionality.
Comments suppressed due to low confidence (2)
braidpool-primitives/src/braid.rs:128
- [nitpick] Consider renaming 'get_geneses' to 'get_genesis' to improve clarity and align with standard naming conventions.
fn get_geneses(&self) -> HashSet<BeadHash> {
braidpool-primitives/src/braid.rs:151
- [nitpick] The variable 'parents' in this loop represents the children of a bead. Consider renaming it to something like 'children_set' for improved readability.
for (beadhash, parents) in children {
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new functions to handle bead consensus, with additional tests to support them.
- Added a function to load beads into memory.
- Implemented helper functions to derive parent, child, genesis, and tip bead relationships.
Comments suppressed due to low confidence (1)
braidpool-primitives/src/braid.rs:128
- [nitpick] The function name 'get_geneses' is unconventional. Consider renaming it to 'get_genesis' for improved clarity and consistency.
fn get_geneses(&self) -> HashSet<BeadHash> {
Sansh2356
left a comment
There was a problem hiding this comment.
Looks fine with the functions , after adding tests further work can be continued from my part @Aritra8438 .
6c2507e to
7b15b93
Compare
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
braidpool-primitives/src/braid.rs:176
- [nitpick] Replacing the orphan check with a genesis bead check could be a logical error; verify if this change is intentional or if the original orphan condition should be retained.
if orphan_bead.is_genesis_bead(self) {
There was a problem hiding this comment.
Using a panic here may result in unexpected application termination; consider propagating the error or handling it gracefully.
| let bead = { | |
| match self.load_bead_from_hash(*parent_hash) { | |
| Ok(bead) => bead, | |
| Err(_) => panic!("This shouldn't be happening!"), | |
| } | |
| }; | |
| bead.add_child(child_bead.bead_hash); | |
| } | |
| let bead = self.load_bead_from_hash(*parent_hash)?; | |
| bead.add_child(child_bead.bead_hash); | |
| } | |
| Ok(()) |
There was a problem hiding this comment.
Looks fine! I'll ask @Sansh2356 as it's my PR and can't approve.
8eaad2a to
ac2023c
Compare
Signed-off-by: Aritra Majumder <aritramajumder8438@gmail.com>
Signed-off-by: Aritra Majumder <aritramajumder8438@gmail.com>
…nship check This commit introduces several improvements and new functionality to the Braid and Bead modules: Key changes: 1. **Bead Parents Refactor**: - Changed the `parents` field in the `Bead` struct from a `HashSet<BeadHash>` to a `HashMap<BeadHash, Time>`. - This change enables faster lookups of parent beads directly using the `BeadHash`, improving performance for parent-child relationship checks. 2. **Function Visibility Refactor**: - Refactored several private functions in the `Braid` implementation to use `pub(crate)` visibility, ensuring they remain private but accessible throughout the crate for better modularity and testing. 3. **New Parent-Child Relationship Function**: - Added a new function `is_parent_of` to the `Bead` struct. - This function checks whether a bead is the parent of another bead by verifying the `parents` field of the child bead. - The function returns a `Result<bool, BeadLoadError>` to handle cases where the child bead is not found in memory. These changes improve the efficiency of parent lookups, enhance code maintainability, and add functionality for verifying parent-child relationships between beads. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
…ability This commit introduces a `children` field to the `Bead` struct, enabling the tracking of child beads. The field is implemented as a `RefCell<HashSet<BeadHash>`, allowing for interior mutability while maintaining Rust's borrowing rules. Key changes: - Added a `children: RefCell<HashSet<BeadHash>>` field to the `Bead` struct. - This field is used to store the `BeadHash` values of child beads. - The use of `RefCell` allows mutation of the `children` field even when the `Bead` instance is immutable. - Added a public method `is_parent_of` to check if the current bead is a parent of a given child bead. - This method uses the `children` field to verify the relationship. - Updated the `Bead` struct to support interior mutability for child bead relationships. These changes improve the functionality of the `Bead` struct by enabling dynamic updates to child bead relationships while adhering to Rust's safety guarantees. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
…anization This commit moves all bead-related functions, such as `is_genesis_bead`, `is_orphaned`, and `is_parent_of`, into the `impl Bead` block. This change improves code organization and readability by grouping functionality directly related to the `Bead` struct within its implementation. Key changes: - Moved `is_genesis_bead` to the `impl Bead` block, as it directly operates on bead data. - Moved `is_orphaned` and `is_parent_of` to the `impl Bead` block for consistency. - Updated references to these functions to reflect their new location. This refactor ensures that bead-related logic is encapsulated within the `Bead` struct, making the codebase more modular and easier to maintain. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
This commit introduces a `children` field to the `Bead` struct as an optimization. The `children` field is implemented as a `RefCell<HashSet<BeadHash>>`, allowing efficient tracking of child beads while maintaining Rust's borrowing rules. Key changes: - Added a `children` field to the `Bead` struct to store the `BeadHash` values of child beads. - Implemented the `is_parent_of` method to check if the current bead is a parent of a given child bead using the `children` set. - Added the `get_children` method to retrieve all child beads as a `Children` collection. These changes improve the performance of parent-child relationship checks and make the `Bead` struct more efficient for operations involving child beads. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
This commit introduces a new `update_children` function to the `Bead` struct, allowing dynamic updates to the `children` field. This function provides a convenient way to manage child bead relationships efficiently. Key changes: - Added the `update_children` function to modify the `children` field in the `Bead` struct. - The function uses the `RefCell` interior mutability pattern to safely update the `HashSet` of child beads. - Ensures that child relationships can be dynamically updated without violating Rust's borrowing rules. This addition improves the flexibility of the `Bead` struct by enabling efficient updates to child bead relationships. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
…nd pruning This commit introduces optimizations to the `Braid` struct by caching the genesis beads during the creation or pruning of a DAG. This ensures that genesis beads are never recalculated, improving performance and reducing redundant computations. Key changes: - Added a `genesis_beads` field to the `Braid` struct to cache the set of genesis beads. - Updated the `new` and `generate_from_previous_dag` functions to initialize and maintain the `genesis_beads` cache. - Ensured that the `genesis_beads` cache is updated appropriately during DAG pruning or bead addition. These changes improve the efficiency of operations involving genesis beads by avoiding unnecessary recalculations and leveraging the cached values. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
…nd Bead This commit introduces several improvements to the `Braid` and `Bead` structures, focusing on better management of child relationships and updating parent beads when a child bead is added to the DAG. Key changes: 1. **Add `add_child` Function to Bead**: - Added a private `add_child` function to the `Bead` struct to manage child relationships. - The `children` field in the `Bead` struct is now private, ensuring that it can only be modified through helper functions like `add_child`. - This encapsulation improves code clarity and prevents direct, uncontrolled modifications to the `children` field. 2. **Update Parents of a Bead**: - Added the `update_parents_of_bead` function to the `Braid` struct. - This function is called when a bead is added to the DAG (not to the orphan set). - It iterates over the bead's parents and updates their `children` field to include the new bead. - Ensures that parent-child relationships are correctly maintained in the DAG. 3. **Integration with `add_bead`**: - Updated the `add_bead` function to call `update_parents_of_bead` when a bead is successfully added to the DAG. - This ensures that parent-child relationships are dynamically updated during DAG construction. These changes improve the modularity and maintainability of the code by: - Encapsulating child management within the `Bead` struct. - Automating parent updates when a bead is added to the DAG. - Preventing direct manipulation of internal fields, ensuring clear and consistent usage of helper functions. Future work: - Extend the `update_parents_of_bead` function to handle asynchronous database updates in future versions. - Add tests to verify the correctness of parent-child relationship updates. Signed-off-by: Calisto Mathias <calistomathias.work@gmail.com>
ac2023c to
9e8239d
Compare
| // Committed Braidpool Metadata, | ||
| pub lesser_difficulty_target: CompactTarget, | ||
| pub parents: HashSet<(BeadHash, Time)>, | ||
| pub parents: HashMap<BeadHash, Time>, |
There was a problem hiding this comment.
I think this should be a HashSet and we should put time elsewhere. Time isn't needed for consensus.
| } | ||
|
|
||
| #[inline] | ||
| pub fn is_parent_of(&self, child_bead_hash: BeadHash) -> bool { |
There was a problem hiding this comment.
How do you intend to use is_* functions? I personally dislike a bunch of helper functions like this, I'd rather write something like (pythonic) if bead in otherbead.parents { ... }. Unless this presents a problem with borrowing in Rust...
| } | ||
|
|
||
| #[inline] | ||
| pub fn get_parents(&self) -> Parents { |
There was a problem hiding this comment.
Why would we need a get_parents() function instead of just using bead.parents (assuming bead is a public struct).
| } | ||
|
|
||
| impl Braid { | ||
| fn update_parents_of_bead(&self, child_bead: &Bead) { |
There was a problem hiding this comment.
The parents of a bead are static, and committed to when the bead is created. Setting the parents should be part of a constructor. We should never be adding parents.
If e.g. we're mining on a bead having parents A and B, and a new bead C comes in, we should delete the previous bead we were mining on and instantiate a new Bead object with A,B,C. Adding the parent C changes the coinbase tx which commits to parents in an OP_RETURN (committed metadata), which changes the merkle root, and changes the bead entirely. There's no need to keep the old (A,B parent) bead around, it will never be mined and can be discarded.
| @@ -112,66 +157,23 @@ impl Braid { | |||
| cohorts | |||
There was a problem hiding this comment.
Call braid.cohorts() here with the parents HashMap (and cached children hashmap)
| // Internal Type Aliases | ||
| pub(crate) type ParentBeadHash = BeadHash; | ||
| pub(crate) type ChildrenBeadHash = BeadHash; | ||
| pub(crate) type RelativeBeadHash = BeadHash; |
There was a problem hiding this comment.
Why have three separate types for this? Why not just use BeadHash?
| use crate::utils::{BeadHash, BeadLoadError, Children, Parents}; | ||
|
|
||
| // Type Aliases | ||
| type TransactionWithMerklePath = (Transaction, MerklePathProof); |
There was a problem hiding this comment.
How and when do we plan to use a Merkle Path proof here?
Txs are included in a bead (and there should be a small Merkle tree with the ~2-5 txs so they can be easily committed to).
Merkle proofs are really only used when you need to communicate compactly with another party who doesn't have the same database that we do. As long as we construct our database consistently, we don't need to prove to ourselves that a tx was included -- we can just do a db query to verify it, and if we want we can chase the container bead, its merkle tree, etc to verify if we think there's a reason our db is wrong.
I can see ways we might use merkle proofs in the future (settlement/payouts), but I'm not sure how to grok Merkle paths in the basic Braid/Bead structure...
| } | ||
| } | ||
|
|
||
| pub fn load_beads_in_memory(&mut self, beads: Vec<Bead>) { |
There was a problem hiding this comment.
I think a sanity check should be done prior to inserting in order to avoid inserting duplicates.
| self.tips.insert(bead.bead_hash); | ||
| self.update_parents_of_bead(&bead); | ||
|
|
||
| self.cohorts = self.calculate_cohorts(); |
There was a problem hiding this comment.
Can we somehow use the previous cohort state to update the current one, rather than recomputing the entire thing. This would be a major optimisation
There was a problem hiding this comment.
We need to improve the overall error handling of the add_bead function. The remove_parent_beads_from_tips must return a Result, in case of an error we must return it (after conversion to the AddBeadStatus enum), same goes for update_parents_of_bead and update_orphan_bead_set. All the functions doing any sort of update deletion must return the status to ensure proper error handling.

This PR adds functions related to consensus.
In this PR, I'll also add tests.