Skip to content
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

Merged
merged 4 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions test/txsim/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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().
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SuppressLogs().

Copy link
Contributor Author

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

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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
conn, err := grpc.NewClient(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))
conn, err := grpc.Dial(grpcAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))

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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)
}
82 changes: 82 additions & 0 deletions test/txsim/upgrade.go
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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]
}
Comment on lines +35 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 account
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.

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]
}
// 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]
}


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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
}

// 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
}
Loading