-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Resolve "Reverted report validator transactions get added in block #10580" #10602
Changes from 6 commits
e1c66fe
41a39c2
57bcba8
6811a06
7843b5a
af932e6
d832f01
285ad8a
e6d9d59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,12 +18,34 @@ | |
|
||
use bytes::Bytes; | ||
use ethereum_types::Address; | ||
use ethabi::{decode, ParamType, Token}; | ||
use types::ids::BlockId; | ||
|
||
/// Provides `call_contract` method | ||
pub trait CallContract { | ||
/// Like `call`, but with various defaults. Designed to be used for calling contracts. | ||
/// Returns an error in case of vm exception. Tries to decode Solidity revert message if there is a revert exception. | ||
fn call_contract(&self, id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String>; | ||
|
||
/// Like `call`, but with various defaults. Designed to be used for calling contracts with specified sender. | ||
/// Returns an error in case of vm exception. Tries to decode Solidity revert message if there is a revert exception. | ||
fn call_contract_from_sender(&self, block_id: BlockId, address: Address, sender: Address, data: Bytes) -> Result<Bytes, String>; | ||
|
||
/// Try to decode Solidity revert string | ||
fn try_decode_solidity_revert_msg(&self, data: &Bytes) -> Option<String> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't suit the trait at all:
This should be moved to a separate function that is use in the implementation. |
||
let mut result = None; | ||
if data.len() > 4 { | ||
let (error_selector, enc_string) = data.split_at(4); | ||
// Error(string) selector. Details: https://solidity.readthedocs.io/en/v0.5.8/control-structures.html#error-handling-assert-require-revert-and-exceptions | ||
if error_selector == [0x08, 0xc3, 0x79, 0xa0] { | ||
VladLupashevskyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result = decode(&[ParamType::String], enc_string) | ||
.as_ref() | ||
.map(|d| if let Token::String(str) = &d[0] { Some(str.as_str().to_string()) } else { None }) | ||
.unwrap_or_else(|_| None); | ||
} | ||
} | ||
result | ||
} | ||
} | ||
|
||
/// Provides information on a blockchain service and it's registry | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ use std::time::{Instant, Duration}; | |
use blockchain::{BlockReceipts, BlockChain, BlockChainDB, BlockProvider, TreeRoute, ImportRoute, TransactionAddress, ExtrasInsert, BlockNumberKey}; | ||
use bytes::Bytes; | ||
use call_contract::{CallContract, RegistryInfo}; | ||
use ethabi::{Token}; | ||
use ethcore_miner::pool::VerifiedTransaction; | ||
use ethereum_types::{H256, Address, U256}; | ||
use evm::Schedule; | ||
|
@@ -42,7 +43,7 @@ use types::filter::Filter; | |
use types::log_entry::LocalizedLogEntry; | ||
use types::receipt::{Receipt, LocalizedReceipt}; | ||
use types::{BlockNumber, header::{Header, ExtendedHeader}}; | ||
use vm::{EnvInfo, LastHashes}; | ||
use vm::{EnvInfo, LastHashes, Error as VmError}; | ||
|
||
use block::{LockedBlock, Drain, ClosedBlock, OpenBlock, enact_verified, SealedBlock}; | ||
use client::ancient_import::AncientVerifier; | ||
|
@@ -1223,15 +1224,19 @@ impl Client { | |
// transaction for calling contracts from services like engine. | ||
// from the null sender, with 50M gas. | ||
fn contract_call_tx(&self, block_id: BlockId, address: Address, data: Bytes) -> SignedTransaction { | ||
let from = Address::default(); | ||
self.contract_call_tx_from_sender(block_id, address, Address::default(), data) | ||
} | ||
|
||
// transaction for calling contracts with specified sender | ||
fn contract_call_tx_from_sender(&self, block_id: BlockId, address: Address, sender: Address, data: Bytes) -> SignedTransaction { | ||
transaction::Transaction { | ||
nonce: self.nonce(&from, block_id).unwrap_or_else(|| self.engine.account_start_nonce(0)), | ||
nonce: self.nonce(&sender, block_id).unwrap_or_else(|| self.engine.account_start_nonce(0)), | ||
action: Action::Call(address), | ||
gas: U256::from(50_000_000), | ||
gas_price: U256::default(), | ||
value: U256::default(), | ||
data: data, | ||
}.fake_sign(from) | ||
data, | ||
}.fake_sign(sender) | ||
} | ||
|
||
fn do_virtual_call( | ||
|
@@ -1438,15 +1443,28 @@ impl RegistryInfo for Client { | |
|
||
impl CallContract for Client { | ||
fn call_contract(&self, block_id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String> { | ||
self.call_contract_from_sender(block_id, address, Address::default(), data) | ||
} | ||
|
||
fn call_contract_from_sender(&self, block_id: BlockId, address: Address, sender: Address, data: Bytes) -> Result<Bytes, String> { | ||
VladLupashevskyi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let state_pruned = || CallError::StatePruned.to_string(); | ||
let state = &mut self.state_at(block_id).ok_or_else(&state_pruned)?; | ||
let header = self.block_header_decoded(block_id).ok_or_else(&state_pruned)?; | ||
|
||
let transaction = self.contract_call_tx(block_id, address, data); | ||
let transaction = self.contract_call_tx_from_sender(block_id, address, sender, data); | ||
|
||
self.call(&transaction, Default::default(), state, &header) | ||
.map_err(|e| format!("{:?}", e)) | ||
.map(|executed| executed.output) | ||
let executed = self.call(&transaction, Default::default(), state, &header).map_err(|e| format!("{:?}", e))?; | ||
if let Some(exception) = executed.exception { | ||
let mut error = format!("{:?}", exception); | ||
if exception == VmError::Reverted { | ||
let output = executed.output.clone(); | ||
let msg = self.try_decode_solidity_revert_msg(&output).unwrap_or_else(|| format!("{}", Token::Bytes(output))); | ||
error = format!("{} {}", error, msg); | ||
} | ||
Err(error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make this a little easier to follow I suggest:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A more general question I have is: why do this error munging here rather than in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately there is no access from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we change that maybe? Like, |
||
} else { | ||
Ok(executed.output) | ||
} | ||
} | ||
} | ||
|
||
|
@@ -2180,6 +2198,10 @@ impl BlockChainClient for Client { | |
self.importer.miner.import_own_transaction(self, signed.into()) | ||
} | ||
|
||
fn call_contract_as_author(&self, id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String> { | ||
self.call_contract_from_sender(id, address, self.importer.miner.authoring_params().author, data) | ||
} | ||
|
||
fn registrar_address(&self) -> Option<Address> { | ||
self.registrar_address.clone() | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,9 @@ pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContra | |
/// Schedule state-altering transaction to be executed on the next pending block. | ||
fn transact_contract(&self, address: Address, data: Bytes) -> Result<(), transaction::Error>; | ||
|
||
/// Call contract as block author | ||
fn call_contract_as_author(&self, id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I like this addition. Seems like a convenience method, but maybe I'm missing something. I'm hesitant to add new trait methods if we can avoid it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue of not using Honestly I'm not sure what would be the best solution here. @dvdplm any thoughts about that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure either tbh. @tomusdrw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have If |
||
|
||
/// Get the address of the registry itself. | ||
fn registrar_address(&self) -> Option<Address>; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ use machine::{AuxiliaryData, Call, EthereumMachine}; | |
use parking_lot::RwLock; | ||
use types::BlockNumber; | ||
use types::header::Header; | ||
use types::ids::BlockId; | ||
|
||
use client::EngineClient; | ||
|
||
|
@@ -58,6 +59,7 @@ impl ValidatorContract { | |
|
||
match client.as_full_client() { | ||
Some(c) => { | ||
c.call_contract_as_author(BlockId::Latest, self.contract_address, data.clone())?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it seems that now we call the contract twice just to improve the logs? |
||
c.transact_contract(self.contract_address, data) | ||
.map_err(|e| format!("Transaction import error: {}", e))?; | ||
Ok(()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considerations regarding the entire trait, extracting revert messages really calls for introducing a proper error type here (at least a two-value enum).
I think it would also be way better to introduce an
Options
parameter (with a builder) instead of introducing another method: