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: cli: gas estimation tooling #9654

Merged
merged 4 commits into from
Nov 22, 2022
Merged

Conversation

geoff-vball
Copy link
Contributor

@geoff-vball geoff-vball commented Nov 16, 2022

Related Issues

Proposed Changes

The idea here is to have a lotus-shed command that can take a message that has already landed on chain, and replay that message using a migrated state and new actors. This will allow us to more easily test the gas changes caused by new actors.

I also add a skip-pre-migration flag to the migrate-nv17 shed command so that it can be used to migrate the state only once, for use with the new trace-gas function.

The function CallAtStateAndVersion is taken from @Kubuxu's branch feat/migrate-and-run

I've refactored Call and CallWithGas to a common function. The main difference between these calls are that Call executes the message in the given tipset, and doesn't care about gas costs. CallWithGas executed the message ON TOP of the given tipset (in the next epoch) and therefore has to compute the tipset state.

The only functional change that should be present is that Call used return ErrExpensiveFork only if the parent of a specified contained an expensive fork. Now it will return ErrExpensiveFork if any epoch from the parent to the tipset itself (including nil tipsets) contain an expensive fork. This matches what was done in CallWithGas and was just changed for simplicity.

Call will also no long default to 0 base fee.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@geoff-vball geoff-vball requested a review from a team as a code owner November 16, 2022 02:32
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch from 3c50d27 to 790e219 Compare November 16, 2022 02:33
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch from 790e219 to 867f4ee Compare November 16, 2022 02:40
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch 4 times, most recently from cc0682e to ae6a3cf Compare November 16, 2022 04:07
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch from ae6a3cf to 62fedfb Compare November 16, 2022 04:12
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Can we also get sample output from this command?

@@ -69,6 +69,10 @@ var DrandConfigs = map[DrandEnum]dtypes.DrandConfig{
ChainInfoJSON: `{"public_key":"8cda589f88914aa728fd183f383980b35789ce81b274e5daee1f338b77d02566ef4d3fb0098af1f844f10f9c803c1827","period":25,"genesis_time":1595348225,"hash":"e73b7dc3c4f6a236378220c0dd6aa110eb16eed26c11259606e07ee122838d4f","groupHash":"567d4785122a5a3e75a9bc9911d7ea807dd85ff76b78dc4ff06b075712898607"}`,
},
DrandIncentinet: {
Servers: []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to some weird initialization stuff I do, the old beacon needs that. No idea why, that was the quickest fix for hacky code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@geoff-vball Can you check if this is needed as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed in the sense that if I remove it everything breaks. Whether or not its the best way to do things another question

cli/state.go Outdated Show resolved Hide resolved
chain/stmgr/call.go Outdated Show resolved Hide resolved
chain/stmgr/call.go Outdated Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch 2 times, most recently from fdf0bb1 to e5c4aa7 Compare November 16, 2022 23:58
@geoff-vball
Copy link
Contributor Author

Example output:

Screenshot 2022-11-16 at 7 10 01 PM

@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch from e5c4aa7 to 4654350 Compare November 17, 2022 00:11
@@ -69,6 +69,10 @@ var DrandConfigs = map[DrandEnum]dtypes.DrandConfig{
ChainInfoJSON: `{"public_key":"8cda589f88914aa728fd183f383980b35789ce81b274e5daee1f338b77d02566ef4d3fb0098af1f844f10f9c803c1827","period":25,"genesis_time":1595348225,"hash":"e73b7dc3c4f6a236378220c0dd6aa110eb16eed26c11259606e07ee122838d4f","groupHash":"567d4785122a5a3e75a9bc9911d7ea807dd85ff76b78dc4ff06b075712898607"}`,
},
DrandIncentinet: {
Servers: []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

@geoff-vball Can you check if this is needed as part of this PR?

chain/stmgr/call.go Outdated Show resolved Hide resolved
chain/stmgr/call.go Outdated Show resolved Hide resolved
chain/stmgr/call.go Outdated Show resolved Hide resolved
chain/stmgr/call.go Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
cmd/lotus-shed/gas-estimation.go Outdated Show resolved Hide resolved
if ts == nil {
return xerrors.Errorf("could not find message within the last %d epochs", lookbackLimit)
}
executionTs, err := cs.GetTipsetByHeight(ctx, ts.Height()-2, ts, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

explain why -2 pls

Copy link
Contributor

Choose a reason for hiding this comment

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

That is probably question for me, no idea, that is what worked at the time :P

@arajasek
Copy link
Contributor

@geoff-vball Please add a description of the stmgr::Call refactor to the description.

@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch 6 times, most recently from 8c7dcb4 to a31b0f2 Compare November 17, 2022 19:51
@geoff-vball geoff-vball force-pushed the gstuart/gas-estimation-tooling branch from a31b0f2 to 9603500 Compare November 18, 2022 05:10
@arajasek arajasek merged commit 6067968 into master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants