feat: add upgrade sequence to tx sim #3890
Conversation
cmwaters
changed the title
cmwaters
requested review from
evan-forbes and
ninabarbakadze
and removed request for
a team
WalkthroughWalkthroughThe changes introduce a new test function, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| } | ||
|
|
||
| opts := txsim.DefaultOptions(). | ||
| // SuppressLogs(). |
There was a problem hiding this comment.
| // SuppressLogs(). |
There was a problem hiding this comment.
I uncommented this in another PR I'm working on
| // We can't check that the upgrade was successful because the upgrade height is thousands of blocks away | ||
| // and even at 300 millisecond block times, it would take too long. Instead we just want to assert | ||
| // that the upgrade is ready to be performed |
There was a problem hiding this comment.
👍 good idea. As a separate issue we can explore making it easy to override the upgrade delay so that tests can configure it to be 1 block and verify the upgrade executed and app version transition from 2 -> 3.
There was a problem hiding this comment.
I'm working on that right now actually
celestia-bot
requested review from
a team and
staheri14
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func (s *UpgradeSequence) Clone(_ int) []Sequence { | ||
| panic("cloning not supported for upgrade sequence. Only a single sequence is needed") | ||
| } |
There was a problem hiding this comment.
Avoid using panic in the Clone method.
Using panic in the Clone method can lead to unexpected crashes if the method is called. Consider returning nil or an empty slice to gracefully handle the unsupported operation, or implement the method as required by the Sequence interface.
Apply this diff to avoid panicking:
func (s *UpgradeSequence) Clone(_ int) []Sequence {
- panic("cloning not supported for upgrade sequence. Only a single sequence is needed")
+ return nil // or return an empty slice []Sequence{}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *UpgradeSequence) Clone(_ int) []Sequence { | |
| panic("cloning not supported for upgrade sequence. Only a single sequence is needed") | |
| } | |
| func (s *UpgradeSequence) Clone(_ int) []Sequence { | |
| return nil // or return an empty slice []Sequence{} | |
| } |
| // this is a no-op for the upgrade sequence | ||
| func (s *UpgradeSequence) Init(_ context.Context, _ grpc.ClientConn, allocateAccounts AccountAllocator, _ *rand.Rand, _ bool) { | ||
| s.account = allocateAccounts(1, fundsForUpgrade)[0] | ||
| } |
There was a problem hiding this comment.
Update the comment to reflect the function's behavior.
The comment indicates that Init is a no-op, but the function allocates an account using allocateAccounts. Please update the comment to accurately describe the initialization logic.
Apply this diff to correct the comment:
-// this is a no-op for the upgrade sequence
+// Initialize the upgrade sequence by allocating an accountCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // this is a no-op for the upgrade sequence | |
| func (s *UpgradeSequence) Init(_ context.Context, _ grpc.ClientConn, allocateAccounts AccountAllocator, _ *rand.Rand, _ bool) { | |
| s.account = allocateAccounts(1, fundsForUpgrade)[0] | |
| } | |
| // Initialize the upgrade sequence by allocating an account | |
| func (s *UpgradeSequence) Init(_ context.Context, _ grpc.ClientConn, allocateAccounts AccountAllocator, _ *rand.Rand, _ bool) { | |
| s.account = allocateAccounts(1, fundsForUpgrade)[0] | |
| } |
| // Choose a random validator to be the authority | ||
| var msg types.Msg | ||
| for _, validator := range validatorsResp.Validators { | ||
| if !s.voted[validator.OperatorAddress] { | ||
| msg = &signaltypes.MsgSignalVersion{ | ||
| ValidatorAddress: validator.OperatorAddress, | ||
| Version: s.version, | ||
| } | ||
| s.voted[validator.OperatorAddress] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the logic to select a random validator and prevent multiple message assignments.
The comment states that a random validator is chosen, but the code iterates over all validators, assigning msg multiple times without breaking the loop, and no randomness is involved.
Consider modifying the code to select a random validator who has not yet voted. Additionally, break the loop after assigning msg to prevent overwriting.
Apply this diff to select a random validator and break the loop:
var msg types.Msg
+var notVotedValidators []stakingtypes.Validator
for _, validator := range validatorsResp.Validators {
if !s.voted[validator.OperatorAddress] {
- msg = &signaltypes.MsgSignalVersion{
- ValidatorAddress: validator.OperatorAddress,
- Version: s.version,
- }
- s.voted[validator.OperatorAddress] = true
- }
+ notVotedValidators = append(notVotedValidators, validator)
+ }
}
+
+if len(notVotedValidators) > 0 {
+ selectedValidator := notVotedValidators[rand.Intn(len(notVotedValidators))]
+ msg = &signaltypes.MsgSignalVersion{
+ ValidatorAddress: selectedValidator.OperatorAddress,
+ Version: s.version,
+ }
+ s.voted[selectedValidator.OperatorAddress] = true
+} else {
+ // if all validators have voted, we can now try to upgrade.
+ msg = signaltypes.NewMsgTryUpgrade(s.account)
+ s.hasUpgraded = true
+}Alternatively, if randomness is not necessary, update the comment and add a break statement after assigning msg:
-// Choose a random validator to be the authority
+// Find the next validator who hasn't voted yet
var msg types.Msg
for _, validator := range validatorsResp.Validators {
if !s.voted[validator.OperatorAddress] {
msg = &signaltypes.MsgSignalVersion{
ValidatorAddress: validator.OperatorAddress,
Version: s.version,
}
s.voted[validator.OperatorAddress] = true
+ break // Prevent overwriting msg by breaking the loop
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Choose a random validator to be the authority | |
| var msg types.Msg | |
| for _, validator := range validatorsResp.Validators { | |
| if !s.voted[validator.OperatorAddress] { | |
| msg = &signaltypes.MsgSignalVersion{ | |
| ValidatorAddress: validator.OperatorAddress, | |
| Version: s.version, | |
| } | |
| s.voted[validator.OperatorAddress] = true | |
| } | |
| } | |
| // Choose a random validator to be the authority | |
| var msg types.Msg | |
| var notVotedValidators []stakingtypes.Validator | |
| for _, validator := range validatorsResp.Validators { | |
| if !s.voted[validator.OperatorAddress] { | |
| notVotedValidators = append(notVotedValidators, validator) | |
| } | |
| } | |
| if len(notVotedValidators) > 0 { | |
| selectedValidator := notVotedValidators[rand.Intn(len(notVotedValidators))] | |
| msg = &signaltypes.MsgSignalVersion{ | |
| ValidatorAddress: selectedValidator.OperatorAddress, | |
| Version: s.version, | |
| } | |
| s.voted[selectedValidator.OperatorAddress] = true | |
| } else { | |
| // if all validators have voted, we can now try to upgrade. | |
| msg = signaltypes.NewMsgTryUpgrade(s.account) | |
| s.hasUpgraded = true | |
| } |
| // We can't check that the upgrade was successful because the upgrade height is thousands of blocks away | ||
| // and even at 300 millisecond block times, it would take too long. Instead we just want to assert | ||
| // that the upgrade is ready to be performed |
There was a problem hiding this comment.
Update comment to reflect actual upgrade height
The comment mentions that the upgrade height is thousands of blocks away, but the upgrade is scheduled at block height 20 in the code. Please update the comment to accurately reflect the current upgrade height.
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| conn, err := grpc.NewClient(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) |
There was a problem hiding this comment.
Incorrect function call: grpc.NewClient does not exist
The function grpc.NewClient does not exist in the grpc package. To create a gRPC client connection, you should use grpc.Dial.
Apply this diff to fix the function call:
- conn, err := grpc.NewClient(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))
+ conn, err := grpc.Dial(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn, err := grpc.NewClient(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| conn, err := grpc.Dial(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) |
Adds the ability to use txsim to upgrade a network