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.

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

contract download - remove exception on empty validatedrootindex #251

Closed
ixje opened this issue Sep 7, 2022 · 2 comments
Closed

contract download - remove exception on empty validatedrootindex #251

ixje opened this issue Sep 7, 2022 · 2 comments

Comments

@ixje
Copy link
Contributor

ixje commented Sep 7, 2022

The contract download feature is intended to support local development. There is a security check that seems unnecessary when used for this purpose, specifically here

stateHeight = validatedRootIndex.HasValue ? validatedRootIndex.Value
: throw new Exception($"Null \"{nameof(validatedRootIndex)}\" in state height response");

this basically forbids using any node that is not voted into the list of state validators by the consensus committee. It should be possible to use nodes that you trust e.g. I want to use our COZ nodes because the NEO nodes are very slow. This could be done by using localrootindex instead of validatedrootindex.

I see 3 ways of solving this

  1. always use localrootindex instead of validatedrootindex
  2. have a flag like --allow-localroot to use localrootindex if validatedrootindex is null
  3. the inverse of 2. where we allow localrootindex by default unless passing some flag like --require-rootvalidated-state
@devhawk
Copy link
Contributor

devhawk commented Sep 7, 2022

I wouldn't characterize this as a security check. If the user doesn't explicitly specify a block height to download, the command gets "most recent available". How the command chooses "most recent available" doesn't seem that critical.

How about a slight variation on option 1: Use localrootindex if validatedrootindex is not available. Something like this maybe?

if (stateHeight == 0)
{
    uint? localRootIndex, validatedRootIndex;
    try
    {
        (localRootIndex, validatedRootIndex) = await stateAPI.GetStateHeightAsync().ConfigureAwait(false);
    }
    catch (RpcException e) when (e.Message.Contains("Method not found"))
    {
        throw new Exception(
            "Could not get state information. Make sure the remote RPC server has state service support");
    }

    stateHeight = validatedRootIndex.HasValue 
        ? validatedRootIndex.Value
        : localRootIndex.HasValue
            ? localRootIndex.Value
            : throw new Exception($"GetStateHeight did not return local or validated root index");
}

@devhawk
Copy link
Contributor

devhawk commented Sep 8, 2022

FYI, can use null coalesce operator instead:

stateHeight = validatedRootIndex ?? localRootIndex 
    ?? throw new Exception($"GetStateHeight did not return local or validated root index");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants