Conversation
🟡 Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| info!(total = %total_drained, "drain complete"); | ||
| Ok(total_drained) |
There was a problem hiding this comment.
total_drained is accumulated optimistically before transactions confirm, but some drain txs may fail to confirm (the timeout warning on line 748 logs and continues). This means the returned U256 overstates the actual amount drained when confirmations time out. Consider tracking confirmed vs unconfirmed amounts separately and returning only the confirmed total, or at minimum documenting that the return value is an upper bound.
| let drain_gas_limit = 21_000u128; | ||
| // Use gas_limit * max_fee + buffer to ensure the tx value + gas cost never exceeds balance. | ||
| // The buffer covers any rounding in the node's cost validation. | ||
| let drain_gas_cost = U256::from(drain_gas_limit * max_fee + 10_000); |
There was a problem hiding this comment.
This multiplication can overflow: drain_gas_limit (21_000 u128) * max_fee (which can be up to DEFAULT_MAX_GAS_PRICE = 1_000_000_000_000 u128) + 10_000. The product itself fits in u128, but then it's converted to U256::from(...). The arithmetic would be clearer and safer using U256 throughout:
| let drain_gas_cost = U256::from(drain_gas_limit * max_fee + 10_000); | |
| let drain_gas_cost = U256::from(drain_gas_limit) * U256::from(max_fee) + U256::from(10_000u64); |
|
|
||
| let precompile_addr = precompile_address(&self.id); | ||
|
|
||
| if self.iterations > 1 |
There was a problem hiding this comment.
When iterations > 1 but looper_contract is None, this silently falls through to the single direct-call path, sending only one precompile call. The config validation in test_config.rs catches iterations > 1 without looper_contract at parse time, but PrecompilePayload is a public type and with_options can be called directly with an inconsistent combination. Consider either:
- Validating in
with_options(returnResultor panic with a clear message), or - Adding a
debug_assert!here so misuse is caught in development.
|
|
||
| /// Address of the precompile looper contract (required when using iterations > 1). | ||
| #[serde(default)] | ||
| pub looper_contract: Option<String>, |
There was a problem hiding this comment.
looper_contract is stored as Option<String> and parsed to Address each time convert_tx_type is called (once per precompile entry in the transaction list). Since TestConfig is a deserialization type, this is fine for now, but consider using Option<Address> with a custom deserializer to make invalid states unrepresentable and avoid repeated parsing of the same string.
| let selector = [0x7b, 0x39, 0x5f, 0xc2]; | ||
|
|
||
| // ABI encode: address (32 bytes) + offset to bytes (32) + iterations (32) + bytes length (32) + bytes data (padded) | ||
| let data_len = data.len(); |
There was a problem hiding this comment.
The data_len is cast to u32 here without a bounds check. While current precompile inputs are small (< 256 bytes), this is a latent truncation bug if larger inputs are ever used (e.g., large modexp inputs or identity copies). The encode_modexp_data function above can produce up to 96 + 32 + 32 + 32 = 192 bytes which is fine, but this is worth a note or a debug_assert!(data_len <= u32::MAX as usize).
| let data_len = data.len(); | |
| let data_len: u32 = data.len().try_into().expect("precompile data must fit in u32"); | |
| let padded_data_len = (data_len as usize).div_ceil(32) * 32; |
Review SummaryThe PR adds precompile load testing with a well-designed Findings
None of these are blocking, but items 1 and 3 are worth addressing before merge. |
2 participants
Extends the base-load-tests framework with comprehensive precompile testing capabilities and improved configuration.
Precompile Changes:
Other changes also included: