Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ mod tests {
solver: Solver::SeqPhragmen { iterations: 10 },
submission_strategy: SubmissionStrategy::IfLeading,
seed_or_path: "//Alice".to_string(),
dry_run: false,
}),
}
);
Expand Down
34 changes: 31 additions & 3 deletions src/monitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,15 @@ fn kill_main_task_if_critical_err(tx: &tokio::sync::mpsc::UnboundedSender<Error>
JsonRpseeError::Call(CallError::Custom(e)) => {
const BAD_EXTRINSIC_FORMAT: i32 = 1001;
const VERIFICATION_ERROR: i32 = 1002;
use jsonrpsee::types::error::ErrorCode;

// Check if the transaction gets fatal errors from the `author` RPC.
// It's possible to get other errors such as outdated nonce and similar
// but then it should be possible to try again in the next block or round.
if e.code() == BAD_EXTRINSIC_FORMAT || e.code() == VERIFICATION_ERROR {
if e.code() == BAD_EXTRINSIC_FORMAT ||
e.code() == VERIFICATION_ERROR || e.code() ==
ErrorCode::MethodNotFound.code()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we got back method not found is most likely because the RPC method was unsafe, then just terminate.

all solutions will be rejected anyway then

{
let _ = tx.send(Error::Subxt(SubxtError::Rpc(
RpcError::ClientError(Box::new(CallError::Custom(e))),
)));
Expand Down Expand Up @@ -215,8 +219,6 @@ async fn mine_and_submit_solution<T>(
},
};

let _lock = submit_lock.lock().await;

if let Err(e) =
ensure_no_previous_solution::<T::Solution>(&api, hash, signer.account_id()).await
{
Expand All @@ -231,6 +233,8 @@ async fn mine_and_submit_solution<T>(
return
}

let _lock = submit_lock.lock().await;

let (solution, score) =
match epm::fetch_snapshot_and_mine_solution::<T>(&api, Some(hash), config.solver)
.timed()
Expand Down Expand Up @@ -292,6 +296,20 @@ async fn mine_and_submit_solution<T>(
return
}

if let Err(e) =
ensure_no_previous_solution::<T::Solution>(&api, best_head, signer.account_id()).await
{
log::debug!(
target: LOG_TARGET,
"ensure_no_previous_solution failed: {:?}; skipping block: {:?}",
e,
best_head,
);

kill_main_task_if_critical_err(&tx, e);
return
}

match ensure_solution_passes_strategy(&api, best_head, score, config.submission_strategy)
.timed()
.await
Expand Down Expand Up @@ -323,6 +341,7 @@ async fn mine_and_submit_solution<T>(
hash,
nonce,
config.listen,
config.dry_run,
)
.timed()
.await
Expand Down Expand Up @@ -419,13 +438,22 @@ async fn submit_and_watch_solution<T: MinerConfig + Send + Sync + 'static>(
hash: Hash,
nonce: u32,
listen: Listen,
dry_run: bool,
) -> Result<(), Error> {
let tx = epm::signed_solution(RawSolution { solution, score, round })?;

let xt = api
.tx()
.create_signed_with_nonce(&tx, &*signer, nonce, ExtrinsicParams::default())?;

if dry_run {
match api.rpc().dry_run(xt.encoded(), None).await? {
Ok(Ok(())) => (),
Ok(Err(e)) => return Err(Error::TransactionRejected(format!("{:?}", e))),
Err(e) => return Err(Error::TransactionRejected(e.to_string())),
};
}

let mut status_sub = xt.submit_and_watch().await.map_err(|e| {
log::warn!(target: LOG_TARGET, "submit solution failed: {:?}", e);
e
Expand Down
7 changes: 7 additions & 0 deletions src/opt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ pub struct MonitorConfig {
/// configured, it might re-try and lose funds through transaction fees/deposits.
#[clap(long, short, env = "SEED")]
pub seed_or_path: String,

/// Verify the submission by `dry-run` the extrinsic to check the validity.
/// If it fails then the block is ignored.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does this mean? What block is ignored?

I guess what you are trying to say is: If this is set, against a node that does not support unsafe RPCs, then we don't submit anything?

I think we should test this at startup, and immediately terminate. Every time we connect to a node, if we have this flag set, we should check it it supports the unsafe stuff or not.

Copy link
Copy Markdown
Contributor Author

@niklasad1 niklasad1 Jan 6, 2023

Choose a reason for hiding this comment

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

Nah, it means if the dry-run fails on a solution at block x then we simply emit a warning and drop that solution.
Then we try on the next block.

Fair enough, it could be good to have a proper error if the RPC call itself is denied because of "unsafe", currently we just terminate when that occurs with the error from substrate.

///
/// This requires a RPC endpoint that exposes unsafe RPC methods.
#[clap(long)]
pub dry_run: bool,
}

#[derive(Debug, Clone, Parser)]
Expand Down