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(app): Integrate the block-sdk #2

Closed
wants to merge 3 commits into from

Conversation

nivasan1
Copy link

@nivasan1 nivasan1 commented Apr 2, 2024

🤜 🤛 From Skip!

In This PR

  • I integrate the block-sdk
    • I integrate the mev, free, default lanes (ordered accordingly) into the LaneMempool which will be used as the app's Mempool
    • I integrate the Prepare / ProcessProposal handlers and custom CheckTx handler
    • Modules
      • I integrate the x/auction module, and the corresponding AnteHandler for the auction module
    • I integrate the ante.IgnoreDecorator with the freeLane, and the DeductFees ante-handler: this will skip the DeductFees ante-handler for any txs that originate from the freeLane

Testing

  • We've tested this integration against our own set of load-tests. These tests, spin up a network of 5 nodes (3 val, 1 seed, 1 full). Perform various perturbations against them (killing, restarting, disconnecting from the network, etc.), and periodically send a load of 350 tx/sec for 90s. The output from one of our trials is here, happy to go through these in more depth / review the methodology.
  • In order for the e2e tests to work, I had to modify the ante_no_seq ante-handlers to ignore signature verification, while incrementing sequence #s, I leave a comment in the section describing why.

Let's Discuss

  • I chose to use stable defaults here (tx-capacity of each lane) and here (block-space allocated to each lane), let me know if any of the parameters are confusing or should be changed
  • regarding the auction-module
    • I imagine that the full integration requires the v23 upgrade-handler (store-migration, genesis-state / parameters, etc.), I am happy to implement those as a separate PR if that is desired
    • Auction module's parameters are here, take a look at these or we can go ahead w/ the defaults, some things to consider
      • What denomination would we like the auction to accept (there can only be a single denom accepted)
      • What would be the desired max bundle size (maximal # of txs that a bid can include)
      • What address would we like the auction revenue to be sent to? How much of it should we give to proposers?
  • re the free-lane: We currently are matching the distributiontypes.WithdrawDelegatorRewards tx, the process of adding more txs to this lane is simple, we just need to identify which types we want!

temp

update branch to latest osmo

remove load-test modifications

set lane block-space

update block-sdk to latest release

update go versions in gomod / work files

update match handler

docs

nop process proposal

nit
Copy link

github-actions bot commented Apr 2, 2024

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

ante.NewSetPubKeyDecorator(ak), // SetPubKeyDecorator must be called before all signature verification decorators
ante.NewValidateSigCountDecorator(ak),
ante.NewSigGasConsumeDecorator(ak, sigGasConsumer),
ante.NewSigVerificationDecorator(ak, signModeHandler),
// ante.NewIncrementSequenceDecorator(ak),
Copy link
Author

Choose a reason for hiding this comment

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

switching to removing the SigVerificationDecorator as the block-sdk introduces the PriorityNonceMempool, which will over-write txs w/ the same nonce, hence it's important that we keep the nonce-increment, while removing the requirement that all txs have the nonces checked + signatures verified

Comment on lines +460 to +463
// we use a no-op ProcessProposal, this way, we accept all proposals in avoidance
// of liveness failures due to Prepare / Process inconsistency. In other words,
// this ProcessProposal always returns ACCEPT.
app.SetProcessProposal(baseapp.NoOpProcessProposal())

Choose a reason for hiding this comment

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

Note: this is set to no-op because the upstream priority nonce mempool that the block SDK utilizes under the hood does not natively support multi-fee denoms. As such the ordering may not be deterministic between proposal creation and verificaiton. This would require an integration with osmosis prices / slinky to turn on the ProcessProposalHandler.

app/lanes.go Outdated
Comment on lines 107 to 118
// WithdrawStakingRewardsMatchHandler is a match handler that matches transactions that contain a withdraw staking rewards message
func WithdrawStakingRewardsMatchHandler() base.MatchHandler {
return func(ctx sdk.Context, tx sdk.Tx) bool {
for _, msg := range tx.GetMsgs() {
if _, ok := msg.(*distrtypes.MsgWithdrawDelegatorReward); ok {
return true
}
}

return false
}
}

Choose a reason for hiding this comment

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

Note: This means that any transaction including a MsgWithdrawDelegatorReward will be considered free. I'm assuming most delegator reward withdraw txs only include a single message?

Choose a reason for hiding this comment

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

This is worth addressing before this gets merged.

Copy link

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Apr 11, 2024
@github-actions github-actions bot closed this Apr 14, 2024
Copy link

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Apr 24, 2024
@github-actions github-actions bot closed this Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment