iframe-proxy

ABD-AZE · 2025-08-11T17:39:21Z

Additional changes made

Added helper functions (reset , get_beads_after) in braid/mod.rs .
Added tests for verifying correct serialisation and deserialisation of enum variants in bead/mod.rs .
Added extensive tests for get_beads_after() function .
Added better error handling by introducing BeadSyncError enum .
Complete IBD functionality following the flow as GetTips ---> GetBeads ----> GetData along with acknowledgement.
IBDHandler for managing the cache requirements corresponding to IBD .
Tested over the cases such as
- A new node is spawned containing zero beads and undergoes IBD from a sync node .
- A new node was spawned and after syncing but it has not mined and the sync node also didn't mined .
- A new node after syncing mined beads and the peer node was also connected .
- A new node after syncing mined beads and the peer got disconnected and reconnected to undergo IBD wrt new node which mined some beads during the time the sync node got disconnected .
- Vice-versa of 4th just mining being done at the end of peer node instead of new node .

NOTE: Algorithm works only if Braid::cohorts vector is topologically sorted

I have read the extend function and it seems that the above condition is always true, please review this once again for any edge cases.

mcelrath

Overall this is good, it just lacks tests. Here is a list from Gemini of missing tests:

No get_beads_after Logic Tests: The new Braid::get_beads_after() function in
node/src/braid/mod.rs, which contains the core logic for this feature, has no unit
tests. As I mentioned in the review, the logic of this function might be fragile,
making the lack of tests a critical issue.
No Serialization Tests: The new BeadRequest::GetBeadsAfter and
BeadResponse::GetBeadsAfter variants are not tested for serialization and
deserialization in node/src/bead/tests.rs. This means there's no guarantee they can be
correctly encoded and decoded for transmission over the network.
No BeadSyncError Serialization Tests: The new BeadSyncError enum itself lacks unit
tests for its Encodable and Decodable implementations.
No End-to-End Test: There are no integration tests that simulate a full
request-response cycle for the GetBeadsAfter flow between two peers. The test in
node/src/behaviour/tests.rs only covers the older GetBeads flow.
No GenesisMismatch Test: The new logic for handling a GenesisMismatch error (where a
peer receives this error and then requests the genesis beads) is not covered by any
test.

Copilot

Pull Request Overview

This PR implements support for GetBeadsAfter request-response functionality to enable efficient incremental synchronization between nodes in the braid pool network.

Added GetBeadsAfter request/response variants to enable requesting beads after a specific set of tip hashes
Introduced BeadSyncError enum for better error handling with structured error types
Implemented get_beads_after() algorithm in the Braid that relies on topologically sorted cohorts

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-25T18:50:03Z

+                                    } else {
+                                        swarm.behaviour_mut().respond_with_error(
+                                            channel,
+                                            BeadSyncError::Other(String::from(


Replace hardcoded error message with a more specific BeadSyncError variant. The TODO comment indicates this should be addressed.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 32 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

- Copies/reimplements some macros from rust-bitcoin (that are not exported) - Allows an "enum" variant that can include both integer and data-carrying variants with explicit discriminant assignment (because discriminants are protocol "magic numbers" and would be needed for any alternative implementation) - Automatically implements consensus_encode/decode including Vec arguments - Update BeadRequest, BeadResponse, BeadSyncError to use braidpool_protocol! - Remove DRY-violating BeadRequestType - Add rustdoc comments to all these messages - Refuse to respond to most messages if ibd_spinlock is held condition for no sync peer in connection mapping removing unwraps and typos in comments

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

mcelrath · 2025-11-30T22:01:41Z

Currently, this PR is not working to download any beads, because:

The boot node never exits IBD
Peer nodes don't serve what beads they have over IBD when they don't think they're synced.

I verified that a node in IBD correctly refuses Stratum connections.

To fix this:

When in IBD, a node should fire SwarmCommand::InitiateIBD and attempt to initiate IBD with any new incoming peer.
If we failed to find an IBD peer, and we have peers, we need to retry SwarmCommand::InitiateIBD on a timer.
Allow IBD to/from any peer, regardless of whether we're synced. If our beads are different than our peers, both peers need to IBD them in both directions.

The dynamics of the Braid are that it should never fork, and we never discard valid beads. If we had a fork, both sides of the fork need to share their beads. Then someone names both sides as parents and ties them up into a big cohort. The side with lower hashrate on the fork won't get paid, but this is expected behavior. But we have to evaluate that by actually getting those beads, computing the cohorts, and evaluating their work. We make payout decisions on beads we didn't receive so shouldn't be using any kind of timing decisions on whether we should ask for beads. We should always ask for beads.

I think we should remove the ibd_or_not boolean flag and the spinlock. With the above changes, it doesn't matter to peers whether we think we're synced and have decided to mine, because we'll serve beads to anyone. So we can remove the BeadSyncError::PeerSyncing error code I added...

So I propose:

Remove the P2P ibd_spinlock checks and simplify IBDManager:
- Remove variables tied to the spinlock timestamp_mapping, incoming_bead_mapping
- Remove logic in IBDManager about when to exit IBD and ibd_or_not (move to Stratum)
- Remove UpdateTimestampMapping, FetchTimestamp, FetchAllTimestamps, UpdateIncomingBeadMapping, GetIncomingBeadRetryCount, AbortWaitHandle.
- Remove related logic in main.rs lines 479-573, 1006-1041, 1241-1301
Remove BeadSyncError::PeerSyncing and every place it's returned, serve the request instead.
Move the ibd_or_not flag to stratum.rs, rename it to ready_to_mine or something.
- Requires Braid::orphanage_occupancy(10*LATENCY_ALPHA) < 0.5 or something like that.
Keep timestamps of when beads enter and exit Braid::orphans. I added an orphan_index: HashSet<BeadHash> to the Braid anyway in Braid refactor: Close the loop #299 because I needed to look up orphans by hash. I'll expand this to:
- pub orphanage: HashMap<BeadHash, (Bead, time::Instant)> actual orphan store with O(1) lookup by hash.
- pub orphanage_history: VecDeque<(time::Instant, time::Instant)> tracks entry/exit of orphans in the orphanage
- pub fn orphanage_occupancy(window: time::Duration) ->f64 returns the fractional occupancy of the orphanage over window. (includes beads currently in the orphanage)
- I'll probably trigger pruning orphanage_history in extend(). Or we can set up a timer.
Move all the IBDManager fields into PeerInfo in peer_manager/mod.rs. Tracking what tips a peer has is a fundamentally peer-to-peer interaction, and I think belongs here. Add sync_batch_offset, cached_tips, and pending_beadhashes to PeerInfo and add the IBDManager methods to PeerInfo -- all these methods are with respect to a single peer anyway.

I will implement (4) as part of #299.

Changing verbosity of logs

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T17:41:13Z

+                                            continue;
+                                        }
+                                    };
+                                    let _val = _ibd_bridge_rx.await.unwrap();


The error handling in line 1148 uses unwrap() on _ibd_bridge_rx.await, which can panic if the sender is dropped. This is inconsistent with the error handling pattern used elsewhere in the code (e.g., lines 1143-1146).

Recommendation: Handle the potential error properly:

let _val = match _ibd_bridge_rx.await { Ok(val) => val, Err(error) => { error!(error=?error, "Failed to receive offset initialization response"); continue; } };

Suggested change

let _val = _ibd_bridge_rx.await.unwrap();

let _val = match _ibd_bridge_rx.await {

Ok(val) => val,

Err(error) => {

error!(error=?error, "Failed to receive offset initialization response");

continue;

}

};

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

mcelrath · 2025-12-05T18:34:27Z

+) -> anyhow::Result<(String, String, String)> {
+    let mut parent_set: HashMap<usize, HashSet<usize>> = HashMap::new();
+
+    for (idx, b) in beads.iter().enumerate() {


This is incredibly expensive, and in the hot path while the braid lock is being held.

After 299 you can just use braid.parents which has the data you need in O(1).

So I'm going to accept this, but we need to merge #240 then #299 then come back and fix this function.

Yes, agreed will replace it accordingly .

mcelrath · 2025-12-05T18:38:02Z

        }
+
+        //Considering the index of the beads in braid will be same as the (insertion ids-1)
+        let bead_id = braid_data


This will panic if the bead is not added. Move this block of code up into the AddBeadStatus::BeadAdded match block.

Changing verbosity of logs refactor: moving condition in lib.rs to if branch

mcelrath

Looks good! Merging!

ABD-AZE requested review from Copilot and mcelrath and removed request for Copilot August 11, 2025 17:39