Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verify-chain node client arg #3678

Merged
merged 6 commits into from
Jan 7, 2022
+125 −29
Merged

verify-chain node client arg #3678

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6f592f
add parameter fast_validation to the api
deevope Dec 26, 2021
880a65e
arg `verify-chain` on node client
deevope Dec 26, 2021
1e8ba0c
arg verify chain + read_timeout parameter
deevope Dec 26, 2021
7547631
change timeout to 2hours
deevope Dec 27, 2021
eeeee1a
update with tromp review
deevope Dec 30, 2021
12e79c3
2nd comment from tromp
deevope Dec 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions api/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub fn get<T>(url: &str, api_secret: Option<String>) -> Result<T, Error>
where
for<'de> T: Deserialize<'de>,
{
handle_request(build_request(url, "GET", api_secret, None)?)
handle_request(build_request(url, "GET", api_secret, None)?, None)
}

/// Helper function to easily issue an async HTTP GET request against a given
Expand All @@ -52,21 +52,26 @@ where
/// on a given URL that returns nothing. Handles request
/// building and response code checking.
pub fn get_no_ret(url: &str, api_secret: Option<String>) -> Result<(), Error> {
send_request(build_request(url, "GET", api_secret, None)?)?;
send_request(build_request(url, "GET", api_secret, None)?, None)?;
Ok(())
}

/// Helper function to easily issue a HTTP POST request with the provided JSON
/// object as body on a given URL that returns a JSON object. Handles request
/// building, JSON serialization and deserialization, and response code
/// checking.
pub fn post<IN, OUT>(url: &str, api_secret: Option<String>, input: &IN) -> Result<OUT, Error>
pub fn post<IN, OUT>(
url: &str,
api_secret: Option<String>,
input: &IN,
read_timeout: Option<u64>,
) -> Result<OUT, Error>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@deevope deevope Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

where
IN: Serialize,
for<'de> OUT: Deserialize<'de>,
{
let req = create_post_request(url, api_secret, input)?;
handle_request(req)
handle_request(req, read_timeout)
}

/// Helper function to easily issue an async HTTP POST request with the
Expand Down Expand Up @@ -94,7 +99,7 @@ pub fn post_no_ret<IN>(url: &str, api_secret: Option<String>, input: &IN) -> Res
where
IN: Serialize,
{
send_request(create_post_request(url, api_secret, input)?)?;
send_request(create_post_request(url, api_secret, input)?, None)?;
Ok(())
}

Expand All @@ -110,7 +115,7 @@ pub async fn post_no_ret_async<IN>(
where
IN: Serialize,
{
send_request_async(create_post_request(url, api_secret, input)?).await?;
send_request_async(create_post_request(url, api_secret, input)?, None).await?;
Ok(())
}

Expand Down Expand Up @@ -155,11 +160,11 @@ where
build_request(url, "POST", api_secret, Some(json))
}

fn handle_request<T>(req: Request<Body>) -> Result<T, Error>
fn handle_request<T>(req: Request<Body>, read_timeout: Option<u64>) -> Result<T, Error>
where
for<'de> T: Deserialize<'de>,
{
let data = send_request(req)?;
let data = send_request(req, read_timeout)?;
serde_json::from_str(&data).map_err(|e| {
e.context(ErrorKind::ResponseError("Cannot parse response".to_owned()))
.into()
Expand All @@ -170,17 +175,24 @@ async fn handle_request_async<T>(req: Request<Body>) -> Result<T, Error>
where
for<'de> T: Deserialize<'de> + Send + 'static,
{
let data = send_request_async(req).await?;
let data = send_request_async(req, None).await?;
let ser = serde_json::from_str(&data)
.map_err(|e| e.context(ErrorKind::ResponseError("Cannot parse response".to_owned())))?;
Ok(ser)
}

async fn send_request_async(req: Request<Body>) -> Result<String, Error> {
async fn send_request_async(
req: Request<Body>,
read_timeout: Option<u64>,
) -> Result<String, Error> {
let https = hyper_rustls::HttpsConnector::new();
let read_timeout = match read_timeout {
Some(v) => Duration::from_secs(v),
None => Duration::from_secs(20),
};
let mut connector = TimeoutConnector::new(https);
connector.set_connect_timeout(Some(Duration::from_secs(20)));
connector.set_read_timeout(Some(Duration::from_secs(20)));
connector.set_read_timeout(Some(read_timeout));
connector.set_write_timeout(Some(Duration::from_secs(20)));
let client = Client::builder().build::<_, Body>(connector);

Expand All @@ -205,11 +217,11 @@ async fn send_request_async(req: Request<Body>) -> Result<String, Error> {
Ok(String::from_utf8_lossy(&raw).to_string())
}

pub fn send_request(req: Request<Body>) -> Result<String, Error> {
pub fn send_request(req: Request<Body>, read_timeout: Option<u64>) -> Result<String, Error> {
let mut rt = Builder::new()
.basic_scheduler()
.enable_all()
.build()
.map_err(|e| ErrorKind::RequestError(format!("{}", e)))?;
rt.block_on(send_request_async(req))
rt.block_on(send_request_async(req, read_timeout))
}
4 changes: 2 additions & 2 deletions api/src/handlers/chain_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use assume_valid_rangeproofs_kernels here as well?

w(&self.chain)?
.validate(true)
.validate(fast_validation)
.map_err(|_| ErrorKind::Internal("chain error".to_owned()).into())
}
}
Expand Down
7 changes: 5 additions & 2 deletions api/src/owner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@tromp tromp Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

///
/// # Returns
/// * Result Containing:
/// * `Ok(())` if the validation was done successfully
/// * or [`Error`](struct.Error.html) if an error is encountered.
///

pub fn validate_chain(&self) -> Result<(), Error> {
pub fn validate_chain(&self, fast_validation: bool) -> Result<(), Error> {
let chain_validation_handler = ChainValidationHandler {
chain: self.chain.clone(),
};
chain_validation_handler.validate_chain()
chain_validation_handler.validate_chain(fast_validation)
}

/// Trigger a compaction of the chain state to regain storage space.
Expand Down
12 changes: 6 additions & 6 deletions api/src/owner_rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub trait OwnerRpc: Sync + Send {
{
"jsonrpc": "2.0",
"method": "validate_chain",
"params": [],
"params": ["false"],
"id": 1
}
# "#
Expand All @@ -100,7 +100,7 @@ pub trait OwnerRpc: Sync + Send {
# );
```
*/
fn validate_chain(&self) -> Result<(), ErrorKind>;
fn validate_chain(&self, fast_validation: bool) -> Result<(), ErrorKind>;

/**
Networked version of [Owner::compact_chain](struct.Owner.html#method.compact_chain).
Expand Down Expand Up @@ -363,8 +363,8 @@ impl OwnerRpc for Owner {
Owner::get_status(self).map_err(|e| e.kind().clone())
}

fn validate_chain(&self) -> Result<(), ErrorKind> {
Owner::validate_chain(self).map_err(|e| e.kind().clone())
fn validate_chain(&self, fast_validation: bool) -> Result<(), ErrorKind> {
Owner::validate_chain(self, fast_validation).map_err(|e| e.kind().clone())
}

fn reset_chain_head(&self, hash: String) -> Result<(), ErrorKind> {
Expand Down Expand Up @@ -403,7 +403,7 @@ macro_rules! doctest_helper_json_rpc_owner_assert_response {
// create temporary grin server, run jsonrpc request on node api, delete server, return
// json response.

{
{
/*use grin_servers::test_framework::framework::run_doctest;
use grin_util as util;
use serde_json;
Expand Down Expand Up @@ -437,6 +437,6 @@ macro_rules! doctest_helper_json_rpc_owner_assert_response {
serde_json::to_string_pretty(&expected_response).unwrap()
);
}*/
}
}
};
}
2 changes: 1 addition & 1 deletion servers/src/mining/mine_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ fn create_coinbase(dest: &str, block_fees: &BlockFees) -> Result<CbData, Error>

trace!("Sending build_coinbase request: {}", req_body);
let req = api::client::create_post_request(url.as_str(), None, &req_body)?;
let res: String = api::client::send_request(req).map_err(|e| {
let res: String = api::client::send_request(req, None).map_err(|e| {
let report = format!(
"Failed to get coinbase from {}. Is the wallet listening? {}",
dest, e
Expand Down
33 changes: 31 additions & 2 deletions src/bin/cmd/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,20 @@ impl HTTPNodeClient {
method: &str,
params: &serde_json::Value,
) -> Result<D, Error> {
let read_timeout: Option<u64> = match method {
// 2hours
"validate_chain" => Some(7200),
// (default) 20sec
_ => None,
};
let url = format!("http://{}{}", self.node_url, ENDPOINT);
let req = build_request(method, params);
let res =
client::post::<Request, Response>(url.as_str(), self.node_api_secret.clone(), &req);
let res = client::post::<Request, Response>(
url.as_str(),
self.node_api_secret.clone(),
&req,
read_timeout,
);

match res {
Err(e) => {
Expand Down Expand Up @@ -149,6 +159,21 @@ impl HTTPNodeClient {
e.reset().unwrap();
}

pub fn validate_chain(&self, fast_validation: bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@deevope deevope Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut e = term::stdout().unwrap();
let params = json!([fast_validation]);
writeln!(
e,
"Checking the state of the chain. This might take time..."
)
.unwrap();
match self.send_json_request::<()>("validate_chain", &params) {
Ok(_) => writeln!(e, "Successfully validated chain.").unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This output should reflect whether rangeproofs and kernels were validated.

Err(err) => writeln!(e, "Failed to validate chain: {:?}", err).unwrap(),
}
e.reset().unwrap();
}

pub fn ban_peer(&self, peer_addr: &SocketAddr) {
let mut e = term::stdout().unwrap();
let params = json!([peer_addr]);
Expand Down Expand Up @@ -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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

node_client.validate_chain(fast_validation);
}
("ban", Some(peer_args)) => {
let peer = peer_args.value_of("peer").unwrap();

Expand Down
8 changes: 8 additions & 0 deletions src/bin/grin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ subcommands:
- hash:
help: The header hash to reset to
required: true
- verify-chain:
about: Verify the chain state
args:
- fast:
help: if present, skip validation of the rangeproof associated with each unspent output and all kernels
short: f
long: fast
takes_value: false
- invalidateheader:
about: Adds header hash to denylist
args:
Expand Down