blockchain: Implement EIP-2028 calldata pricing

hyunsooda commented 1 month ago

Proposed changes

The gas calculation for EIP-2028 has been integrated into IntrinsicGasPayload(). The expected gas values are also being adjusted with precise HF state.

Types of changes

Please put an x in the boxes related to your change.

[ ] Bugfix
[ ] New feature or enhancement
[ ] Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[ ] I have read the CONTRIBUTING GUIDELINES doc
[ ] I have read the CLA and signed by comment I have read the CLA Document and I hereby sign the CLA in first time contribute
[ ] Lint and unit tests pass locally with my changes ($ make test)
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] I have added necessary documentation (if appropriate)
[ ] Any dependent changes have been merged and published in downstream modules

Related issues

Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

hyunsooda commented 1 month ago

@ian0371 The file https://github.com/kaiachain/kaia/blob/dev/tests/hard_fork_test.go#L52 has a test which read previously stored test files (genesis.json, b1.rlp and b2.rlp). The Istanbul HF has been absent to genesis.json. So, I add it first into it and regenerate b1.rlp and b2.rlp. This modification was necessary because the semantic of IntrinsicGasPayload() has been changed in this PR, from unconditionally payment of 100gas per data to EIP2028. Thus, without this change, the test was failed because it reaches to here

ian0371 commented 1 month ago

@hyunsooda Got it. Although it seems the new genesis.json also doesn't have istanbulCompatibleBlock. Is this ok?

hyunsooda commented 1 month ago

I thought that I've modified it, but did not actually and tried to understand how it was twisted.

The Istanbul or Shanghai fork number had not been specified to genesis.json.
IntrinsicGasPayload() unconditionally have been subtracting 100gas per data.
both b1.rlp and b2.rlp were generated 5y ago (based on the klaytn's initial commit)

Given the three facts above, IntrinsicGasPayload() should subtract 100gas per data, but the PR changes it. It was the root cause and now I reverted genesis.json which is not necessarily one to be modified. Instead, I regenerated b1.rlp and b2.rlp so that the generated files would take the current behavior of IntrinsicGasPayload().

ian0371 commented 1 month ago

IntrinsicGasPayload() unconditionally have been subtracting 100gas per data.

Could you show me where? I can't find it...

hyunsooda commented 1 month ago

@ian0371 blockchain/types/tx_internal_data_account_creation.go

ian0371 commented 1 month ago

I don't see any "subtraction" in the link or in this PR. What am I missing?

In addition, do you mean that IntrinsicGas() behavior is different now compared? I'm thinking that b1.rlp is a legacy block, and this PR's IntrinsicGas() cannot handle it (it's a pre-Istanbul case, right?). Doesn't this mean that it would break the backward compatibility? Sorry but I'm a little confused.

hyunsooda commented 1 month ago

I'm sorry. Subtraction seems corrupt you. Let me correct this word to payment.

In addition, do you mean that IntrinsicGas() behavior is different now compared? I'm thinking that b1.rlp is a legacy block, and this PR's IntrinsicGas() cannot handle it (it's a pre-Istanbul case, right?).

Yes.

Doesn't this mean that it would break the backward compatibility?

Yes, it calculates differently. Can you refer to the first problem in the PR description? Directly calling the IntrinsicGasPayload() does not check Istanbul HF and directly pays 100gas per data. However, it is different if the IntriscGas() is called. This checks HF state and calls IntrinsicGasPayload() if Istanbul HF is on.

Now I think that I was wrong. This direction was initiated from the thought that wanted to resolve the wrong behavior implemented initially. But for the backward compatibility, I've should not touch them damn.. It's inevitable to leave as it is. If this is confirmed, let me rework to keep the backward compatability.

ian0371 commented 1 month ago

Ohh I see. I think calling IntrinsicGasPayload() in the first place was a very big mistake.... only IntrinsicGas() must've been used.. It feels like a headache now to refactor it

ian0371 commented 1 month ago

How about we leave the ingrate function as in the PR, unify the gas calculation after prague, and adjust (subtract) gas from the callers pre-Prague for those who call IntrinsicGasPayload() directly?

as-is

gas = IntrinsicGasPayload()

to-be

gas = IntrinsicGas()
if prePrague:
    gas -= 100

Let's discuss with @blukat29 in the next scrum.

hyunsooda commented 1 month ago

@ian0371 @blukat29 Simplified the logic by inserting EIP-2028 calculation into IntrinsicGasPayload() and updated the PR description.

kaiachain / kaia