nimbus-eth2 icon indicating copy to clipboard operation
nimbus-eth2 copied to clipboard

optimize slashing protection interchange import

Open tomasandroil opened this issue 5 months ago • 4 comments

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.

tomasandroil avatar Jul 22 '25 11:07 tomasandroil

Unit Test Results

       15 files  ±0    2 660 suites  ±0   1h 18m 38s :stopwatch: - 2m 49s   6 679 tests ±0    6 122 :heavy_check_mark: ±0  557 :zzz: ±0  0 :x: ±0  45 936 runs  ±0  45 164 :heavy_check_mark: ±0  772 :zzz: ±0  0 :x: ±0 

Results for commit 31071cdb. ± Comparison against base commit a92517b3.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 24 '25 12:07 github-actions[bot]

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! 🚀

tomasandroil avatar Aug 05 '25 19:08 tomasandroil

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.

tersec avatar Aug 25 '25 07:08 tersec

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

  • Removed the stray inline commentary.
  • Restored the pruning explanations for blocks/attestations.
  • Re-added the formal proof link and the genesis special-case in attestation checks (slashing-proofs: https://github.com/michaelsproul/slashing-proofs).
  • Restored the EIP-3076 advice context next to the ZeroDigest use (EIP-3076: https://eips.ethereum.org/EIPS/eip-3076#advice-for-complete-databases).
  • Kept the O(n) selection (no sorting) and left std/algorithm out since it’s no longer needed.

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

tomasandroil avatar Aug 25 '25 09:08 tomasandroil