rewrote tests in selfdestruct_post_cancun

[0xRVE]

fixes paritytech/contract-issues#209

These three lines make it so that the existential deposit is transferred as regular balance (as opposed to deposit).
https://github.com/paritytech/polkadot-sdk/blob/ef07f24b0cd49a52d1a5a0f3a42ea8d714187f18/substrate/frame/revive/src/exec.rs#L1225

let ed = <Contracts<T>>::min_balance();
frame.nested_storage.record_charge(&StorageDeposit::Charge(ed))?;
<Contracts<T>>::charge_deposit(None, origin, account_id, ed, self.exec_config)
	.map_err(|_| <Error<T>>::StorageDepositNotEnoughFunds)?;

Because of this the beneficiary receives 1 ether + ED which fails our assertion (we assert that the beneficiary received 1 ether). NB: this only happens if the contract is destroyed in the same transaction as it was created.
So I simply changed the assertion to 1 ether + ED.

Another issue is these two tests:

    function test_create_and_terminate() public {
        deploy_create();
        assert(exists());
        test_balance_after_create();
        terminate();
        test_balance_after_selfdestruct();
    }

    function test_create2_and_terminate() public {
        deploy_create2();
        assert(exists());
        test_balance_after_create();
        terminate();
        test_balance_after_selfdestruct();
    }

At the time we call test_balance_after_selfdestruct the contract has not been destroyed yet because the transaction is still going.
I removed this type of test and check the balances in a separate transaction.

[xermicus]

Why don't we fix the underlying issue instead?

[0xRVE]

Why don't we fix the underlying issue instead?

there is no underlying issue. everything works as it should.

[xermicus]

I can't follow. If everything works as it should:

• Why does the test need to be changed? The assertions you remove here in the test look like they should not fail otherwise we introduce a difference.
• Why you write in the issue the existential deposit is transferred as regular balance?

[xermicus]

When implementing this PR I found that SELFDESTRUCT is broken even in latest master. Seeing tests being changed to "fix" something doesn't spark confidence in me.

What should be done is to fix the pallet such that SELFDESTRUCT does send:

• The free balance away and;
• Return the ED back to the origin if the account is reaped (SELFDESTRUCT in same transaction it was created).

If this was implemented correctly in the pallet, the differential test in revive would work fine. How is changing a test going fix the differential test in revive? Or am I completely out of the picture here?

[0xRVE]

• Why does the test need to be changed? The assertions you remove here in the test look like they should not fail otherwise we introduce a difference.

1. we are testing if beneficiary received 1 ether but if the contract was truly destroyed beneficiary received 1 ether + ED
2. we cannot do these two things in the same transaction: selfdestruct, check if beneficiary received balance

• Why you write in the issue the existential deposit is transferred as regular balance?

Because I thought the ED was a deposit charge on the meter and was supposed to be refunded to the origin.

[xermicus]

but if the contract was truly destroyed beneficiary received 1 ether + ED

Because I thought the ED was a deposit charge on the meter and was supposed to be refunded to the origin.

So this is the bug?

[0xRVE]

Because I thought the ED was a deposit charge on the meter and was supposed to be refunded to the origin.

So this is the bug?

Either I misunderstood the way it is supposed to work or it is a bug.

So two issues need to be decided:

1. in case of true contract destruction, do we send ED to origin or to beneficiary?
2. do we send funds to beneficiary before the end of the tx that executed selfdestruct?

[xermicus]

The ED is supposed to be returned to from wherever it came from. The ED being the ED in one contract and then balance to another makes no sense. In EVM you can't turn gas into ETH like that as far as I am aware.

If it is deducted from the meter it should be returned to the meter.

If it can't be returned to the meter because the meter is one dimensional and this creates an attack vector (because ref_time was already spent but now you found additional gas be reclaiming ED). It may be returned to the origin (which is kind of like refunded gas) or burned.

The goal is to achieve high compatibility with EVM. I am unaware of the decision to intentionally break SELFDESTRUCT.

[0xRVE]

The ED is supposed to be returned to from wherever it came from.

If it is deducted from the meter it should be returned to the meter.

If it can't be returned to the meter because the meter is one dimensional and this creates an attack vector. It may be returned to the origin (which is kind of like refunded gas) or burned.

The goal is to achieve high compatibility with EVM. I am unaware of the decision to intentionally break SELFDESTRUCT.

I am not 100% convinced the ED was returned to the origin before this PR paritytech/polkadot-sdk#9699

[xermicus]

If termination doesn't happen in the same TX it doesn't need to be returned anywhere. Otherwise, why not just return it to the meter or TX origin (not sure about the details - if it can be used to buy back other resources it seems attackable)?

[athei]

Hey. I am currently fixing a few things about SELFDESTRUCT. Will open a PR soon. There were a few things broken that I will fix to be like this:

• The free balance (without ed) should be send away right away to the beneficiary and not be delayed like the contract deletion.
• The ed and storage deposit will be send away only when terminating but to the origin (delayed).
• The scheduling of the terminate needs to be reverted if the scheduling frame reverts.
• SELFDESTRUCT should be allowed inside the constructor. The issuing contract will exist as account without code for the remainder of the transaction.
• The terminate pre-compile should revert if delegate called or its caller was delegate called. This is just my opinion but if we are changing semantics we can might as well add some security. We are increasing the attack surface by allowing the destruction of any contract (not only created in the current tx).

[0xRVE]

Closing this PR as @athei will change behaviour of selfdestruct that will make the tests pass