Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions prdoc/pr_10192.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
title: 'revive: Fix dust & child contract calls'
doc:
- audience: Runtime Dev
description: |-
When transferring to self we should just return early as it's a noop.
Not doing so cause bug in `transfer_with_dust`

as we do
```
from.dust -= from.dust
to.dust += to.dust
```

We end up with a value of dust - planck (that we burnt from to create dust amount) on the account

fix https://github.com/paritytech/contract-issues/issues/211
crates:
- name: pallet-revive-fixtures
bump: patch
- name: pallet-revive
bump: patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
contract CallSelfWithDust {
function f() external payable {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check, if f here contained some code, this code will be executed and the only thing that we have a NOOP for is just the transfers, is that correct?

Copy link
Copy Markdown
Member

@xermicus xermicus Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this breaks the EVM semantics.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check, if f here contained some code, this code will be executed and the only thing that we have a NOOP for is just the transfers, is that correct?

yes we short-circuit the logic of the transfer_with_dust fn when from = to or value = 0 the rest does not change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right we should always check that from.balance > amount is that what you are suggesting @xermicus

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind. I saw that there is already a zero balance check in the transfer function and short-circuited that this is implemented in logic further up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation is wrong i believe we should ensure that the account has enough balance to do the transfer even if to == from

Will patch it when i het back

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the logic is quite complex with dust. Maybe needs some more tests for these corner cases.


function call() public payable {
this.f{value: 10}();
}
}
4 changes: 4 additions & 0 deletions substrate/frame/revive/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1521,6 +1521,10 @@ where
storage_meter: &mut storage::meter::GenericMeter<T, S>,
exec_config: &ExecConfig<T>,
) -> DispatchResult {
if from == to || value.is_zero() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this fix applies to pallet-revive as a whole and therefore it would fix this issue for EVM and PolkaVM. Is that correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I guess we could check by whitelisting this test in your CI PR

#10071

let me update the test here we can make it run against pvm as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it applies to both backends. The PVM syscall handler isn't supposed to handle low level details like that.

return Ok(())
}

fn transfer_with_dust<T: Config>(
from: &AccountIdOf<T>,
to: &AccountIdOf<T>,
Expand Down
23 changes: 23 additions & 0 deletions substrate/frame/revive/src/tests/sol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,26 @@ fn upload_evm_runtime_code_works() {
assert_eq!(55u64, decoded, "Contract should correctly compute fibonacci(10)");
});
}

#[test]
fn dust_work_with_child_calls() {
use pallet_revive_fixtures::CallSelfWithDust;
let (code, _) = compile_module_with_type("CallSelfWithDust", FixtureType::Solc).unwrap();

ExtBuilder::default().build().execute_with(|| {
let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
let Contract { addr, .. } =
builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();

let value = 1_000_000_000.into();
builder::bare_call(addr)
.data(
CallSelfWithDust::CallSelfWithDustCalls::call(CallSelfWithDust::callCall {})
.abi_encode(),
)
.evm_value(value)
.build_and_unwrap_result();

assert_eq!(crate::Pallet::<Test>::evm_balance(&addr), value);
});
}
Loading