Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
687 changes: 352 additions & 335 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ revive-strategy = { path = "crates/revive-strategy" }
revive-utils = { path = "crates/revive-utils" }

# polkadot-sdk
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master", features = [
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage", features = [
"experimental",
"runtime",
"polkadot-runtime-common",
Expand Down
6 changes: 3 additions & 3 deletions crates/anvil-polkadot/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ path = "bin/main.rs"
# foundry internal
codec = { version = "3.7.5", default-features = true, package = "parity-scale-codec" }
substrate-runtime = { path = "substrate-runtime" }
pallet-revive-eth-rpc = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master" }
pallet-revive-eth-rpc = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage"}
secp256k1 = { version = "0.28.0", default-features = false }
libsecp256k1 = { version = "0.7.0", default-features = false }
sp-runtime-interface = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master", default-features = false }
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master", default-features = false, features = [
sp-runtime-interface = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage", default-features = false }
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage", default-features = false, features = [
"sc-allocator",
"sc-basic-authorship",
"sc-block-builder",
Expand Down
4 changes: 2 additions & 2 deletions crates/anvil-polkadot/substrate-runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ license.workspace = true
[dependencies]
array-bytes = { version = "6.2.2", default-features = false }
codec = { version = "3.7.5", default-features = false, package = "parity-scale-codec" }
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master", default-features = false, features = [
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage", default-features = false, features = [
Copy link

Choose a reason for hiding this comment

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

Do you plan to use this ?

Copy link
Author

Choose a reason for hiding this comment

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

will use latest master as soon as the pr is merged

"pallet-aura",
"pallet-balances",
"pallet-revive",
Expand All @@ -29,7 +29,7 @@ scale-info = { version = "2.11.6", default-features = false }
serde_json = { version = "1.0", default-features = false, features = ["alloc"] }

[build-dependencies]
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "master", default-features = false, optional = true, features = ["substrate-wasm-builder"] }
polkadot-sdk = { git = "https://github.com/paritytech/polkadot-sdk.git", branch = "pkhry/external_transient_storage", default-features = false, optional = true, features = ["substrate-wasm-builder"] }

[features]
default = ["std"]
Expand Down
1 change: 1 addition & 0 deletions crates/forge/tests/it/revive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ pub mod cheat_store;
pub mod cheats_individual;
pub mod migration;
pub mod record_accesses;
pub mod transient_storage;
pub mod tx_gas_price;
16 changes: 16 additions & 0 deletions crates/forge/tests/it/revive/transient_storage.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use crate::{config::*, test_helpers::TEST_DATA_REVIVE};
use foundry_test_utils::Filter;
use revive_strategy::ReviveRuntimeMode;
use revm::primitives::hardfork::SpecId;
use rstest::rstest;

#[rstest]
#[case::pvm(ReviveRuntimeMode::Pvm)]
#[case::evm(ReviveRuntimeMode::Evm)]
#[tokio::test(flavor = "multi_thread")]
async fn test_transient_storage(#[case] runtime_mode: ReviveRuntimeMode) {
let runner: forge::MultiContractRunner = TEST_DATA_REVIVE.runner_revive(runtime_mode);
let filter = Filter::new("testTransientStoragePersistence", "TransientStorage", ".*/revive/.*");

TestConfig::with_filter(runner, filter).spec_id(SpecId::PRAGUE).run().await;
}
2 changes: 1 addition & 1 deletion crates/revive-env/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ parameter_types! {
pub const UnstableInterface: bool = true;
pub const CodeHashLockupDepositPercent: Perbill = Perbill::from_percent(0);
pub const NativeToEthRatio: u32 = 1_000_000;
pub const GasScale : u32 = 1_000_000;
pub const GasScale : u32 = 100_000_000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it fix?

Copy link
Author

Choose a reason for hiding this comment

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

OutOfGas errors with balancer v3


pub const DepositPerByte: Balance = 1;
pub const DepositPerItem: Balance = 2;
Expand Down
20 changes: 12 additions & 8 deletions crates/revive-strategy/src/cheatcodes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector
let caller_h160 = H160::from_slice(input.caller().as_slice());
let eth_deals = &state.eth_deals;

let res = ctx.externalities.execute_with(|| {
let res = ctx.externalities.execute_with_transient_storage(|transient_storage| {
tracer.watch_address(&caller_h160);

tracer.trace(|| {
Expand Down Expand Up @@ -1135,9 +1135,10 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector
effective_gas_price: Some(gas_price_pvm),
mock_handler: Some(Box::new(mock_handler.clone())),
is_dry_run: None,
transient_storage: Some(transient_storage),
};

Pallet::<Runtime>::bare_instantiate(
let result = Pallet::<Runtime>::bare_instantiate(
origin,
evm_value,
pallet_revive::TransactionLimits::WeightAndDeposit {
Expand All @@ -1147,8 +1148,9 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector
code,
data,
salt,
exec_config,
)
&exec_config,
);
(result, exec_config.transient_storage.expect("can't happen"))
})
});
let mut gas = Gas::new(input.gas_limit());
Expand Down Expand Up @@ -1292,7 +1294,7 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector

let mut tracer = Tracer::new(state.expected_calls.clone(), state.expected_creates.clone());
let eth_deals = &state.eth_deals;
let res = ctx.externalities.execute_with(|| {
let res = ctx.externalities.execute_with_transient_storage(|transient_storage| {
// Watch the caller's address so its nonce changes get tracked in prestate trace
tracer.watch_address(&caller_h160);

Expand All @@ -1310,13 +1312,14 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector
effective_gas_price: Some(gas_price_pvm),
mock_handler: Some(Box::new(mock_handler.clone())),
is_dry_run: None,
transient_storage: Some(transient_storage),
};
if should_bump_nonce {
System::inc_account_nonce(
AccountId32Mapper::<Runtime>::to_fallback_account_id(&caller_h160),
);
}
Pallet::<Runtime>::bare_call(
let result = Pallet::<Runtime>::bare_call(
origin,
target,
evm_value,
Expand All @@ -1325,8 +1328,9 @@ impl foundry_cheatcodes::CheatcodeInspectorStrategyExt for PvmCheatcodeInspector
deposit_limit: if call.is_static { 0 } else { 100_000_000_000_000 },
},
call.input.bytes(ecx).to_vec(),
exec_config,
)
&exec_config,
);
(result, exec_config.transient_storage.expect("can't happen"))
})
});
mock_handler.update_state_mocks(state);
Expand Down
2 changes: 2 additions & 0 deletions crates/revive-strategy/src/executor/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,13 @@ impl ExecutorStrategyExt for ReviveExecutorStrategyRunner {
let ctx = get_context_ref(ctx);
let mut externalities = ctx.externalties.0.lock().unwrap();
externalities.externalities.ext().storage_start_transaction();
externalities.transient_storage.start_transaction();
}

fn rollback_transaction(&self, ctx: &dyn ExecutorStrategyContext) {
let ctx = get_context_ref(ctx);
let mut state = ctx.externalties.0.lock().unwrap();
let _ = state.externalities.ext().storage_rollback_transaction();
state.transient_storage.rollback_transaction();
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we could wrap this functionality and just call rollback_transaction()
instead of calling rollback on transient storage and storage?
Similarly we could do it for start_transaction. WDYT?

}
}
35 changes: 30 additions & 5 deletions crates/revive-strategy/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use polkadot_sdk::{
},
pallet_revive::{
self, AccountId32Mapper, AccountInfo, AddressMapper, BytecodeType, ContractInfo,
ExecConfig, Executable, HoldReason, Pallet, ResourceMeter,
ExecConfig, Executable, HoldReason, Pallet, ResourceMeter, TransientStorage,
},
sp_core::{self, H160, H256},
sp_externalities::Externalities,
Expand All @@ -17,13 +17,15 @@ use polkadot_sdk::{
};
use revive_env::{Balances, BlockAuthor, ExtBuilder, NativeToEthRatio, Runtime, System, Timestamp};
use std::{
cell::RefCell,
fmt::Debug,
sync::{Arc, Mutex},
};

pub(crate) struct Inner {
pub externalities: TestExternalities,
pub depth: usize,
pub transient_storage: TransientStorage<Runtime>,
}

#[derive(Default)]
Expand All @@ -39,6 +41,7 @@ impl Default for Inner {
)])
.build(),
depth: 0,
transient_storage: TransientStorage::new(u32::MAX),
}
}
}
Expand All @@ -52,8 +55,10 @@ impl Debug for TestEnv {
impl Clone for TestEnv {
fn clone(&self) -> Self {
let mut inner: Inner = Default::default();
inner.externalities.backend = self.0.lock().unwrap().externalities.as_backend();
inner.depth = self.0.lock().unwrap().depth;
let mut data = self.0.lock().unwrap();
inner.externalities.backend = data.externalities.as_backend();
inner.transient_storage = data.transient_storage.clone();
inner.depth = data.depth;
Self(Arc::new(Mutex::new(inner)))
}
}
Expand All @@ -67,22 +72,42 @@ impl TestEnv {
let mut state = self.0.lock().unwrap();
state.depth += 1;
state.externalities.ext().storage_start_transaction();
state.transient_storage.start_transaction();
}

pub fn revert(&mut self, depth: usize) {
let mut state = self.0.lock().unwrap();
while state.depth > depth + 1 {
state.externalities.ext().storage_rollback_transaction().unwrap();
let _ = state.externalities.ext().storage_rollback_transaction();
state.transient_storage.rollback_transaction();
state.depth -= 1;
}
state.externalities.ext().storage_rollback_transaction().unwrap();
let _ = state.externalities.ext().storage_rollback_transaction();
state.transient_storage.rollback_transaction();
state.externalities.ext().storage_start_transaction();
state.transient_storage.start_transaction();
}

pub fn execute_with<R, F: FnOnce() -> R>(&mut self, f: F) -> R {
self.0.lock().unwrap().externalities.execute_with(f)
}

pub fn execute_with_transient_storage<
R,
F: FnOnce(RefCell<TransientStorage<Runtime>>) -> (R, RefCell<TransientStorage<Runtime>>),
>(
&mut self,
f: F,
) -> R {
let mut data = self.0.lock().unwrap();
let mut temp = TransientStorage::new(u32::MAX);
std::mem::swap(&mut data.transient_storage, &mut temp);
let transient_storage = RefCell::new(temp);
let (result, transient_storage) = data.externalities.execute_with(|| f(transient_storage));
data.transient_storage = transient_storage.into_inner();
result
}

pub fn get_nonce(&mut self, account: Address) -> u32 {
self.0.lock().unwrap().externalities.execute_with(|| {
System::account_nonce(AccountId32Mapper::<Runtime>::to_fallback_account_id(
Expand Down
2 changes: 1 addition & 1 deletion crates/revive-strategy/src/tracing/expect_create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl Tracing for CreateTracer {
if self.calls.is_empty() {
self.calls.push(_from);
}
self.calls.push(if _is_delegate_call.is_some() { self.current_addr() } else { to });
self.calls.push(if let Some(delegate) = _is_delegate_call { delegate } else { to });
}

fn exit_child_span(
Expand Down
2 changes: 1 addition & 1 deletion crates/revive-strategy/src/tracing/revert_tracer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl Tracing for RevertTracer {
self.calls.push(if self.call_types.last().is_some_and(|x| matches!(x, Type::Create)) {
self.current_addr()
} else {
to
if let Some(delegate) = _is_delegate_call { delegate } else { to }
});

if self.has_reverted.is_none() {
Expand Down
6 changes: 4 additions & 2 deletions crates/revive-strategy/src/tracing/storage_tracer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ impl Tracing for StorageTracer {
) {
let code = self.is_create.take();

if is_delegate_call.is_some() {
let to = if let Some(delegate) = is_delegate_call {
self.calls.push(self.current_addr());
delegate
} else {
self.calls.push(to);
}
to
};

let kind = if code.is_some() {
AccountAccessKind::Create
Expand Down
2 changes: 1 addition & 1 deletion testdata/default/cheats/RecordAccessesRevive.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ contract RecordAccountAccessesTest is DSTest {
assertEq(called[0].account, address(proxy));

assertEq(toUint(called[1].kind), toUint(Vm.AccountAccessKind.DelegateCall), "incorrect kind");
// assertEq(called[1].account, address(one), "incorrect account"); incorrect account with DelegateCall
assertEq(called[1].account, address(one), "incorrect account");
assertEq(called[1].accessor, address(this), "incorrect accessor");

assertEq(
Expand Down
52 changes: 52 additions & 0 deletions testdata/default/revive/TransientStorage.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import "ds-test/test.sol";
import "cheats/Vm.sol";
import "../../default/logs/console.sol";

/**
* @title Minimal reproducer for foundry-polkadot transient storage bug
* @dev Demonstrates that transient storage is incorrectly cleared between external calls
*/
contract Counter {
function increment() external {
assembly {
let value := tload(0)
tstore(0, add(value, 1))
}
}

function get() external view returns (uint256 value) {
assembly {
value := tload(0)
}
}
}

contract TransientStorage is DSTest {
Vm constant vm = Vm(HEVM_ADDRESS);
Counter counter;

function setUp() public {
counter = new Counter();
}

/**
* @dev This test FAILS in foundry-polkadot
* Expected: counter.get() returns 1
* Actual: counter.get() returns 0
*
* The bug: transient storage is cleared between the two external calls
* According to EIP-1153, transient storage should persist for the entire transaction
*/
function testTransientStoragePersistence() public {
// Store value 1 in transient storage
counter.increment();

// This should return 1, but returns 0 in foundry-polkadot
uint256 value = counter.get();

assertEq(value, 1, "Transient storage should persist across external calls");
}
}
Loading