Skip to content

revive: Fix dust & child contract calls #10192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pgherveou merged 12 commits into from
Nov 4, 2025
Merged

revive: Fix dust & child contract calls #10192

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
77eda1a
add test
pgherveou Nov 3, 2025
cff6c37
fix ignore transfer to self
pgherveou Nov 3, 2025
948a809
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] Nov 3, 2025
a4f6970
Merge branch 'master' into pg/issue-211
pgherveou Nov 3, 2025
fbcdeec
fix conflict
pgherveou Nov 3, 2025
fac4618
fix
pgherveou Nov 3, 2025
0d74778
fail with transferFailed when account.value < value regardless of to
pgherveou Nov 3, 2025
5b90b78
nit
pgherveou Nov 3, 2025
1d0d91d
add tests
pgherveou Nov 3, 2025
959c7d2
one more test case
pgherveou Nov 3, 2025
3d0628b
extra test
pgherveou Nov 3, 2025
b8bed4f
Merge branch 'master' into pg/issue-211
pgherveou Nov 3, 2025
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
7 changes: 7 additions & 0 deletions substrate/frame/revive/fixtures/contracts/call_self_with_dust.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
contract CallSelfWithDust {
function f() external payable {}

@0xOmarA 0xOmarA Nov 3, 2025

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?

@xermicus xermicus Nov 3, 2025 •
edited
Loading

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.

Yes this breaks the EVM semantics.

@pgherveou pgherveou Nov 3, 2025

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

@pgherveou pgherveou Nov 3, 2025

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

@xermicus xermicus Nov 3, 2025

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.

@pgherveou pgherveou Nov 3, 2025

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

@xermicus xermicus Nov 3, 2025

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
View file
Open in desktop
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() {

@0xOmarA 0xOmarA Nov 3, 2025

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?

@pgherveou pgherveou Nov 3, 2025

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

@xermicus xermicus Nov 3, 2025

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,26 @@ fn eth_contract_too_large() {
});
}
}

#[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