Fix incorrect balance when CreateContract is used in block-stm

[cffls]

Description

CreateContract was a new function introduced from upstream merge after v1.5.0. When CreateContract is called, it should also call mvRecordWritten to ensure the state object is in the live set of statedb. Otherwise, some information, e.g. balance of the account, might get lost in the rest of opcodes in that txn.

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Changes only for a subset of nodes

Nodes audience

In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)

Checklist

• I have added at least 2 reviewer or the whole pos-v1 team
• I have added sufficient documentation in code
• I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
• Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
• Includes RPC methods changes, and the Notion documentation has been updated

Cross repository changes

• This PR requires changes to heimdall
  • In case link the PR here:
• This PR requires changes to matic-cli
  • In case link the PR here:

Testing

• I have added unit tests
• I have added tests to CI
• I have tested this code manually on local environment
• I have tested this code manually on remote devnet using express-cli
• I have tested this code manually on mumbai/amoy
• I have created new e2e tests into express-cli

Manual tests

Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

[manav2401]

The fix LGTM. I think the upstream merge introduced a few more changes and we should address them (not necessarily in this PR).

1. In the ApplyMVWriteSet function, this change was made during the upstream merge.

case SuicidePath:
-		stateObject := sr.getDeletedStateObject(addr)
-		if stateObject != nil && stateObject.deleted {
+		stateObject := sr.getStateObject(addr)
+		if stateObject != nil {
			s.SelfDestruct(addr)
		}

This was done because getDeletedStateObject function was removed and getStateObject was used instead. As the code suggests above, we checked the deleted field of state object and then did self destruct. The new definition of getStateObject says that it will return nil if object was not found or deleted in this execution context. But, the newly added logic during upstream merge will proceed to self destruct if stateObject != nil which seems incorrect as it means that object wasn't deleted actually. The check should be stateObject == nil instead.

Just wanted to confirm this because I wonder why any block until now has not failed.

2. A minor nit: CreateAccount function calls createObject which internally runs the below code.

MVWrite(s, blockstm.NewAddressKey(addr))

The same thing is also executed in CreateAccount again which seems redundant. Nothing major, but it can be removed and the TODO can also be resolved.

3. Rest of the TODOs are about adding reason for balance change. I think they can also be resolved to keep the code clean.

[cffls]

Thanks for the review!

1. This is a nice catch, but I think the current code is correct. Let's analyze SelfDestruct in two scenarios: a) pre-EIP-6780, and b) post-EIP-6780.
   a). In pre EIP-6780 era, a previously created account could be self-destructed by a new txn. If an address has a SelfDestruct in the write list, we know the state object existed and it should be self-destructed. The code seems to align with this.
   b). After EIP-6780, a previously created account could not be self-destructed in a new txn after, and we only need to consider when the object is created and self-destructed in the same txn. In this case, the object returned by sr.getStateObject will be nil, see this code block. It is important to understand that ApplyMVWriteSet applies the writes to the final statedb, named s in the code block, that hasn't been written by the current txn beforehand. Therefore, we won't need to do anything to the final statedb in this case.
2. Makes sense, will remove.
3. Agreed, will remove those todos.

[manav2401]

Thanks for the explanation. I wasn't aware of the full context.