Skip to content

fix: dryRun check + fix multiple solutions bug#443

Merged
niklasad1 merged 7 commits intomainfrom
na-add-dryrun-monitor
Jan 11, 2023
Merged

fix: dryRun check + fix multiple solutions bug#443
niklasad1 merged 7 commits intomainfrom
na-add-dryrun-monitor

Conversation

@niklasad1
Copy link
Copy Markdown
Contributor

No description provided.

src/monitor.rs Outdated
.tx()
.create_signed_with_nonce(&tx, &*signer, nonce, ExtrinsicParams::default())?;

let outcome = api.rpc().dry_run(xt.encoded(), None).await?;
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.

dry run the extrinsic before submitting, if that fails bail

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.

Should be optional, via a flag, in case we are running against a node that does not support unsafe RPCs.

Perhaps the default can be with dry-run, and --no-dry-run flag added.

Or, if you prefer to keep it flexible, we always do-run iff it is available

Lastly, needs to inform devops that our nodes should expose unsafe RPCs :)

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.

let's add a flag then forgot that that RPC is unsafe

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

@niklasad1 niklasad1 changed the title fix: dryRun check + fix multiple solutions bug fix: dryRun check + fix multiple solutions bug Dec 20, 2022
@niklasad1 niklasad1 requested a review from kianenigma January 3, 2023 13:52
@kianenigma
Copy link
Copy Markdown
Contributor

Please add these project to the NPoS project as it helps me find them faster 👍

src/opt.rs Outdated
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.

Copy link
Copy Markdown
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

niklasad1 and others added 3 commits January 11, 2023 09:44
@niklasad1 niklasad1 merged commit fcbbeef into main Jan 11, 2023
@niklasad1 niklasad1 deleted the na-add-dryrun-monitor branch January 11, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants