feat(estimator): Send/exec action costs #8251
Conversation
Add action cost estimations that measure send and exec separately in isolation from all other costs. These new costs do not replace existing estimations. They are only used to inform the decision for send/exec gas split, which today is almost always fifty-fifty. The new estiamtions show that often times the send costs should be cheaper. But we have to be careful before we reduce any costs because we unfortunately ignore network costs in all our estimations. (See near#8214.) The main purpose of the new estimations is to verify the gas cost strategy for meta transactions. (See near#8114.) There, we execute the send step twice, so we want to ensure this side of the action cost is not undercharged. It seems to be overcharged at the moment, so we should be safe to move forward with the proposed strategy.
| .add_action(create_account_action()) | ||
| .predecessor(AccountRequirement::SameAsSigner) | ||
| .receiver(AccountRequirement::SubOfSigner) | ||
| .inner_iters(1) // creating account works only once in a receipt |
There was a problem hiding this comment.
Wouldn’t this work okay if SubOfSigner generated random sub-AccountIds? Similarly for other actions below that use SameAsSigner requirement – all the self.inner_iter actions could've been signed by different signers. I imagine this doesn't affect the cost all that much?
There was a problem hiding this comment.
Inner iterations generate more actions per receipt. Actions inside a receipt must share the same receiver, because that's defined on the receipt and not on the action. That's why outer iterations (execute multiple transactions) works fine but not inner iterations (execute multiple actions).
There was a problem hiding this comment.
I guess the easiest way to show this is by looking at the definition of the action:
pub struct CreateAccountAction {}
It literally has no args, no account name and no keys. A create account action just creates the account for the receiver, and then executes the following actions on behalf of that account. The following actions don't even require access key checks. Usually create account is therefore paired with an add key action, which bootstraps access keys to the account. Otherwise, the account will be locked immediately.
| /// Besides account id requirements, the builder also accepts a few other | ||
| /// settings. Most importantly, a vector of actions. This is what ultimately | ||
| /// defines the workload to be executed. |
There was a problem hiding this comment.
These typically are better off documented on each method. That way if the method to add a vector of actions is changed or removed, this top-level comment does not go stale by default. As more methods are added, the parent comment would also stay fresher if it doesn’t call out specific builder methods.
There was a problem hiding this comment.
Good point. I moved parts down and I generally extended the docs on the methods.
| .predecessor(AccountRequirement::SameAsSigner) | ||
| .receiver(AccountRequirement::SameAsSigner) |
There was a problem hiding this comment.
Nit: could make a constructor to abstract this common pattern (i.e. if new sets these to RandomUnused, a new new_sir method could set these to SameAsSigner and remove a fair bit of duplication.)
There was a problem hiding this comment.
Yes I like that a lot, done.
| ActionEstimation::new(ctx) | ||
| .add_action(stake_action()) | ||
| .predecessor(AccountRequirement::SameAsSigner) | ||
| .receiver(AccountRequirement::RandomUnused) |
There was a problem hiding this comment.
This is superfluous, isn’t it? Receiver defaults to RandomUnused. I’m more curious why this estimation is being made at all if the action is only sender-is-receiver-compatible?
There was a problem hiding this comment.
Yes, superfluous, I removed it.
For the second question, I now added a comment:
/// This is not a useful action, as staking only works with sender = receiver.
/// But since this fails only in the exec step, we must still charge a fitting
/// amount of gas in the send step.
| let base = ActionEstimation::new(ctx) | ||
| .add_action(add_fn_access_key_action(ActionSize::Max)) | ||
| .predecessor(AccountRequirement::SameAsSigner) | ||
| .inner_iters(1) | ||
| .receiver(AccountRequirement::SameAsSigner) | ||
| .apply_cost(&mut ctx.testbed()); |
There was a problem hiding this comment.
Any reason this is open-coded rather than delegating to add_function_call_key_base_exec?
Alternatively, this could remain open-coded, but could the builder be reused between this and the next invocation, as in:
let base_builder = ActionEstimation::new(ctx)
.add_action(add_fn_access_key_action(ActionSize::Max))
.predecessor(AccountRequirement::SameAsSigner)
.inner_iters(1)
.receiver(AccountRequirement::SameAsSigner);
let base = base_builder.apply_cost(&mut ctx.testbed());
let total = base_builder
.add_action(delete_key_action())
.inner_iters(100)
.apply_cost(&mut ctx.testbed());There was a problem hiding this comment.
Yes I like the second alternative, it keeps the code local. Updated accordingly.
| } | ||
|
|
||
| pub(crate) fn function_call_base_exec(ctx: &mut EstimatorContext) -> GasCost { | ||
| ActionEstimation::new(ctx) |
There was a problem hiding this comment.
In some cases the exec estimation uses a SiR action, and in others it uses the non-SiR one. Any reason for the discrepancy (AFAIU all methods that are non-SiR compatible, are also SiR compatible, right?) I’d say either make it consistent everywhere or document what the preference is and why pick one over the other.
There was a problem hiding this comment.
Yes, you are right, we can use new_sir for all exec estimations. It really shouldn't matter for the execution, so let's just be consistent.
Thanks for the great review on this PR, it was very helpful!
Add action cost estimations that measure send and exec separately in isolation from all other costs. These new costs do not replace existing estimations. They are only used to inform the decision for send/exec gas split, which today is almost always fifty-fifty. The new estiamtions show that often times the send costs should be cheaper. But we have to be careful before we reduce any costs because we unfortunately ignore network costs in all our estimations. (See near#8214.) The main purpose of the new estimations is to verify the gas cost strategy for meta transactions. (See near#8114.) There, we execute the send step twice, so we want to ensure this side of the action cost is not undercharged. It seems to be overcharged at the moment, so we should be safe to move forward with the proposed strategy.
Fixes a problem for daily estimations, introduced in #8251. Because account IDs were not constant, the TX sizes depended on the randomly picked indices, which caused some failures in daily estimations where more accounts were used than in local testing.
Add action cost estimations that measure send and exec separately in
isolation from all other costs.
These new costs do not replace existing estimations.
They are only used to inform the decision for send/exec gas split,
which today is almost always fifty-fifty. The new estiamtions show that
often times the send costs should be cheaper. But we have to be careful
before we reduce any costs because we unfortunately ignore network costs
in all our estimations. (See #8214.)
The main purpose of the new estimations is to verify the gas cost
strategy for meta transactions. (See #8114.) There, we execute the
send step twice, so we want to ensure this side of the action cost is
not undercharged. It seems to be overcharged at the moment, so we should
be safe to move forward with the proposed strategy.