Skip to content

revert: remove stress-test seeding offset changes#1829

Merged
mmagician merged 1 commit intommagician-claude/update-base-to-betafrom
mmagician-claude/stress-test-seed
Mar 24, 2026
Merged

revert: remove stress-test seeding offset changes#1829
mmagician merged 1 commit intommagician-claude/update-base-to-betafrom
mmagician-claude/stress-test-seed

Conversation

@mmagician
Copy link
Contributor

Summary

  • Reverts the index offset separation (pub_offset/priv_offset) and high-entropy StdRng-based seed generation in the stress test seeding
  • Returns to the original simpler block_num * num_accounts + account_index indexing and index.to_be_bytes() seed generation

These changes were introduced to avoid account ID prefix collisions, but the stress test passes without them (verified locally with seed-store --num-accounts 500 --public-accounts-percentage 50).

Test plan

  • Stress test seed-store passes locally without these changes

🤖 Generated with Claude Code

The index offset separation and StdRng-based seed generation were
introduced to avoid account ID prefix collisions, but the stress test
passes without them. Revert to the simpler original logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mmagician mmagician added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 24, 2026
Copy link
Contributor Author

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

LGTM

@Mirko-von-Leipzig
Copy link
Collaborator

Mirko-von-Leipzig commented Mar 24, 2026

Maybe to note that we need this to work with millions of accounts as well. So collisions are maybe possible?

@mmagician
Copy link
Contributor Author

Maybe to note that we need this to work with millions of accounts as well. So collisions are maybe possible?

Yes we might end up with some collisions w.h.p. for large account numbers. This PR only reverts the changes introduced (by confusion in the other PR to keep the diff there relevant).

Are you suggesting to keep the changes? If so, I'd maybe leave this for a follow-up, where we also adapt the stress test in CI?

@Mirko-von-Leipzig
Copy link
Collaborator

Maybe to note that we need this to work with millions of accounts as well. So collisions are maybe possible?

Yes we might end up with some collisions w.h.p. for large account numbers. This PR only reverts the changes introduced (by confusion in the other PR to keep the diff there relevant).

Are you suggesting to keep the changes? If so, I'd maybe leave this for a follow-up, where we also adapt the stress test in CI?

Ah, I wasn't aware of the context :) Was just saying that 500 accounts isn't sufficient to be sure :D Happy to merge this then.

@mmagician mmagician marked this pull request as ready for review March 24, 2026 09:35
@mmagician mmagician merged commit 44c1a68 into mmagician-claude/update-base-to-beta Mar 24, 2026
15 of 16 checks passed
@mmagician mmagician deleted the mmagician-claude/stress-test-seed branch March 24, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants