feat: allow specifying num callers for simulator #143
+216
−39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
🟡 Heimdall Review Status
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
runner/payload/simulator/worker.go
Dismissed
| keys := make([]*ecdsa.PrivateKey, numCallers) | ||
| addrs := make([]common.Address, numCallers) | ||
| for i := 0; i < numCallers; i++ { | ||
| key, err := ecdsa.GenerateKey(crypto.S256(), src) |
Check failure
Code scanning / CodeQL
Use of insufficient randomness as the key of a cryptographic algorithm High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, cryptographic keys must be generated using a cryptographically secure pseudo-random number generator (CSPRNG), such as Go’s crypto/rand, and must not depend on math/rand. If deterministic derivation from a master key is desired (e.g., for reproducible simulations), it should be implemented using deterministic key-derivation techniques (e.g., hashing plus modular reduction) rather than by plugging a weak PRNG into a cryptographic key-generation API.
For this specific code, the simplest, least invasive fix is:
- Stop using
math/randas theio.Readerpassed toecdsa.GenerateKey. - Instead, derive each new private key deterministically from the prefunded key using a cryptographic hash, and construct the ECDSA private key directly on the secp256k1 curve:
- For each caller index
i, computeskInt = keccak256(prefundedKey.D || i) mod N, whereNis the curve order. - Ensure
skIntis non-zero; if zero, tweak it (e.g., hash again with a suffix) or fall back to a minimal non-zero value. - Set
privKey := &ecdsa.PrivateKey{D: skInt, PublicKey: *pubKey}wherepubKeyis computed viacrypto.S256().ScalarBaseMult(skInt.Bytes()).
- For each caller index
- This preserves determinism while relying only on cryptographic primitives, not on
math/rand, and removes the taintedrand.NewSourcepath entirely.
Concretely in runner/payload/simulator/worker.go:
- Remove the use of
math/randinsidegenerateCallerAccounts(we can leave the import alone if we’re not allowed to touch unused imports, but we will not use it). - Add a small helper inside
generateCallerAccounts(or inline logic) to compute a derived scalar fromprefundedKey.Dand the index usingcrypto.Keccak256andnew(big.Int).Mod(...). - Use that derived scalar to populate
keys[i]andaddrs[i]instead of callingecdsa.GenerateKeywithsrc.
No new external dependencies are needed; we only use crypto/ecdsa, crypto/elliptic via crypto.S256(), math/big, and github.com/ethereum/go-ethereum/crypto (already imported).
Suggested changeset
1
runner/payload/simulator/worker.go
| @@ -204,20 +204,46 @@ | ||
| return []*ecdsa.PrivateKey{prefundedKey}, []common.Address{crypto.PubkeyToAddress(prefundedKey.PublicKey)} | ||
| } | ||
|
|
||
| // Use deterministic random source seeded from prefunded key | ||
| seed := int64(prefundedKey.D.Uint64()) | ||
| src := rand.New(rand.NewSource(seed)) | ||
| // Deterministically derive additional caller keys from the prefunded key using a cryptographic hash. | ||
| // This avoids using a non-cryptographic PRNG as the entropy source for key generation. | ||
| curve := crypto.S256() | ||
| N := curve.Params().N | ||
|
|
||
| keys := make([]*ecdsa.PrivateKey, numCallers) | ||
| addrs := make([]common.Address, numCallers) | ||
| for i := 0; i < numCallers; i++ { | ||
| key, err := ecdsa.GenerateKey(crypto.S256(), src) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to generate caller key: %v", err)) | ||
|
|
||
| // First caller uses the prefunded key directly for consistency with the numCallers == 1 case. | ||
| keys[0] = prefundedKey | ||
| addrs[0] = crypto.PubkeyToAddress(prefundedKey.PublicKey) | ||
|
|
||
| for i := 1; i < numCallers; i++ { | ||
| // Derive a new private scalar from prefundedKey.D and the index using keccak256, reduced mod N. | ||
| // Encode index deterministically as base-10 string bytes. | ||
| indexBytes := []byte(strconv.Itoa(i)) | ||
| baseDBytes := prefundedKey.D.Bytes() | ||
| derivedHash := crypto.Keccak256(baseDBytes, indexBytes) | ||
|
|
||
| d := new(big.Int).SetBytes(derivedHash) | ||
| d.Mod(d, N) | ||
| if d.Sign() == 0 { | ||
| // Extremely unlikely, but ensure we never use zero as a private scalar. | ||
| d.Add(d, big.NewInt(1)) | ||
| } | ||
| keys[i] = key | ||
| addrs[i] = crypto.PubkeyToAddress(key.PublicKey) | ||
|
|
||
| x, y := curve.ScalarBaseMult(d.Bytes()) | ||
| priv := &ecdsa.PrivateKey{ | ||
| D: d, | ||
| PublicKey: ecdsa.PublicKey{ | ||
| Curve: curve, | ||
| X: x, | ||
| Y: y, | ||
| }, | ||
| } | ||
|
|
||
| keys[i] = priv | ||
| addrs[i] = crypto.PubkeyToAddress(priv.PublicKey) | ||
| } | ||
|
|
||
| return keys, addrs | ||
| } | ||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
Collaborator
Author
There was a problem hiding this comment.
Used for testing - we want a deterministic key here.
This allows testing things like Block-STM where the number of callers affects performance. If we use a single caller, every transaction conflicts with the next transaction on the balance/nonce of the caller.
meyer9
force-pushed
the
meyer9/simulator-num-callers
branch
from
518daed to
e4aab51
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows testing things like Block-STM where the number of callers affects performance. If we use a single caller, every transaction conflicts with the next transaction on the balance/nonce of the caller.
Description
Testing