Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup blockexchange blockAddress handling #1051

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

benbierens
Copy link
Contributor

@benbierens benbierens commented Dec 18, 2024

The use of CIDs and BlockAddresses in this part of the code could use a little clean-up. Some of this is unused code, left-over from the announce-every-block days. Some of the code can be easily made redundant by concolidating some knowledge where possible. Every little bit we clean up, we don't have to carry forward and maintain.

@benbierens benbierens changed the base branch from master to feature/blkexc-peer-selection December 18, 2024 12:35
@dryajov dryajov force-pushed the feature/blkexc-peer-selection branch 2 times, most recently from afbfadb to af0c9d8 Compare January 9, 2025 21:17
Base automatically changed from feature/blkexc-peer-selection to master January 9, 2025 23:57
@benbierens benbierens force-pushed the feature/blockexc-cleanup-blockaddrs-handling branch from 3e706b3 to 8318eac Compare January 10, 2025 08:19
@benbierens benbierens marked this pull request as ready for review January 10, 2025 08:35
@benbierens benbierens requested review from dryajov and gmega January 13, 2025 08:28
toSeq(b.pendingBlocks.wantListCids)
.filter do(cid: Cid) -> bool:
not b.peers.anyIt( cid in it.peerHaveCids ))
toSeq(b.pendingBlocks.wantList).filterIt(b.peers.peersHave(it).len == 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm trusting those two things are the same....

@@ -130,19 +130,6 @@ iterator wantList*(p: PendingBlocksManager): BlockAddress =
for a in p.blocks.keys:
yield a

iterator wantListBlockCids*(p: PendingBlocksManager): Cid =
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the actual dead code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, unused code.

@@ -38,12 +38,6 @@ type
proc peerHave*(self: BlockExcPeerCtx): seq[BlockAddress] =
toSeq(self.blocks.keys)

proc peerHaveCids*(self: BlockExcPeerCtx): HashSet[Cid] =
Copy link
Member

Choose a reason for hiding this comment

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

And this 🙂

Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

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

OK, LGTM

@benbierens benbierens enabled auto-merge January 15, 2025 10:22
@benbierens benbierens added the Client See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Jan 15, 2025
@benbierens benbierens self-assigned this Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client See https://miro.com/app/board/uXjVNZ03E-c=/ for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants