Skip to content

Commit

Permalink
Fix reentrant hang issue (#707)
Browse files Browse the repository at this point in the history 
* fix unhandled transaction

* fix evm deliver_tx nonce

* update startnodes.sh and checkpoint file

---------

Co-authored-by: simonjiao <[email protected]>
  • Loading branch information
@simonjiao @harryliisme3
2 people authored and harryliisme3 committed Feb 7, 2023
1 parent 88fa8e7 commit c2b27c5
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 82 deletions.
6 changes: 6 additions & 0 deletions src/components/config/src/abci/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ pub struct CheckPointConfig {
pub fix_delegators_am_height: u64,
pub validators_limit_v2_height: u64,
pub enable_eip1559_height: u64,

// https://github.com/FindoraNetwork/platform/pull/707
// FO-1370: V0.3.30 EVM bug: receipt missing when error code === 1
pub fix_deliver_tx_revert_nonce_height: i64,
}

impl CheckPointConfig {
Expand Down Expand Up @@ -129,6 +133,7 @@ impl CheckPointConfig {
fix_delegators_am_height: 0,
validators_limit_v2_height: 0,
enable_eip1559_height: 0,
fix_deliver_tx_revert_nonce_height: 0,
};
#[cfg(not(feature = "debug_env"))]
let config = CheckPointConfig {
Expand Down Expand Up @@ -158,6 +163,7 @@ impl CheckPointConfig {
fix_delegators_am_height: 30000000,
validators_limit_v2_height: 30000000,
enable_eip1559_height: 40000000,
fix_deliver_tx_revert_nonce_height: 40000000,
};
let content = toml::to_string(&config).unwrap();
file.write_all(content.as_bytes()).unwrap();
Expand Down
165 changes: 97 additions & 68 deletions src/components/contracts/modules/ethereum/src/impls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use fp_core::{
use fp_events::Event;
use fp_evm::{BlockId, CallOrCreateInfo, Runner, TransactionStatus};
use fp_storage::{Borrow, BorrowMut};
use fp_types::crypto::Address;
use fp_types::{
actions::evm as EvmAction,
crypto::{secp256k1_ecdsa_recover, HA256},
Expand Down Expand Up @@ -249,72 +250,95 @@ impl<C: Config> App<C> {
transaction.action,
);

if let Err(e) = execute_ret {
let mut to = Default::default();
if let ethereum::TransactionAction::Call(target) = transaction.action {
to = target;
}
events.push(Event::emit_event(
Self::name(),
TransactionExecuted {
sender: source,
to,
contract_address: Default::default(),
transaction_hash,
reason: ExitReason::Fatal(ExitFatal::UnhandledInterrupt),
},
));

return Ok(ActionResult {
code: 1,
data: vec![],
log: format!("{e}",),
gas_wanted: gas_limit.low_u64(),
gas_used: 0,
events,
});
}
let (ar_code, info, to, contract_address, reason, data, status, used_gas) =
match execute_ret {
Err(e) => {
let to = if let ethereum::TransactionAction::Call(target) =
transaction.action
{
Some(target)
} else {
None
};

let logs = vec![ethereum::Log {
address: source,
topics: vec![],
data: format!("{e}").as_str().into(),
}];

let reason = ExitReason::Fatal(ExitFatal::UnhandledInterrupt);
let data = vec![];
let status = TransactionStatus {
transaction_hash,
transaction_index,
from: source,
to,
contract_address: None,
logs: logs.clone(),
logs_bloom: {
let mut bloom: Bloom = Bloom::default();
Self::logs_bloom(logs, &mut bloom);
bloom
},
};
let used_gas = U256::zero();

(Some(1), None, to, None, reason, data, status, used_gas)
}

let (to, contract_address, info) = execute_ret.unwrap();

let (reason, data, status, used_gas) = match info.clone() {
CallOrCreateInfo::Call(info) => (
info.exit_reason,
info.value.clone(),
TransactionStatus {
transaction_hash,
transaction_index,
from: source,
to,
contract_address: None,
logs: info.logs.clone(),
logs_bloom: {
let mut bloom: Bloom = Bloom::default();
Self::logs_bloom(info.logs, &mut bloom);
bloom
},
},
info.used_gas,
),
CallOrCreateInfo::Create(info) => (
info.exit_reason,
vec![],
TransactionStatus {
transaction_hash,
transaction_index,
from: source,
to,
contract_address: Some(info.value),
logs: info.logs.clone(),
logs_bloom: {
let mut bloom: Bloom = Bloom::default();
Self::logs_bloom(info.logs, &mut bloom);
bloom
},
},
info.used_gas,
),
};
Ok((to, contract_address, info)) => {
let (reason, data, status, used_gas) = match info.clone() {
CallOrCreateInfo::Call(info) => (
info.exit_reason,
info.value.clone(),
TransactionStatus {
transaction_hash,
transaction_index,
from: source,
to,
contract_address: None,
logs: info.logs.clone(),
logs_bloom: {
let mut bloom: Bloom = Bloom::default();
Self::logs_bloom(info.logs, &mut bloom);
bloom
},
},
info.used_gas,
),
CallOrCreateInfo::Create(info) => (
info.exit_reason,
vec![],
TransactionStatus {
transaction_hash,
transaction_index,
from: source,
to,
contract_address: Some(info.value),
logs: info.logs.clone(),
logs_bloom: {
let mut bloom: Bloom = Bloom::default();
Self::logs_bloom(info.logs, &mut bloom);
bloom
},
},
info.used_gas,
),
};

(
None,
Some(info),
to,
contract_address,
reason,
data,
status,
used_gas,
)
}
};

for log in &status.logs {
debug!(target: "ethereum", "transaction status log: block: {:?}, address: {:?}, topics: {:?}, data: {:?}",
Expand All @@ -332,7 +356,7 @@ impl<C: Config> App<C> {

let (code, message) = match reason {
ExitReason::Succeed(_) => (0, String::new()),
// code 1 indicates `Failed to execute evm function`, see above
// code 1 indicates `Failed to execute evm function`, see the `ar_code`
ExitReason::Error(_) => (2, "EVM error".to_string()),
ExitReason::Revert(_) => {
let mut message =
Expand All @@ -348,7 +372,9 @@ impl<C: Config> App<C> {
}
(3, message)
}
ExitReason::Fatal(_) => (4, "EVM Fatal error".to_string()),
ExitReason::Fatal(_) => {
(ar_code.unwrap_or(4), "EVM Fatal error".to_string())
}
};

if code != 0 {
Expand Down Expand Up @@ -393,7 +419,10 @@ impl<C: Config> App<C> {

Ok(ActionResult {
code,
data: serde_json::to_vec(&info).unwrap_or_default(),
source: Some(Address::from(source)),
data: info
.and_then(|i| serde_json::to_vec(&i).ok())
.unwrap_or_default(),
log: message,
gas_wanted: gas_limit.low_u64(),
gas_used: used_gas.low_u64(),
Expand Down
40 changes: 40 additions & 0 deletions src/components/contracts/modules/ethereum/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use fp_traits::{
use fp_types::{actions::ethereum::Action, crypto::Address};
use ruc::*;
use std::marker::PhantomData;
use tracing::debug;

pub const MODULE_NAME: &str = "ethereum";

Expand Down Expand Up @@ -234,6 +235,45 @@ impl<C: Config> ValidateUnsigned for App<C> {

Ok(())
}

fn post_execute(ctx: &Context, result: &ActionResult) -> Result<()> {
if result.code != 0
&& ctx.header.height >= CFG.checkpoint.fix_deliver_tx_revert_nonce_height
{
let prev = result
.source
.as_ref()
.map(|who| C::AccountAsset::nonce(ctx, who))
.unwrap_or_default();

ctx.state.write().discard_session();

// If EVM transaction fails, state change gets reverted/discarded except nonce.
let current = result
.source
.as_ref()
.map(|who| C::AccountAsset::nonce(ctx, who))
.unwrap_or_default();

if prev.gt(&current) {
if let Some(who) = result.source.as_ref() {
debug!(
target:
"baseapp",
"In post_execute: source {} {} {}",
who,
prev,
current
);
// Keep it same with `pre_execute`
let _ = C::AccountAsset::inc_nonce(ctx, who);
} else {
unreachable!();
}
}
}
Ok(())
}
}

impl<C: Config> BlockHashMapping for App<C> {
Expand Down
30 changes: 21 additions & 9 deletions src/components/contracts/primitives/core/src/transaction.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::context::{Context, RunTxMode};
use abci::Event;
use config::abci::global_cfg::CFG;
use fp_types::transaction::CheckedTransaction;
use fp_types::{crypto::Address, transaction::CheckedTransaction};
use impl_trait_for_tuples::impl_for_tuples;
use ruc::*;
use std::fmt::Debug;
Expand Down Expand Up @@ -117,6 +117,12 @@ pub trait ValidateUnsigned {
///
/// Changes made to storage should be discarded by caller.
fn validate_unsigned(ctx: &Context, call: &Self::Call) -> Result<()>;

/// Do any post-flight stuff for an transaction.
///
fn post_execute(_ctx: &Context, _result: &ActionResult) -> Result<()> {
Ok(())
}
}

impl<Address, Call, Extra> Applyable for CheckedTransaction<Address, Call, Extra>
Expand All @@ -143,12 +149,12 @@ where
U: ValidateUnsigned<Call = Self::Call>,
U: Executable<Origin = Self::Origin, Call = Self::Call>,
{
let (maybe_who, pre) = if let Some((sender, extra)) = self.signed {
let (signed_tx, maybe_who, pre) = if let Some((sender, extra)) = self.signed {
let pre = Extra::pre_execute(extra, ctx, &sender)?;
(Some(sender), pre)
(true, Some(sender), pre)
} else {
U::pre_execute(ctx, &self.function)?;
(None, Default::default())
(false, None, Default::default())
};

ctx.state.write().commit_session();
Expand All @@ -167,12 +173,16 @@ where
res.log = String::from("ctx state is not good to commit");

ctx.state.write().discard_session();
} else if res.code == 0 {
Extra::post_execute(ctx, pre, &res)?;

ctx.state.write().commit_session();
} else if signed_tx {
if res.code == 0 {
Extra::post_execute(ctx, pre, &res)?;
ctx.state.write().commit_session();
} else {
ctx.state.write().discard_session();
}
} else {
ctx.state.write().discard_session();
U::post_execute(ctx, &res)?;
ctx.state.write().commit_session();
}

ctx.db.write().commit_session();
Expand All @@ -198,6 +208,8 @@ pub struct ActionResult {
/// 4 - EVM ExitReason::Fatal
/// 0xff - context state maybe messed up
pub code: u32,
/// Record the source address
pub source: Option<Address>,
/// Data is any data returned from message or handler execution.
pub data: Vec<u8>,
/// Log contains the log information from message or handler execution.
Expand Down
5 changes: 0 additions & 5 deletions tools/devnet/startnodes.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,6 @@

# env
source ./tools/devnet/env.sh || exit 1
DEFAULT_BIN_CFG="release"
export BIN="target/$DEFAULT_BIN_CFG"

$BIN/fn setup -S $ENDPOINT > /dev/null
$BIN/fn setup -O $WALLET/mnenomic.key > /dev/null

# start one single node if specified
Node=""
Expand Down

0 comments on commit c2b27c5

Please sign in to comment.