`verify-chain` node client arg #3678

deevope · 2021-12-27T01:19:09Z

Following this feature suggestion #3609

Add the parameter on the api fast_validation
Add verify-chain node client argurment

Usage:
Full chain validation
grin client verify-chain

Partial chain validation
grin client verify-chain --fast

PS: This PR need the API to gracefully shutdown, need to merge #3677

phyro · 2021-12-28T23:22:59Z

Thanks for making it a reality! Sorry for the delay. I've set up a new system at home and have hit some unexpected issues with syncing for some reason and am still figuring why. I'll try to sync on the latest master tomorrow that contains some optimizations and see if I can make it sync and then check this one. One thing we'll have to check is whether this blocks other actions (api, block processing, potential sync requests, can we get banned if we fall behind but advertise availability) due to locking or similar since I expect it to be running for some time. Maybe we don't even care for these cases, but if nothing else, to document if that's the case.

tromp · 2021-12-29T12:32:50Z

api/src/client.rs

+	api_secret: Option<String>,
+	input: &IN,
+	read_timeout: Option<u64>,
+) -> Result<OUT, Error>


Instead of adding Option parameters to multiple methods, isn't it preferable to add a new timeout member,
and a method to change it from its default value of 20secs?

I did it a bit more cleaner but just adding a method in api/client.rs and trigger a change of timeout in function of the api method used in the request is a bit painful in the form it's built. If you have better idea, i'm open.
However these method are almost only used for cmd/client.rs

tromp · 2021-12-29T12:37:53Z

api/src/owner.rs

@@ -80,17 +80,20 @@ impl Owner {

 	/// Trigger a validation of the chain state.
 	///
+	/// # Arguments
+	/// * `fast_validation` - if false verify the rangeproof associated with each unspent output and all kernels


i'd prefer this is called something like assume_valid_rangeproofs_kernels,
to reflect its importance and potential for harm.
Also, describe it's behaviour in terms of - if true ...

tromp · 2021-12-29T12:42:24Z

src/bin/cmd/client.rs

@@ -149,6 +159,21 @@ impl HTTPNodeClient {
 		e.reset().unwrap();
 	}

+	pub fn validate_chain(&self, fast_validation: bool) {


how come this is a new method, rather than a changed method?

I don't really get what you mean here by a new or changed method?

I mean that it looks like you're adding a validate_chain method to HTTPNodeClient,
whereas elsewhere in this commit, there are changes to existing validate_chain methods.

ah ok, will call it, verify_chain to make it clear it's a new method.

I don't object to the name. If it behaves identical to those others, then the same name is appropriate. But I just wondered, since I'm not very familiar with this code, why HTTPNodeClient didn't need such a method before and does now.

tromp · 2021-12-29T12:43:17Z

src/bin/cmd/client.rs

+		)
+		.unwrap();
+		match self.send_json_request::<()>("validate_chain", &params) {
+			Ok(_) => writeln!(e, "Successfully validated chain.").unwrap(),


This output should reflect whether rangeproofs and kernels were validated.

tromp · 2021-12-29T12:44:07Z

src/bin/cmd/client.rs

@@ -191,6 +216,10 @@ pub fn client_command(client_args: &ArgMatches<'_>, global_config: GlobalConfig)
 			let hash = args.value_of("hash").unwrap();
 			node_client.invalidate_header(hash.to_string());
 		}
+		("verify-chain", Some(args)) => {
+			let fast_validation = if args.is_present("fast") { false } else { true };


This is simply let fast_validation = !args.is_present("fast")

tromp

See in-line comments.

tromp · 2021-12-30T12:38:21Z

api/src/handlers/chain_api.rs

@@ -53,9 +53,9 @@ pub struct ChainValidationHandler {
 }

 impl ChainValidationHandler {
-	pub fn validate_chain(&self) -> Result<(), Error> {
+	pub fn validate_chain(&self, fast_validation: bool) -> Result<(), Error> {


why not use assume_valid_rangeproofs_kernels here as well?

tromp · 2021-12-30T12:39:37Z

src/bin/cmd/client.rs

@@ -149,6 +158,27 @@ impl HTTPNodeClient {
 		e.reset().unwrap();
 	}

+	pub fn verify_chain(&self, fast_validation: bool) {


why not use the name assume_valid_rangeproofs_kernels here as well?

Done but i let --fast as the option for the arg verify-chain.

tromp

I recommend more use of the name assume_valid_rangeproofs_kernels
as indicated with in-line comments.

yeastplume · 2022-01-06T13:36:07Z

Reviewed requested changes, all good with me. Thanks for this PR at @deevope and for the detailed review @tromp !

deevope added 3 commits December 26, 2021 22:42

add parameter fast_validation to the api

d6f592f

arg verify-chain on node client

880a65e

arg verify chain + read_timeout parameter

1e8ba0c

deevope force-pushed the validate-chain branch from 8edee19 to 1e8ba0c Compare December 27, 2021 02:48

change timeout to 2hours

7547631

phyro requested review from phyro, yeastplume and tromp December 28, 2021 23:23

tromp reviewed Dec 29, 2021

View reviewed changes

update with tromp review

eeeee1a

deevope requested a review from tromp December 30, 2021 08:00

tromp reviewed Dec 30, 2021

View reviewed changes

tromp approved these changes Dec 30, 2021

View reviewed changes

2nd comment from tromp

12e79c3

yeastplume approved these changes Jan 6, 2022

View reviewed changes

yeastplume merged commit a3eebbc into mimblewimble:master Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`verify-chain` node client arg #3678

`verify-chain` node client arg #3678

deevope commented Dec 27, 2021 •

edited

Loading

phyro commented Dec 28, 2021 •

edited

Loading

tromp Dec 29, 2021

deevope Dec 30, 2021 •

edited

Loading

tromp Dec 29, 2021 •

edited

Loading

tromp Dec 29, 2021

deevope Dec 29, 2021 •

edited

Loading

tromp Dec 29, 2021

deevope Dec 29, 2021

tromp Dec 29, 2021

tromp Dec 29, 2021

tromp Dec 29, 2021

tromp left a comment

tromp Dec 30, 2021

tromp Dec 30, 2021

deevope Dec 30, 2021

tromp left a comment

yeastplume commented Jan 6, 2022

verify-chain node client arg #3678

verify-chain node client arg #3678

Conversation

deevope commented Dec 27, 2021 • edited Loading

phyro commented Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

deevope Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

tromp Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deevope Dec 29, 2021 • 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

tromp 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

tromp left a comment

Choose a reason for hiding this comment

yeastplume commented Jan 6, 2022

`verify-chain` node client arg #3678

`verify-chain` node client arg #3678

deevope commented Dec 27, 2021 •

edited

Loading

phyro commented Dec 28, 2021 •

edited

Loading

deevope Dec 30, 2021 •

edited

Loading

tromp Dec 29, 2021 •

edited

Loading

deevope Dec 29, 2021 •

edited

Loading