Skip to content

Conversation

tomasandroil
Copy link

This PR addresses a TODO in slashing_protection_common.nim related to the interchange import logic.

Changes

  • Replaced inefficient O(n log n) sort of signed_blocks with a linear O(n) scan to select the block with the highest slot.
  • Removed redundant sorting of signed_attestations. The following loop already determines the max epochs.
  • Eliminated unnecessary imports (algorithm) to reduce compile scope.

Rationale

These changes improve performance and reduce complexity with no changes to functionality.

Testing

All slashing protection tests pass:
tests/slashing_protection/test_slashing_protection_db.nim


This is a performance-only cleanup and has been verified locally with full test suite.

Copy link

github-actions bot commented Jul 24, 2025

Unit Test Results

       15 files  ±0    2 660 suites  ±0   1h 18m 38s ⏱️ - 2m 49s
  6 679 tests ±0    6 122 ✔️ ±0  557 💤 ±0  0 ±0 
45 936 runs  ±0  45 164 ✔️ ±0  772 💤 ±0  0 ±0 

Results for commit 31071cd. ± Comparison against base commit a92517b.

♻️ This comment has been updated with latest results.

@etan-status etan-status force-pushed the unstable branch 3 times, most recently from a0510ce to aa88cd4 Compare July 25, 2025 15:05
@tomasandroil
Copy link
Author

Quick reminder about PR #7312 (“optimize slashing protection interchange import”):

  • Switched signed_blocks selection from O(n log n) sort to an O(n) scan for max slot
  • Removed redundant signed_attestations sort (max epochs already determined)
  • Eliminated the unused algorithm import

All tests in tests/slashing_protection/ still pass. Let me know if you need anything else! 🚀

@tersec
Copy link
Contributor

tersec commented Aug 25, 2025

The core idea of replacing O(n log n) sorting with O(n) selection is sound, but this particular PR has lots of other unwarranted comment changes.

It's true that not using std/algorithms might slightly compile time, but those std/ imports are fairly fast, and not in themselves much concern, and practically, it's likely imported anyway by the time beacon_chain/validators/slashing_protection_common.nim is imported due to other modules in Nimbus, so it itself isn't that compelling on its own.

@tomasandroil
Copy link
Author

@tersec
Thanks for the review! I’ve addressed the feedback:

I also restored the chronicles.formatIt PubKey0x for log parity; Please let me know if you’d like any other comments tweaked.

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