feat: add max-msg-num parameter for batch broadcast in chain configs #1470
Conversation
There was a problem hiding this comment.
Hey @mmsqe thanks for the contribution!
What is the need you have for limiting the max number of messages to broadcast?
Are you hitting block size issues? something else? Just a nice to have feature? Any extra context would be helpful here.
Impl looks good.
I use |
| if maxMsgNum < 2 && maxMsgNum != 0 { | ||
| maxMsgNum = 2 | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yes, do we mean we return error instead of auto correct if user wrongly config as 1?
There was a problem hiding this comment.
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.
| if len(batch) >= int(maxMsgNum) && maxMsgNum != 0 { | ||
| go mp.sendBatchMessages(ctx, src, dst, batch) | ||
| batch = nil | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yea, batch <= maxMsgNum
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
actually it's to avoid confusion if there's need for BroadcastMaxMsgBytes later
There was a problem hiding this comment.
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?
| @@ -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"` | |||
There was a problem hiding this comment.
can we rename this to BroadcastMaxMsg, i think the name is more clear with regards to what the value is used for
|
I apologize for the amount of time it's taken to follow up on this. I've moved into a newer role that has led to me having less bandwidth for maintaining the relayer and we have only recently begun to onboard new developers for the role. Thanks for sticking with us and continuing to make contributions! |
No description provided.