Skip to content

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
@tomasandroil
Copy link
Author

Hi @MarcoPolo!
Quick reminder about PR #3274 (“fix(pstoreds): Fix cyclic batch threshold handling and add error checking for peer ID decoding”):

  • Correctly initialize cyclicBatch threshold to avoid premature commits
  • Add error handling in uniquePeerIds to skip invalid peer IDs instead of panicking
  • New unit tests for batch threshold behavior and invalid-peer-ID cases

Let me know if you need anything else! 🔧

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