near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.32k stars 622 forks source link

Create ProfileData per receipt #4560

Closed matklad closed 3 years ago

matklad commented 3 years ago

The issue was uncovered in https://near.zulipchat.com/#narrow/stream/295306-dev-contract-runtime/topic/data.20receipt.20creation.20fee/near/246980086.

Today, we store ProfileData (gas profiling record) in ApplyState:

https://github.com/near/nearcore/blob/6cc390944660354fdd28e1f52071ea03a5d5634f/core/primitives/src/runtime/apply_state.rs#L43-L45

This means that we collect a cumulative gas profile per block, which doesn't make sense: we need profile per ExecutionOutccome.

I think the following refactor is required:

Before we do this, it makes sense to take a look at git history and understand, why this wasn't done in the first place -- the current design of ProfileData, which uses Rc and is threaded via ApplyState feels very roundabout to me (as opposed to creating a fresh profile in gas counter), I might be missing some rationale for that.

matklad commented 3 years ago

@ailisp I wonder if we want to do this refactor before #4438?

ailisp commented 3 years ago

I found this issue too that profile data stored is actually gas used per block. Research history I found this PR: https://github.com/near/nearcore/pull/3730. There wasn't a rationale on why ProfileData is added to ApplyState. I guess the reason is ProfileData is intended to use on the special local debug node (evidence: there was special feature, not compiled by default). And when use that way, there's only one transaction on one block so added to ApplyState is fine. And now we need it per receipt, willemneal please added if there's other reason we don't see that prevent us to have per receipt profile data. Thanks!

matklad commented 3 years ago

closed by #4582 I think, thanks @ailisp !