-
Notifications
You must be signed in to change notification settings - Fork 297
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
feat: add upgrade sequence to tx sim #3890
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -12,12 +12,17 @@ import ( | |||||
|
||||||
"github.com/celestiaorg/celestia-app/v3/app" | ||||||
"github.com/celestiaorg/celestia-app/v3/app/encoding" | ||||||
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2" | ||||||
v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3" | ||||||
"github.com/celestiaorg/celestia-app/v3/test/txsim" | ||||||
"github.com/celestiaorg/celestia-app/v3/test/util/testnode" | ||||||
"github.com/cosmos/cosmos-sdk/crypto/keyring" | ||||||
sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
"google.golang.org/grpc" | ||||||
"google.golang.org/grpc/credentials/insecure" | ||||||
|
||||||
blob "github.com/celestiaorg/celestia-app/v3/x/blob/types" | ||||||
signaltypes "github.com/celestiaorg/celestia-app/v3/x/signal/types" | ||||||
bank "github.com/cosmos/cosmos-sdk/x/bank/types" | ||||||
distribution "github.com/cosmos/cosmos-sdk/x/distribution/types" | ||||||
staking "github.com/cosmos/cosmos-sdk/x/staking/types" | ||||||
|
@@ -152,3 +157,50 @@ func Setup(t testing.TB) (keyring.Keyring, string, string) { | |||||
|
||||||
return cctx.Keyring, rpcAddr, grpcAddr | ||||||
} | ||||||
|
||||||
func TestTxSimUpgrade(t *testing.T) { | ||||||
if testing.Short() { | ||||||
t.Skip("skipping TestTxSimUpgrade in short mode.") | ||||||
} | ||||||
cp := app.DefaultConsensusParams() | ||||||
cp.Version.AppVersion = v2.Version | ||||||
cfg := testnode.DefaultConfig(). | ||||||
WithTimeoutCommit(300 * time.Millisecond). | ||||||
WithConsensusParams(cp). | ||||||
WithFundedAccounts("txsim-master") | ||||||
cctx, _, grpcAddr := testnode.NewNetwork(t, cfg) | ||||||
|
||||||
// updrade to v3 at height 20 | ||||||
sequences := []txsim.Sequence{ | ||||||
txsim.NewUpgradeSequence(v3.Version, 20), | ||||||
} | ||||||
|
||||||
opts := txsim.DefaultOptions(). | ||||||
// SuppressLogs(). | ||||||
WithPollTime(time.Millisecond * 100) | ||||||
|
||||||
err := txsim.Run( | ||||||
cctx.GoContext(), | ||||||
grpcAddr, | ||||||
cctx.Keyring, | ||||||
encoding.MakeConfig(app.ModuleEncodingRegisters...), | ||||||
opts, | ||||||
sequences..., | ||||||
) | ||||||
require.NoError(t, err) | ||||||
|
||||||
conn, err := grpc.NewClient(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect function call: The function 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
Suggested change
|
||||||
require.NoError(t, err) | ||||||
defer conn.Close() | ||||||
|
||||||
querier := signaltypes.NewQueryClient(conn) | ||||||
|
||||||
// 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 | ||||||
Comment on lines
+198
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm working on that right now actually
Comment on lines
+198
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.Eventually(t, func() bool { | ||||||
upgradePlan, err := querier.GetUpgrade(cctx.GoContext(), &signaltypes.QueryGetUpgradeRequest{}) | ||||||
require.NoError(t, err) | ||||||
return upgradePlan.Upgrade != nil && upgradePlan.Upgrade.AppVersion == v3.Version | ||||||
}, time.Second*20, time.Millisecond*100) | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,82 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
package txsim | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"math/rand" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
signaltypes "github.com/celestiaorg/celestia-app/v3/x/signal/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/cosmos/cosmos-sdk/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/gogo/protobuf/grpc" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var _ Sequence = &UpgradeSequence{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const fundsForUpgrade = 100_000 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// UpgradeSequence simulates an upgrade proposal and voting process | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type UpgradeSequence struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
voted map[string]bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
height int64 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
version uint64 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
account types.AccAddress | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
hasUpgraded bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func NewUpgradeSequence(version uint64, height int64) *UpgradeSequence { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return &UpgradeSequence{version: version, height: height, voted: make(map[string]bool)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (s *UpgradeSequence) Clone(_ int) []Sequence { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
panic("cloning not supported for upgrade sequence. Only a single sequence is needed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using Using 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// 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] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+35
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the comment to reflect the function's behavior. The comment indicates that Apply this diff to correct the comment: -// this is a no-op for the upgrade sequence
+// Initialize the upgrade sequence by allocating an account Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (s *UpgradeSequence) Next(ctx context.Context, querier grpc.ClientConn, _ *rand.Rand) (Operation, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if s.hasUpgraded { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Operation{}, ErrEndOfSequence | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stakingQuerier := stakingtypes.NewQueryClient(querier) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
validatorsResp, err := stakingQuerier.Validators(ctx, &stakingtypes.QueryValidatorsRequest{}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Operation{}, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if len(validatorsResp.Validators) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Operation{}, errors.New("no validators found") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+55
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Consider modifying the code to select a random validator who has not yet voted. Additionally, break the loop after assigning 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 -// 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// if all validators have voted, we can now try to upgrade. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if msg == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
msg = signaltypes.NewMsgTryUpgrade(s.account) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
s.hasUpgraded = true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
delay := uint64(0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// apply a delay to the first sequence only | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if len(s.voted) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
delay = uint64(s.height) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Operation{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Msgs: []types.Msg{msg}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Delay: delay, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
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.
I uncommented this in another PR I'm working on