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

refactor bypass-min-fee-msg-types #2213

Closed
11 tasks
yaruwangway opened this issue Feb 16, 2023 · 9 comments · Fixed by #2218
Closed
11 tasks

refactor bypass-min-fee-msg-types #2213

yaruwangway opened this issue Feb 16, 2023 · 9 comments · Fixed by #2218
Assignees
Labels
scope: UI Issue addressing UX changes and improvements to user interface type: tech-debt Slows down development in the long run

Comments

@yaruwangway
Copy link
Contributor

yaruwangway commented Feb 16, 2023

Summary

Presently, gaia allows certain message free of fee charge, the default setup in app.toml is:

bypass-min-fee-msg-types = ["/ibc.core.channel.v1.MsgRecvPacket", 
"/ibc.core.channel.v1.MsgAcknowledgement", 
"/ibc.core.client.v1.MsgUpdateClient"]

Nodes created using Gaiad v7.0.2 or later will have the above bypass-min-fee-msg-types as defaults in app.toml. Nodes with bypass-min-fee-msg-types = [] or missing this field in app.toml also use default bypass message types.

Bypass-msgs should not exceed gas usage of uint64(len(bypassMsgs))*MaxBypassMinFeeMsgGasUsage. MaxBypassMinFeeMsgGasUsage = 200,000.

Type

Impact

There will be more msgs from relayers free of fee charges.

Proposed Solution

The following changes are proposed:

  • Nodes with bypass-min-fee-msg-types = [], will not bypass any message, node with bypass-min-fee-msg-types missing in app.toml will use default bypass-min-fee-msg-types change bypass-min-fee-types parsing; change defaults #2092, corresponding docs need to be updated.
  • add ["/ibc.core.channel.v1.MsgTimeout", "/ibc.core.channel.v1.MsgTimeoutOnClose"] to default ypass-min-fee-msg-types
  • bypass-msg should not exceed an arbitrary MaxTotalBypassMinFeeMsgGasUsage, this MaxTotalBypassMinFeeMsgGasUsage should be smaller than half of a block's max_gas so that other messages can also get into the block. With MaxTotalBypassMinFeeMsgGasUsage, relayers can arrange the bypass-msgs to utilize the bypass setup.
    from cosmoshub-4 genesis, we can get
 "block": {
    "max_bytes": "200000",
    "max_gas": "2000000",
    "time_iota_ms": "1000"
  }

so the MaxTotalBypassMinFeeMsgGasUsage can be 1,000,000

  • add unit tests and test manually.
    • Add tests for multiple bypass msg in one tx.
    • Add tests for mixing bypass and non-bypass msg in one tx.

Discussion

  1. in the future, shall we write bypass-min-fee-msg-types and MaxTotalBypassMinFeeMsgGasUsage to the store ?

  2. there are difference voices also if we write MaxTotalBypassMinFeeMsgGasUsage to app.toml together with bypass-min-fee-msg-types


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
  • Is a spike necessary to map out how the issue should be approached?
@yaruwangway
Copy link
Contributor Author

yaruwangway commented Feb 16, 2023

Hi @jtremback, the reason I propose to move bypass-min-fee-msg-types to v9.0.1 or v8 is:
v10 will have sdk 0.47, in sdk0.47, params module is deprecated, that means globalfee will have a store migration, fee ante handler will also have a refactor as well. So I am thinking if we can move part of the minor changes earlier ?

@yaruwangway yaruwangway added the type: tech-debt Slows down development in the long run label Feb 16, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Feb 16, 2023
@yaruwangway
Copy link
Contributor Author

yaruwangway commented Feb 17, 2023

@MSalopek , do we need to fix the extra , in the bypass-min-fee-msg-types in app.tom ?

bypass-min-fee-msg-types = ["/ibc.core.channel.v1.MsgRecvPacket", 
"/ibc.core.channel.v1.MsgAcknowledgement", 
"/ibc.core.client.v1.MsgUpdateClient", ]

@yaruwangway
Copy link
Contributor Author

Hi @ancazamfir , thank you for the thoughtful input and discussion ! ❤️
just let you know also about writing MaxTotalBypassMinFeeMsgGasUsage = 1,000,000 to app.toml is to be decided. The reason is: we might not want the MaxTotalBypassMinFeeMsgGasUsage adjusted by each node.

anyway, like discussed, writing both MaxTotalBypassMinFeeMsgGasUsage and bypass-msg to store might be a future option.

@ancazamfir
Copy link

we might not want the MaxTotalBypassMinFeeMsgGasUsage adjusted by each node.

Who is "we"? How is this different than operator preference to have gas fee configurable? I am trying to understand and as I mentioned I don't know why some params are in app.toml vs in genesis.json and how a decision was reached for these?
For MaxTotalBypassMinFeeMsgGasUsage is there feedback from node operators? Or are there concerns that issues (like non-determinism) may occur due to different nodes setting this differently?

There might be some justification in this particular case but having a hardcoded param is generally not a good idea imho.

@yaruwangway
Copy link
Contributor Author

yaruwangway commented Feb 20, 2023

I discussed with @MSalopek.

How is this different than operator preference to have gas fee configurable?

do you mean setup --gas when submit tx ? tx submitter can set up this very high but they have pay, but if MaxTotalBypassMinFeeMsgGasUsage is setup high, there might be no space left for other types of tx in a block.
I can ask also node operators if they prefer to setup this value or agree to use the same.

@yaruwangway
Copy link
Contributor Author

yaruwangway commented Feb 20, 2023

Hi @mmulji-ic, for software design related feedback from node operators (like above), what is the best channel to ask, validator-verified ? thanks!

@mmulji-ic
Copy link
Contributor

mmulji-ic commented Feb 21, 2023

I slacked you some channels, if you don't get enough feedback, I can reach out to validators via telegram. IBC devrel would be good to get some input also.

@yaruwangway
Copy link
Contributor Author

yaruwangway commented Feb 21, 2023

Hi @ancazamfir, just to correct what i said, changing gas is state-breaking. please check here #2218 (comment)

so MaxTotalBypassMinFeeMsgGasUsage cannot stay in app.toml and this refactor need an upgrade.

@yaruwangway yaruwangway changed the title Discussion: refactor bypass-min-fee-msg-types V10: refactor bypass-min-fee-msg-types Feb 21, 2023
@yaruwangway yaruwangway changed the title V10: refactor bypass-min-fee-msg-types Gaia v10: refactor bypass-min-fee-msg-types Feb 21, 2023
@yaruwangway yaruwangway changed the title Gaia v10: refactor bypass-min-fee-msg-types efactor bypass-min-fee-msg-types Mar 3, 2023
@yaruwangway yaruwangway changed the title efactor bypass-min-fee-msg-types refactor bypass-min-fee-msg-types Mar 3, 2023
@mpoke mpoke moved this from 🩹 Triage to 📥 Todo in Cosmos Hub Mar 6, 2023
@mpoke mpoke added the scope: UI Issue addressing UX changes and improvements to user interface label Mar 6, 2023
@yaruwangway yaruwangway moved this from 📥 Todo to 🏗 In progress in Cosmos Hub Mar 27, 2023
@sainoe sainoe self-assigned this Apr 5, 2023
@yaruwangway
Copy link
Contributor Author

closed by pr #2218

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Cosmos Hub Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: UI Issue addressing UX changes and improvements to user interface type: tech-debt Slows down development in the long run
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

6 participants