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.

Sign up for GitHub

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

Jump to bottom

feat: add max-msg-num parameter for batch broadcast in chain configs #1470

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add max-msg-num parameter for batch broadcast in chain configs #1470

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
878c4cb
feat: add max-msg-num parameter for batch broadcast in chain configs
mmsqe Jun 13, 2024
0e0c6cd
Update CHANGELOG.md
mmsqe Jun 13, 2024
1fe4063
larger timeout
mmsqe Jun 13, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
* [\#1364](https://github.com/cosmos/relayer/pull/1364) Include feegrant message when calculate gas.
* [\#1390](https://github.com/cosmos/relayer/pull/1390) Avoid no concrete type registered for type URL error of EthAccount.
* [\#1455](https://github.com/cosmos/relayer/pull/1455) Allow retry for pathEnd to avoid packet message get removed before open channel.
* [\#1468](https://github.com/cosmos/relayer/pull/1468) Add max-msg-num parameter for batch broadcast in chain configs.
mmsqe marked this conversation as resolved.
Show resolved Hide resolved

## v0.9.3

Expand Down
5 changes: 5 additions & 0 deletions relayer/chains/cosmos/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type CosmosProviderConfig struct {
Slip44 *int `json:"coin-type" yaml:"coin-type"`
SigningAlgorithm string `json:"signing-algorithm" yaml:"signing-algorithm"`
Broadcast provider.BroadcastMode `json:"broadcast-mode" yaml:"broadcast-mode"`
MaxMsgNum uint64 `json:"max-msg-num" yaml:"max-msg-num"`
MinLoopDuration time.Duration `json:"min-loop-duration" yaml:"min-loop-duration"`
ExtensionOptions []provider.ExtensionOption `json:"extension-options" yaml:"extension-options"`

Expand Down Expand Up @@ -89,6 +90,10 @@ func (pc CosmosProviderConfig) BroadcastMode() provider.BroadcastMode {
return pc.Broadcast
}

func (pc CosmosProviderConfig) BroadcastMaxMsgNum() uint64 {
return pc.MaxMsgNum
}

// NewProvider validates the CosmosProviderConfig, instantiates a ChainClient and then instantiates a CosmosProvider
func (pc CosmosProviderConfig) NewProvider(log *zap.Logger, homepath string, debug bool, chainName string) (provider.ChainProvider, error) {
if err := pc.Validate(); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions relayer/chains/penumbra/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type PenumbraProviderConfig struct {
Modules []module.AppModuleBasic `json:"-" yaml:"-"`
Slip44 int `json:"coin-type" yaml:"coin-type"`
Broadcast provider.BroadcastMode `json:"broadcast-mode" yaml:"broadcast-mode"`
MaxMsgNum uint64 `json:"max-msg-num" yaml:"max-msg-num"`
Copy link
Member

Choose a reason for hiding this comment

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

can we rename this to BroadcastMaxMsg, i think the name is more clear with regards to what the value is used for

MinLoopDuration time.Duration `json:"min-loop-duration" yaml:"min-loop-duration"`
ExtensionOptions []provider.ExtensionOption `json:"extension-options" yaml:"extension-options"`
}
Expand All @@ -75,6 +76,10 @@ func (pc PenumbraProviderConfig) BroadcastMode() provider.BroadcastMode {
return pc.Broadcast
}

func (pc PenumbraProviderConfig) BroadcastMaxMsgNum() uint64 {
return pc.MaxMsgNum
}

// NewProvider validates the PenumbraProviderConfig, instantiates a ChainClient and then instantiates a CosmosProvider
func (pc PenumbraProviderConfig) NewProvider(log *zap.Logger, homepath string, debug bool, chainName string) (provider.ChainProvider, error) {
if err := pc.Validate(); err != nil {
Expand Down
8 changes: 8 additions & 0 deletions relayer/processor/message_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ func (mp *messageProcessor) trackAndSendMessages(
needsClientUpdate bool,
) error {
broadcastBatch := dst.chainProvider.ProviderConfig().BroadcastMode() == provider.BroadcastModeBatch
maxMsgNum := dst.chainProvider.ProviderConfig().BroadcastMaxMsgNum()
if maxMsgNum < 2 && maxMsgNum != 0 {
maxMsgNum = 2
}
Comment on lines +334 to +336
Copy link
Member

Choose a reason for hiding this comment

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

is there any case where it would even make sense to set this variable to a value less than 2? i'm guessing it would always need to be at least 2 since we are attempting to broadcast some msg along with the MsgUpdateClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, do we mean we return error instead of auto correct if user wrongly config as 1?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh right this check is to ensure the user chosen value is always at least 2.

Could this check then be simplified to:

if maxMsgNum < 2 {
   maxMsgNum = 2
}

Since the maxMsgNum < 2 would also account for the case of maxMsgNum == 0 since 0 < 2.

Perhaps I'm overlooking something here.

I would be okay with at least adding a log inside of the if statement when we auto correct the maxMsgNum value to 2, that way the user at least is away that they have configured an invalid value.

var batch []messageToTrack

for _, t := range mp.trackers() {
Expand All @@ -347,6 +351,10 @@ func (mp *messageProcessor) trackAndSendMessages(

if broadcastBatch && (retries == 0 || ordered) {
batch = append(batch, t)
if len(batch) >= int(maxMsgNum) && maxMsgNum != 0 {
go mp.sendBatchMessages(ctx, src, dst, batch)
batch = nil
}
Comment on lines +354 to +357
Copy link
Member

Choose a reason for hiding this comment

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

just for clarity, is batch bound in size by the value of maxMsgNum? i.e. will batch always be less than or equal to maxMsgNum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, batch <= maxMsgNum

continue
}
go mp.sendSingleMessage(ctx, src, dst, t)
Expand Down
1 change: 1 addition & 0 deletions relayer/provider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type ProviderConfig interface {
NewProvider(log *zap.Logger, homepath string, debug bool, chainName string) (ChainProvider, error)
Validate() error
BroadcastMode() BroadcastMode
BroadcastMaxMsgNum() uint64
Copy link
Member

Choose a reason for hiding this comment

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

can we just name this BroadcastMaxMsg? the suffix of Num feels redundant given the type is an integer and dropping the suffix is a bit less verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it's to avoid confusion if there's need for BroadcastMaxMsgBytes later

Copy link
Member

Choose a reason for hiding this comment

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

that seems fair to me but perhaps it is a premature optimization, i guess i'm not strongly opinionated on this.
@Reecepbcups any thoughts on this?

}

type RelayerMessage interface {
Expand Down
Loading