-
Notifications
You must be signed in to change notification settings - Fork 759
chore(tests/load): C-chain load testing #3914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Scratch that, let's tackle separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the comments under tests/load/c
can be ignored. The code there is temporary.
address public owner; | ||
mapping(uint256 => uint256) public data; | ||
|
||
event ValueUpdated(uint256 newValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event is never used, is that intentional?
/// a constructor and simple functions to have a real-like deployed size. | ||
contract Dummy { | ||
uint256 public value; | ||
address public owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owner
is only used in the constructor, is this needed?
maxFeeCap *big.Int | ||
generateAndIssueTx func(txCtx context.Context, gasFeeCap *big.Int, nonce uint64) (*types.Transaction, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have maxFeeCap
as a field on this struct just to pass it into the function defined in this struct? Shouldn't generateAndIssueTx
just include this number directly?
return txType{}, errZeroTotalWeight | ||
} | ||
|
||
r := rand.UintN(totalWeight) //nolint:gosec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than disabling gosec
entirely, we should disable the specific error.
r := rand.UintN(totalWeight) //nolint:gosec | |
r := rand.UintN(totalWeight) //#nosec G404 |
if seed <= 0 { | ||
return nil, errNonpositiveSeed | ||
} | ||
return big.NewInt(rand.Int64N(seed)), nil //nolint:gosec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return big.NewInt(rand.Int64N(seed)), nil //nolint:gosec | |
return big.NewInt(rand.Int64N(seed)), nil //#nosec G404 |
return txType{}, errFailedToSelectTxType | ||
} | ||
|
||
func randomNum(seed int64) (*big.Int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seed
is a pretty confusing name for this. This is actually maximum
. That being said, the existence of this function just further complicates the code. All of the places that call this function call it with a constant maximum value (which is always > 0
).
imo, we should just get rid of this function and replace:
val, err := randomNum(seed)
if err != nil {
return err
}
...
with:
val := big.NewInt(rand.Int64N(seed))
...
if totalWeight == 0 { | ||
return txType{}, errZeroTotalWeight | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a comment that this ensures that UintN
won't panic.
{ | ||
name: "zero self transfer", | ||
weight: 1000, | ||
maxFeeCap: big.NewInt(4761904), // equiavelent to 100 ETH which is the maximum value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like 100 * 1_000_000_000 / params.TxGas
would have been more readable
Signed-off-by: rodrigo <[email protected]> Signed-off-by: Elvis <[email protected]> Co-authored-by: Michael Kaplan <[email protected]> Co-authored-by: Rodrigo Villar <[email protected]> Co-authored-by: rodrigo <[email protected]> Co-authored-by: Elvis <[email protected]>
Why this should be merged
This PR adds a load test which can be used to test the C-Chain.
How this works
Configuration of the load test can be found in
./tests/load/c/main/main.go
How this was tested
Need to be documented in RELEASES.md?
N/A