Resolve "Reverted report validator transactions get added in block #10580" #10602

VladLupashevskyi · 2019-04-19T22:40:12Z

Here I added the call check before transact_contract for Validators contract in order to prevent adding reverted report transactions to the block.

For this I had to introduce call_contract_from_sender for CallContract trait where I added processing of VM exception as well as decoding of revert message according to Solidity specs (Error(string) function). I'm not sure whether the decoding logic should be moved into separate function, so I implemented everything in call_contract_from_sender for now.

So now having revert("Benign reporting is disabled") in reportBenign function in ValidatorSet smart contract would lead to such messages in the console:

Validator 0x7721…aafc could not be reported Reverted Benign reporting is disabled

In case if the revert result is not decodable pure bytes will be shown.

Closes #10580

…tion checking + add call before transact to Validator contract

parity-cla-bot · 2019-04-19T22:40:15Z

It looks like @VladLupashevskyi signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

ethcore/src/client/client.rs

HCastano · 2019-04-26T16:26:39Z

Like you said, I also think that the decoding of the revert message should be moved into it's own function. I'm not entirely sure what the best place for it is in the codebase, so maybe just make it a method of the CallContract trait.

ethcore/src/client/client.rs

HCastano · 2019-04-26T16:42:58Z

ethcore/src/engines/validator_set/contract.rs

@@ -58,6 +59,7 @@ impl ValidatorContract {

 		match client.as_full_client() {
 			Some(c) => {
+				c.call_contract_as_author(BlockId::Latest, self.contract_address.clone(), data.clone())?;


Two things here:

Maybe I'm missing something, but why do we need to add this?

I don't think the .clone() on the contract address is necessary

It's used exactly for checking whether the tx will be reverted or not and if yes it will fail with an error.

Done :)

…nts + simplify code according to code review

…into 10580-estimate-gas-for-validator-contract-calls

ethcore/call-contract/src/call_contract.rs

ethcore/src/client/client.rs

dvdplm · 2019-05-24T08:02:56Z

ethcore/src/client/client.rs

+				let msg = self.try_decode_solidity_revert_msg(&output).unwrap_or_else(|| format!("{}", Token::Bytes(output)));
+				error = format!("{} {}", error, msg);
+			}
+			Err(error)


To make this a little easier to follow I suggest:

if let Some(exception) = executed.exception { match exception { VmError::Reverted => { … Err(format!("{:?} {}", exception, msg)) }, _ => Err(exception.to_string()) // not sure if `From<String>` is impl'd for `VmError` }

A more general question I have is: why do this error munging here rather than in the Debug impl for VmError? Couldn't we add the decoding attempt there and essentially keep this code to a simple map_err(|e| format!("{:?}", e)) like it was before? Seems like this is spreading out the logic into several places and I'm not sure I understand why it's worth it.

Unfortunately there is no access from VmError to tx output which we need for decoding error. Alternatively we could put all this error munging under Executed in some function like format_error_and_decode_ouput. What do you think about it @dvdplm ?

Unfortunately there is no access from VmError to tx output

Can we change that maybe? Like, VmError::Reverted(tx_output). If not, then I'm not opposed to having a format_error_and_decode_ouput helper like you suggest.

dvdplm · 2019-05-24T08:12:40Z

ethcore/src/client/traits.rs

@@ -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>;


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.

The issue of not using call_contract_from_sender is because we can't get blocks author outside client in transact method of ValidatorContract. By avoiding usage of this method we would need to introduce getter for author or the whole miner instance (which is available but only for tests):

https://github.com/paritytech/parity-ethereum/blob/af932e6806b080f50d1f3d83c0e96348a6e91e84/ethcore/src/client/client.rs#L1001

Honestly I'm not sure what would be the best solution here. @dvdplm any thoughts about that?

Not sure either tbh. @tomusdrw?

We already have CallContract trait for calling contracts, we have put a lot of effort trying to make BlockChainClient smaller, so adding any new methods to this trait is a no go.

If validator_set/contract.rs needs to call a contract as author (what's questionable as well), it should get the author address from engine (where it is created or called)

tomusdrw

I'm very unsure if that's he right approach. I see two other options (or combination of them) to what I believe would achieve the same goal as this PR:

CLI to disable reports (then transactions are never produced)
Parsing Revert reason when printing "Reverted ..." to display more friendly message about the call being reverted due to being not supported by the contract.

My reasoning behind this:

We introduce additional call during the process (and calls are heavy!)
The fact that we succesfuly call the contract doesn't guarantee that the transaction will pass (nor vice versa)

tomusdrw · 2019-06-17T07:35:31Z

ethcore/src/engines/validator_set/contract.rs

@@ -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())?;


So it seems that now we call the contract twice just to improve the logs?
I'm really hesitant about this change - why can't we decode the error code from the receipt of actual transaction? Now you have no guarantees that if the first call passes the transaction will actually succeed.

tomusdrw · 2019-06-17T07:38:12Z

ethcore/src/client/traits.rs

@@ -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>;


We already have CallContract trait for calling contracts, we have put a lot of effort trying to make BlockChainClient smaller, so adding any new methods to this trait is a no go.

If validator_set/contract.rs needs to call a contract as author (what's questionable as well), it should get the author address from engine (where it is created or called)

tomusdrw · 2019-06-17T07:53:24Z

ethcore/call-contract/src/call_contract.rs

+	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> {


This doesn't suit the trait at all:

You don't use self

It's should never be overwritten by the implementers.

This should be moved to a separate function that is use in the implementation.

tomusdrw · 2019-06-17T08:04:10Z

ethcore/call-contract/src/call_contract.rs

 use types::ids::BlockId;

+const ERROR_SELECTOR: [u8; 4] = [0x08, 0xc3, 0x79, 0xa0];
+
 /// Provides `call_contract` method
 pub trait CallContract {


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:

pub enum CallError { Reverted(String), Other(String), } #[derive(derive_builder::Builder, Default, Clone)] pub CallOptions { contract_address: Address, sender: Address, data: Bytes, value: U256, gas: U256, gas_price: U256, } impl CallOptions { /// Convenience method for creating the most common use case. pub fn new(contract: Address, data: Bytes) -> Self { Self::default().contract_address(contract).data(data) } } pub trait CallContract { /// Executes a transient call to a contract at given `BlockId`. /// /// The constructed transaction must be executed on top of state at block `id`, /// any changes introduce by the call must be discarded. /// Returns: /// - A return data from the contract if the call was successful, /// - A `CallError::Reverted(msg)` error in case the call was reverted with an exception and message. /// - A `CallError::Other(msg)` in case the call did not succeed for other reasons. fn call_contract(&self, id: BlockId, call_options: CallOptions) -> Result<Bytes, CallError>; }

tomusdrw · 2019-06-17T08:05:05Z

ethcore/src/client/client.rs

@@ -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::zero();
+		self.contract_call_tx_from_sender(block_id, address, Address::zero(), data)


Since it's repeated so many times it should be just default implementation, but instead best to have a look at the CallOptions pattern above.

VladLupashevskyi · 2019-06-17T14:50:12Z

@tomusdrw Thank you very much for very informative comments! At the very beginning I was considering adding just CLI flags to disable report benign/malicious, however I thought it won't be that robust in cases when the conditions that report will be reverted might change. For example if we have the following pattern in the contract:

function reportBenign(address sealingAddress, uint256 blockNumber) external {
        require(reportBenignEnabled, "Report benign is disabled");
        ...
    }

the sealers configuration file would need to be updated every time reportBenignEnabled is changed, therefore I have decided to implement that check via call. If I'm not mistaken the call will only happen once per validator on each new block, so I'm not sure that will be a bottleneck here given that validator node has higher system requirements than non-sealing node. The only thing I can think about when call might slow down the process of syncing when the sealing is enabled (btw I noticed that it's already slowed down during syncing with enabled sealing on blocks when some validator was missing blocks, because the syncing node tries to report it - I have to disable sealing to increase the speed, not sure but it might worth a separate issue).

However I have to admit that I haven't payed enough attention to the architectures where you cannot afford call checks before tx is mined due to constantly changing contract storage.

Considering everything above I would propose to have the CLI flags (benign_reporting, malicious_reporting) which would have the following values:

force - the call check does not happen (basically the same what happens now). Reporting tx will be always submitted (reverted txes will be always put in blocks).
call - the reporting is done only if call check passes. Can be chosen when the design can afford some missing reports (reverted txes can be put in block in some cases).
disable - the reporting is disabled. Can be chosen when the reporting is always disabled.

I believe having such a choice everybody could choose the compromise between robustness and guarantees that every report is added to the block.

As for the logging, I like the idea of adding logs when tx is mined, however it's not possible to get output of tx neither from tx nor from tx receipt, the only thing comes to my mind is trace module. Or did you mean just logging tx failure based on status_code of tx receipt?

tomusdrw · 2019-06-17T15:31:38Z

@VladLupashevskyi I like the idea of having it fully configurable, that would be quite amazing, but will probably require a bit more effort.
I'd modify the CLI flag a bit and introduce a cache (in block numbers), so that we can call say every 1k blocks instead of calling on every block.

For example if we have the following pattern in the contract, the sealers configuration file would need to be updated every time reportBenignEnabled is changed, therefore I have decided to implement that check via call. If I'm not mistaken the call will only happen once per validator on each new block, so I'm not sure that will be a bottleneck here given that validator node has higher system requirements than non-sealing node.

I see, although the question is which use case is dominant and we should definitely set the defaults according to this. I'd argue that calling the contract even though we know the reporting is always enabled is extremely wasteful and will negatively affect performance, even if it's just a tiny bit, the performance was always our focus and any slowness can also affect network stability (validator can miss it's slot).

not sure but it might worth a separate issue
I think so, yeah. I believe we might rethink and unify how sealing is driven for Authority chains (for AuRa it's miner, but I believe Clique has it's own StepService). --force-sealing is also pretty wasteful, cause authorities need to create the pending block even if it's not their turn to do so.

As for the logging, I like the idea of adding logs when tx is mined, however it's not possible to get output of tx neither from tx nor from tx receipt, the only thing comes to my mind is trace module. Or did you mean just logging tx failure based on status_code of tx receipt?

Oh, I actually thought that revert reason is stored in the receipt, if that's not the case we could actually call into the contract right after we see the transaction fail.

I'm still not a huge fan of call, cause the point is that even in your example the flag can be flipped between call and the actual transaction, so it's not super reliable, just a convenience that most of reverted transactions won't be included.
Also if that's the main reason to have that issue, maybe rather we could detect such reverted transactions during block import (we can probably easily detect that those are ours transactions) and just print a warning and ignore such transaction (i.e. never actually include it in a block, cause we would revert and just push next transaction on the list).

BTW Thank you for the PR anyway!

VladLupashevskyi · 2019-06-22T13:58:59Z

@tomusdrw Thank you for your arguments :) I have tried the idea of not including the reverted transaction as I think i would be 100% guaranteed that the tx is reverted at that point.

So I did a little research in the code base and found this place where we get a transaction receipt, but the tx is not yet added to the block (push_transaction method):

https://github.com/paritytech/parity-ethereum/blob/e6d9d59239a6fc87a211fa88f81a8b6d0bc2720e/ethcore/src/block.rs#L241

I added here these lines for checking (deny_reverted can be true only when called from prepare_block, we don't want to check that when we enact the block):

if deny_reverted {
	if let types::receipt::TransactionOutcome::StatusCode(status_code) = outcome.receipt.outcome {
		if status_code == 0 {
			self.block.state.dec_nonce(&t.sender());
			return Err(TransactionError::Reverted.into());
		}
	}
};

This seems to work (with removing tx from the queue of course), however I realized then that there is actually no connection between prepare_block and import_own_transaction (which makes sense of course), but this means that the tx can be fetched by another node before it gets removed from the queue.

Just to prove that I checked how many reports do I have on the chain with 25k reports where the reporter and the block author addresses differ. The result is 2k reports (10%).

Therefore I came up with an idea to handle these cases more deterministically. In a nutshell I would add the function to the engine which would check whether applied tx should be removed from the block (so basically it's the check that happens after verify_signed). And the rule base for this case will be that validator has reported, not that it's just own transaction.

To implement this I would refactor the push_transaction into 2 methods apply_tx_to_state and push_applied_transaction, so it can be used for enact block purpose, etc:

fn push_transaction (...) {
  apply_tx_to_state(...);
  push_applied_transaction(...);
}

In prepare_block instead of using push_transaction, I would do the following sequence:

make state checkpoint
apply_tx_to_state
engine.verify_signed_and_applied(tx, receipt, tx_output)
- if failed - revert back to the checkpoint and remove from tx queue
- if succeeded - push_applied_transaction and continue

The method verify_signed_and_applied is engine specific and for AuRa it would check let's say the chain spec for the flag deny_reverted_report_transactions and if it's true the method would indicate that the tx should be removed.

So using this logic we could guarantee that no reverted tx will be placed in the block by any validator (given they have the same chain spec), and the performance won't be affected because of calls.

What do you think is it worth to try that or should I better implement configurable check using calls with caching?

tomusdrw · 2019-06-24T12:38:01Z

Thanks for the write-up @VladLupashevskyi. Having that fully fleshed-out as you did, I don't think we are going into the right direction. Altering the low-level block.rs and applying to state seems a bit too much hassle for this feature, sorry I suggested that and then retracted, I thought we could get away with something less involved and mostly isolated to miner.rs code.

Currently I'm more inclined towards implementing call + cache and having it configurable via CLI. @dvdplm what's your thoughts on this?

dvdplm · 2019-07-08T17:47:27Z

Hmm. It's a tricky one. The complexity introduced to the code would be quite significant on the one hand; on the other now that I understand the use-case better I see @VladLupashevskyi side of things as well. (Excellent write up btw, thank you for that.)
I think that if the CLI option can work (with a cache to reduce the overhead to once-per-1k-blocks) then that seems to be the option with the least friction. It seems like the safer way to go to avoid getting bogged down in delicate low-level code.

debris · 2019-07-29T14:01:53Z

I'm closing this pr for now. Please reopen if the issues are resolved or the new approach is implemented

Introduce call_contract_from_sender for CallContract + add call excep…

e1c66fe

…tion checking + add call before transact to Validator contract

HCastano added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 23, 2019

HCastano reviewed Apr 26, 2019

View reviewed changes

ethcore/src/client/client.rs Outdated Show resolved Hide resolved

HCastano reviewed Apr 26, 2019

View reviewed changes

ethcore/src/client/client.rs Outdated Show resolved Hide resolved

HCastano reviewed Apr 26, 2019

View reviewed changes

VladLupashevskyi added 5 commits May 9, 2019 21:49

Introduce try_decode_solidity_revert_msg for CallContract + add comme…

41a39c2

…nts + simplify code according to code review

Update cargo lock

57bcba8

Merge branch 'master' of https://github.com/paritytech/parity-ethereum …

6811a06

…into 10580-estimate-gas-for-validator-contract-calls

Merge branch 'master' of https://github.com/paritytech/parity-ethereum …

7843b5a

…into 10580-estimate-gas-for-validator-contract-calls

Merge branch 'master' of https://github.com/paritytech/parity-ethereum …

af932e6

…into 10580-estimate-gas-for-validator-contract-calls

dvdplm suggested changes May 24, 2019

View reviewed changes

VladLupashevskyi added 3 commits May 29, 2019 20:09

Improve code style

d832f01

Merge master into 10580-estimate-gas-for-validator-contract-calls

285ad8a

Fix cargo lock

e6d9d59

dvdplm requested a review from tomusdrw June 17, 2019 07:04

tomusdrw suggested changes Jun 17, 2019

View reviewed changes

debris closed this Jul 29, 2019

VladLupashevskyi mentioned this pull request Sep 8, 2019

Resolve "Reverted report validator transactions get added in block #10580" #11030

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve "Reverted report validator transactions get added in block #10580" #10602

Resolve "Reverted report validator transactions get added in block #10580" #10602

VladLupashevskyi commented Apr 19, 2019 •

edited

Loading

parity-cla-bot commented Apr 19, 2019

HCastano commented Apr 26, 2019

HCastano Apr 26, 2019

VladLupashevskyi May 9, 2019

dvdplm May 24, 2019

dvdplm May 24, 2019

VladLupashevskyi May 29, 2019

dvdplm Jun 17, 2019

dvdplm May 24, 2019

VladLupashevskyi May 29, 2019

dvdplm Jun 17, 2019

tomusdrw Jun 17, 2019

tomusdrw left a comment

tomusdrw Jun 17, 2019

tomusdrw Jun 17, 2019

tomusdrw Jun 17, 2019

tomusdrw Jun 17, 2019

tomusdrw Jun 17, 2019

VladLupashevskyi commented Jun 17, 2019

tomusdrw commented Jun 17, 2019

VladLupashevskyi commented Jun 22, 2019

tomusdrw commented Jun 24, 2019

dvdplm commented Jul 8, 2019

debris commented Jul 29, 2019

Resolve "Reverted report validator transactions get added in block #10580" #10602

Resolve "Reverted report validator transactions get added in block #10580" #10602

Conversation

VladLupashevskyi commented Apr 19, 2019 • edited Loading

parity-cla-bot commented Apr 19, 2019

HCastano commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladLupashevskyi commented Jun 17, 2019

tomusdrw commented Jun 17, 2019

VladLupashevskyi commented Jun 22, 2019

tomusdrw commented Jun 24, 2019

dvdplm commented Jul 8, 2019

debris commented Jul 29, 2019

VladLupashevskyi commented Apr 19, 2019 •

edited

Loading