Skip to content

fix(pstoreds): Fix cyclic batch threshold handling and add error checking for peer ID decoding #3274

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

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

Conversation

tomasandroil
Copy link

@tomasandroil tomasandroil commented Apr 24, 2025

Summary

This pull request improves robustness and correctness in the pstoreds package by:

  • Fixing incorrect cyclicBatch threshold initialization that could cause unintended immediate commits.
  • Adding proper error handling during peer ID decoding in uniquePeerIds to skip invalid datastore entries instead of causing potential panics.
  • Adding unit tests for:
    • Cyclic batch threshold behavior.
    • Handling of invalid peer IDs in uniquePeerIds.

Changes

  • Correctly assign the threshold field in newCyclicBatch.
  • Handle errors from base32.RawStdEncoding.DecodeString and peer.IDFromBytes when extracting peer IDs.
  • Add cyclic_batch_test.go with a test verifying that batching only happens after the configured threshold is reached.
  • Add peerstore_test.go with a test verifying that invalid datastore entries are gracefully skipped in uniquePeerIds.

Motivation

Robust error handling and correct thresholding are critical in persistent peerstore operations to ensure system stability, especially under datastore corruption scenarios or large-scale peer management.

Adding unit tests ensures that regressions in these critical code paths are less likely to occur in the future.

Testing

All new unit tests pass:

go test ./p2p/host/peerstore/pstoreds/...

No regressions in existing functionality.

@p-shahi p-shahi requested a review from sukunrt April 24, 2025 20:33
@MarcoPolo
Copy link
Collaborator

Can you please add new tests for this as well? Thanks!

@tomasandroil tomasandroil changed the title fix(pstoreds): Correct cyclic batch threshold and handle peer ID decoding errors fix(pstoreds): Fix cyclic batch threshold handling and add error checking for peer ID decoding Apr 28, 2025
@tomasandroil
Copy link
Author

@MarcoPolo I have added the requested unit tests for both cyclic batch threshold handling and peer ID decoding error handling

@MarcoPolo MarcoPolo self-requested a review May 5, 2025 16:29
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.

2 participants