feat: optimize delegate and undelegate instruction handling - #6920
feat: optimize delegate and undelegate instruction handling#6920ouy95917 wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| @Test | ||
| public void testSuicide2RejectsSelfDestructedBeneficiaryAfterFork() |
There was a problem hiding this comment.
Could we also add a pre-fork case asserting that suicide2() succeeds when VERSION_4_8_2_2 is not activated? Since the beneficiary check is invoked unconditionally and gated inside MUtil, this would explicitly protect backward compatibility before the fork.
There was a problem hiding this comment.
Thanks for pointing this out. Before VERSION_4_8_2_2 is activated, checkCPUTimeForSelfDestructedBeneficiary() is a no-op and execution continues through the existing suicide2() path. The existing testSuicideAction2() still covers the successful legacy flow, while the newly added test focuses specifically on the rejection behavior introduced after activation.
Given that the behavioral change is confined to the activated branch, we would prefer to keep the current test scope unless there is another pre-activation side effect that needs to be covered.
| } | ||
|
|
||
| @Test | ||
| public void testSuicide2RejectsSelfDestructedBeneficiaryAfterFork() |
There was a problem hiding this comment.
Could we also add a pre-fork case asserting that suicide2() succeeds when VERSION_4_8_2_2 is not activated? Since the beneficiary check is invoked unconditionally and gated inside MUtil, this would explicitly protect backward compatibility before the fork.
There was a problem hiding this comment.
Thanks. This appears to be the same concern as the discussion above. The pre-activation check returns without throwing, so the existing suicide2() behavior remains unchanged. I have added the detailed explanation in the other thread.
ouy95917
changed the title
2a4f859 to
c839702
Compare
c839702 to
25513b1
Compare
|
Just to confirm, is the self-destruct marker intentionally maintained at the repository level so that it can be observed across nested executions within the same transaction? |
|
@TonyStank911 Yes. Keeping it in the repository allows nested executions to observe the same transaction-scoped state. It also follows the existing repository commit and rollback lifecycle, so the marker is propagated or discarded together with the corresponding state changes. |
| if (parent != null) { | ||
| boolean isSelfDestructed = parent.isSelfDestructed(address); | ||
| if (isSelfDestructed) { | ||
| selfDestructCache.add(key); |
There was a problem hiding this comment.
Is caching the positive result from the parent repository intentional here? My understanding is that the self-destruct state is monotonic within the transaction, while a negative result is left uncached so that a later parent update can still be observed.
There was a problem hiding this comment.
Yes, that is the intention. Once an address is marked, it remains self-destructed for the current transaction, so caching a positive result avoids repeated parent lookups. Negative results are not cached because the parent repository may receive the marker later.
|
|
||
| private void commitSelfDestructCache(Repository deposit) { | ||
| if (deposit != null) { | ||
| selfDestructCache.forEach(key -> deposit.markSelfDestruct(key.getData())); |
There was a problem hiding this comment.
Does propagating the marker only during commit mean that it follows the same lifecycle as the other repository state, so a reverted child repository requires no separate cleanup?
There was a problem hiding this comment.
Correct. The marker is propagated to the parent only when the child repository commits. If the child execution is reverted, the child repository and its marker are discarded together.
6 participants
What does this PR do?
This PR optimizes the execution of SELFDESTRUCT, FreezeBalanceV2, UnfreezeBalanceV2, WithdrawExpireUnfreeze, and CancelAllUnfreezeV2 by exiting early in specific edge cases to improve performance.
Why are these changes required?
To streamline the related execution paths and improve processing efficiency in specific scenarios.
This PR has been tested by:
Follow up
The release and activation plans will be evaluated separately.
Extra details
The version identifier used in this PR is provisional and may be adjusted later.