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

VladLupashevskyi · 2019-09-08T21:53:10Z

Continuation of #10602

I opened the new MR since I couldnt reopen the previous MR (probably because I force pushed).

Basically here I implemented CLI flags for selecting a preferred strategy for reports as mentioned here with caching.

It's possible to use the following CLI flags:

benign_reporting=[force|call|disable]
malicious_reporting=[force|call|disable]
report_cache_call_every=[blk_num]

The frequency in blocks for caching is set for both reporting types. I wasn't sure whether it makes sense to make it also separated, but it will be easy to add if needed.

Please take a look and let me know if I need to improve or update something :)

Closes #10580

parity-cla-bot · 2019-09-08T21:53:13Z

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

Many thanks,

Parity Technologies CLA Bot

…into val-call

VladLupashevskyi · 2019-09-08T23:40:16Z

Hmm. I cant figure out why tests are failing. It seems that error is Bad proof which is caused because of ExecutionError::Internal and this one is caused because of IncompleteDatabase. This is so strange since I haven't touched that pieces of the code.

Could somebody please throw a quick look at it, maybe there is something really simple that I've missed?

dvdplm · 2019-09-11T06:08:27Z

Could somebody please throw a quick look at it, maybe there is something really simple that I've missed?

Nothing obvious stands out to me. I can repeat the failures locally and see Insufficient validation proof: Bad proof from zoom_to_after().

VladLupashevskyi · 2019-09-13T15:21:29Z

@dvdplm Thank you for checking this out.

I've tried running test on different commits and it starts to fail since this one a9079db. Will dig into finding what's causing the issue.

…into val-call � Conflicts: � ethcore/src/client/client.rs

VladLupashevskyi · 2019-09-13T16:59:32Z

I think it's ready for review :)

debris

All the libraries touching and utilising Engine are quite complex and we are trying to simplify them and make them easier to edit/maintain. I believe that this pull request, as is, cannot be accepted cause it is a step back in terms of complexity and simplicity of that code.

debris · 2019-10-18T14:28:02Z

ethcore/call-contract/src/call_contract.rs

+}
+
+#[derive(Builder, Default, Clone)]
+#[builder(default)]


I believe this using this macro once is not worth bringing 5 new external dependencies to the project

I agree, that using this one once does not make really sense! That and CallError have been introduced by @tomusdrw suggestion in this comment. I understood that intention to use builder was because it was planned to be used in other places. And according to CallError usage I assumed this one was suggested as the base boilerplate and is gonna be extended in the future for other cases which actually depend on the error type. My bad - I didn't clarify that before implementation, and I don't mind getting rid of this (I think it's not a big deal with builder macro, but more unsure about CallError), but I would like first to clarify this in order to prevent reverting the changes back and forth.

@tomusdrw is there any reason to have 2 separate enum variants for CallError? We don't use them at all

Also, do you think we should commit to derive_builder and if yes, should we also use it in other places?

@debris The first line in the linked comment contains the answer:

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).

That previous PR was parsing the string to figure out that the call was reverted. So yes, I believe it's justified to have two variants.

Regarding derive_builder: I used it, cause it made the code nice and concise so it could fit in the comment. You can implement the methods manually or not use the builder pattern at all.

I think it's still worth to consider a refactor of some of the builders we already have in the code to have them derived to reduce boilerplate.

my 2c on derive_builder: we already use derive_more and other proc macros so I think the "heavy" dependencies on syn, quote and proc_macro2 are already in-tree (need to be updated to the 1.0 releases though) so I don't think we add 5 new dependencies with this so think it's ok?

debris · 2019-10-18T14:31:13Z

ethcore/call-contract/src/call_contract.rs

+	Reverted(String),
+	/// Other error
+	Other(String),
+}


api consumer doesn't doesn't care whether the call is reverted has been reverted or not, which, imo makes this new data structure redundant

debris · 2019-10-18T14:35:26Z

ethcore/engine/src/engine.rs

@@ -381,6 +382,9 @@ pub trait Engine: Sync + Send {
 	fn decode_transaction(&self, transaction: &[u8]) -> Result<UnverifiedTransaction, transaction::Error> {
 		self.machine().decode_transaction(transaction)
 	}
+
+	/// Set reporting
+	fn set_reporting(&self, _benign_reporting: ReportingConfig, _malicious_reporting: ReportingConfig) {}


this new method should not be here, cause:
a) it's engine specific
b) it's used only when engine is created, so it should be a part of engine creation process

Nice catch, thank you! :) I used engine_signer logic as an example to introduce this one, but I have missed that signer can be changed during the lifetime of the Engine, which we don't need to do in this case. So this one and mutexes shall go away.

debris · 2019-10-18T14:37:36Z

ethcore/engines/authority-round/src/lib.rs

+	benign_reporting: Mutex<ValidatorReporting>,
+	malicious_reporting: Mutex<ValidatorReporting>,


those values are set only once per lifetime of an engine - after it's creation, so they should not wrapped in the Mutex and should be set in AuthorityRound::new method

debris · 2019-10-18T14:41:11Z

ethcore/engines/validator-set/reporting-config/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+description = "Validators reporting config"
+name = "validator-reporting-config"


creating a new library for a single structure that is a config option for a validator-set library doesn't make sense. The content of this library should be a part of validator-set lib

The main intention to move this into a separate module was the idea of not introducing other dependancies to the engine crate, which validator-set would bring with necessary to use just one struct from there. This was inspired by call-contract and other similar crates.

So you think it would be ok, to add validator-set to the Engine?

So you think it would be ok, to add validator-set to the Engine?

No, but if you use ReportingConfig in the construction phase of the validator-set engine, the Engine trait doesn't have to know about this ReportingConfig struct at all

debris · 2019-10-18T14:43:20Z

ethcore/machine/src/tx_filter.rs

 							.and_then(|value| decoder.decode(&value).map_err(|e| e.to_string()))
 							.map(|(p, f)| (p.low_u32(), f))
 							.unwrap_or_else(|e| {
-								error!(target: "tx_filter", "Error calling tx permissions contract: {:?}", e);


{} implies displaying less information than {:?}. Why was it changed?

Can't tell exactly now from top of my head, but I think this formatting is used because we map error to string before, so debug fmt is not really needed here anymore. Will double check this when I'm with my laptop next week.

debris · 2019-10-21T11:53:43Z

Thank you @VladLupashevskyi for the answers! For now, let's change the way validator-set engine is initialized and let's wait for @tomusdrw answers regarding call_contract.rs. You are right that it doesn't make sense to make changes back and forth :)

…into val-call � Conflicts: � Cargo.lock � ethcore/call-contract/src/call_contract.rs � ethcore/engines/authority-round/src/lib.rs � ethcore/private-tx/src/key_server_keys.rs � ethcore/src/client/client.rs � ethcore/src/test_helpers/test_client.rs � miner/src/service_transaction_checker.rs � parity/run.rs � updater/src/updater.rs

VladLupashevskyi · 2019-11-16T10:21:39Z

I was trying to change the way how validator-set engine is initialized and I found out that it will be a bit tricky to pass CLI args to it, since AuthorityRound::new finally uses Spec::load which is dependent on json specification and SpecParams, so I think the simplest way to do that is pass ReportingConfig there. However I'm not sure that it logically would fit there.

@debris What do you think about this approach, would it make sense to extend SpecParams?

dvdplm · 2019-11-17T09:48:25Z

ethcore/call-contract/src/call_contract.rs

+}
+
+#[derive(Builder, Default, Clone)]
+#[builder(default)]


my 2c on derive_builder: we already use derive_more and other proc macros so I think the "heavy" dependencies on syn, quote and proc_macro2 are already in-tree (need to be updated to the 1.0 releases though) so I don't think we add 5 new dependencies with this so think it's ok?

dvdplm · 2019-11-17T09:49:57Z

ethcore/call-contract/src/call_contract.rs

+impl CallOptions {
+	/// Convenience method for creating the most common use case.
+	pub fn new(contract: Address, data: Bytes) -> Self {
+		CallOptionsBuilder::default().contract_address(contract).data(data).build().unwrap()


No unwrap()s allowed outside tests; you need expect with a proper proof of why the inputs are guaranteed to be valid.

dvdplm · 2019-11-17T09:51:47Z

ethcore/call-contract/src/call_contract.rs

+#[derive(Builder, Default, Clone)]
+#[builder(default)]
+/// Options to make a call to contract
+pub struct CallOptions {


I think this should be called CallArgs – it's not really passing in options, is it? It's the actual arguments to the contract call.

dvdplm · 2019-11-17T09:52:14Z

ethcore/call-contract/src/call_contract.rs

-		address: Address,
-		data: Bytes
-	) -> Result<Bytes, String>;
+	/// Executes a transient call to a contract at given `BlockId`.


I don't know what a "transient" call is: can you help me understand?

dvdplm · 2019-11-17T09:58:40Z

ethcore/engines/validator-set/reporting-config/src/lib.rs

+// You should have received a copy of the GNU General Public License
+// along with Parity Ethereum.  If not, see <http://www.gnu.org/licenses/>.
+
+/// Reporting config


Regardless of where this ends up we need more docs here. What is its purpose, what do the variants mean, which engines is it relevant for etc.

dvdplm · 2019-11-17T10:18:14Z

ethcore/machine/src/tx_filter.rs

@@ -109,11 +109,12 @@ impl TransactionFilter {
 				match version_u64 {
 					2 => {
 						let (data, decoder) = transact_acl::functions::allowed_tx_types::call(sender, to, value);
-						client.call_contract(BlockId::Hash(*parent_hash), contract_address, data)
+						client.call_contract(BlockId::Hash(*parent_hash), CallOptions::new(contract_address, data))
+							.map_err(|e| e.to_string())


dvdplm · 2019-11-17T10:20:17Z

parity/cli/mod.rs

@@ -834,6 +834,19 @@ usage! {
 			"--max-round-blocks-to-import=[S]",
 			"Maximal number of blocks to import for each import round.",

+		["Reporting configuration"]


Suggested change

["Reporting configuration"]

["Validator behaviour reporting"]

dvdplm · 2019-11-17T10:20:45Z

parity/cli/mod.rs

+		["Reporting configuration"]
+			ARG arg_benign_reporting: (Option<String>) = None, or |c: &Config| c.reporting.as_ref()?.benign_reporting.clone(),
+			"--benign_reporting=[force|call|disable]",
+			"AuRa benign reporting configuration.",


Document what the options mean. And defaults.

dvdplm · 2019-11-17T10:20:51Z

parity/cli/mod.rs

+
+			ARG arg_malicious_reporting: (Option<String>) = None, or |c: &Config| c.reporting.as_ref()?.malicious_reporting.clone(),
+			"--malicious_reporting=[force|call|disable]",
+			"AuRa malicious reporting configuration.",


Document what the options mean. And defaults.

dvdplm · 2019-11-17T10:21:36Z

parity/cli/mod.rs

+
+			ARG arg_report_cache_call_every: (u64) = 100u64, or |c: &Config| c.reporting.as_ref()?.cache_call_every.clone(),
+			"--report_cache_call_every=[BLOCK_NUM]",
+			"AuRa cache call report successfulness for specified amount if blocks.",


Not sure I understand what this means.

VladLupashevskyi added 10 commits September 6, 2019 23:12

Introduce validator reporting config

5b17311

WIP: Bail out in contract call if transaction is reverted

7eeea72

Fix case when checked_last_block is none

a6d9b6a

Refactor call_contract with CallOptions and CallError

a9079db

Code style fix

14b35f4

Set proper builder params

9468b47

Fix typo

a7901ea

Add Solidity error decoding

874b17b

Make 50kk gas as default for CallOptionsBuilder

33714f7

Pass engine signer address from Engine to validator set for call

492f6e8

jam10o-new added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 8, 2019

VladLupashevskyi added 2 commits September 9, 2019 00:56

Fix tests and secret-store

f839e7a

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

964ecaf

…into val-call

VladLupashevskyi added 4 commits September 13, 2019 18:16

Fix typo

535c583

Remove unused import

a8537b5

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

57fd33d

…into val-call � Conflicts: � ethcore/src/client/client.rs

Add missing import

507233a

jam10o-new added this to the 2.7 milestone Sep 13, 2019

debris suggested changes Oct 18, 2019

View reviewed changes

debris added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 18, 2019

dvdplm suggested changes Nov 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

VladLupashevskyi commented Sep 8, 2019

parity-cla-bot commented Sep 8, 2019

VladLupashevskyi commented Sep 8, 2019

dvdplm commented Sep 11, 2019

VladLupashevskyi commented Sep 13, 2019

VladLupashevskyi commented Sep 13, 2019

debris left a comment

debris Oct 18, 2019

VladLupashevskyi Oct 18, 2019

debris Oct 21, 2019

tomusdrw Oct 21, 2019 •

edited

Loading

dvdplm Nov 17, 2019

debris Oct 18, 2019

debris Oct 18, 2019

VladLupashevskyi Oct 18, 2019

debris Oct 18, 2019

debris Oct 18, 2019

VladLupashevskyi Oct 18, 2019

debris Oct 21, 2019

debris Oct 18, 2019

VladLupashevskyi Oct 18, 2019

debris commented Oct 21, 2019

VladLupashevskyi commented Nov 16, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

dvdplm Nov 17, 2019

		benign_reporting: Mutex<ValidatorReporting>,
		malicious_reporting: Mutex<ValidatorReporting>,

	["Reporting configuration"]
	["Validator behaviour reporting"]

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

Are you sure you want to change the base?

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

Conversation

VladLupashevskyi commented Sep 8, 2019

parity-cla-bot commented Sep 8, 2019

VladLupashevskyi commented Sep 8, 2019

dvdplm commented Sep 11, 2019

VladLupashevskyi commented Sep 13, 2019

VladLupashevskyi commented Sep 13, 2019

debris 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

tomusdrw Oct 21, 2019 • edited Loading

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

Choose a reason for hiding this comment

debris commented Oct 21, 2019

VladLupashevskyi commented Nov 16, 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 Oct 21, 2019 •

edited

Loading