Add functions related to consensus by Aritra8438 · Pull Request #109 · braidpool/braidpool · GitHub
Skip to content

Add functions related to consensus#109

Closed
Aritra8438 wants to merge 11 commits into
merged-devfrom
add-prereq-functions
Closed

Add functions related to consensus#109
Aritra8438 wants to merge 11 commits into
merged-devfrom
add-prereq-functions

Conversation

@Aritra8438

Copy link
Copy Markdown
Member

This PR adds functions related to consensus.

In this PR, I'll also add tests.

@Aritra8438 Aritra8438 requested review from TypiCal25 and Copilot April 3, 2025 10:58
@Aritra8438 Aritra8438 self-assigned this Apr 3, 2025

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 {

@Aritra8438 Aritra8438 requested review from Sansh2356 and Copilot April 3, 2025 10:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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
Sansh2356 previously approved these changes Apr 4, 2025

@Sansh2356 Sansh2356 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine with the functions , after adding tests further work can be continued from my part @Aritra8438 .

Comment thread braidpool-primitives/src/braid.rs
Comment thread braidpool-primitives/src/braid.rs Outdated
Comment thread braidpool-primitives/src/braid.rs Outdated
Comment thread braidpool-primitives/src/braid.rs Outdated
Comment thread braidpool-primitives/src/braid.rs Outdated
Comment thread braidpool-primitives/src/braid.rs Outdated
@TypiCal25 TypiCal25 force-pushed the add-prereq-functions branch from 6c2507e to 7b15b93 Compare April 5, 2025 21:02
@TypiCal25 TypiCal25 changed the base branch from add-consensus-functions to dev April 6, 2025 04:56
@TypiCal25 TypiCal25 dismissed Sansh2356’s stale review April 6, 2025 04:56

The base branch was changed.

@TypiCal25 TypiCal25 marked this pull request as ready for review April 6, 2025 05:54
@TypiCal25 TypiCal25 self-assigned this Apr 6, 2025
@TypiCal25 TypiCal25 requested review from a team and mcelrath April 6, 2025 05:55
@Aritra8438 Aritra8438 requested a review from Copilot April 6, 2025 06:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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) {

Comment on lines +122 to +130

Copilot AI Apr 6, 2025

Copy link

Choose a reason for hiding this comment

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

Using a panic here may result in unexpected application termination; consider propagating the error or handling it gracefully.

Suggested change
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(())

Copilot uses AI. Check for mistakes.

@Aritra8438 Aritra8438 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks fine! I'll ask @Sansh2356 as it's my PR and can't approve.

@TypiCal25 TypiCal25 force-pushed the add-prereq-functions branch from 8eaad2a to ac2023c Compare April 6, 2025 06:14
Aritra8438 and others added 10 commits April 6, 2025 14:52
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>
@TypiCal25 TypiCal25 force-pushed the add-prereq-functions branch from ac2023c to 9e8239d Compare April 6, 2025 09:23
@TypiCal25 TypiCal25 self-requested a review April 6, 2025 09:24
@TypiCal25 TypiCal25 dismissed their stale review April 6, 2025 09:24

Self-requested by mistake.

// Committed Braidpool Metadata,
pub lesser_difficulty_target: CompactTarget,
pub parents: HashSet<(BeadHash, Time)>,
pub parents: HashMap<BeadHash, Time>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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>) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

6 participants