refactor: share EVM rpc package

[JonathanOppenheimer]

Why this should be merged

The subnet-evm rpc package was practically identical to the coreth rpc package. It makes no sense to maintain two copies of this code. This also makes moving to libevm's rpc package far easier, to look in one location and see what needs to be libevmified.

How this works

• Created graft/evm/rpc/ directory structure
• Copied all source files from coreth/rpc to the shared location
• Updated graft/evm/go.mod with required dependencies:
  • github.com/gorilla/websocket v1.5.0
  • github.com/deckarep/golang-set/v2 v2.1.0
  • golang.org/x/time v0.12.0
• Fixed test imports and updated TestNotify to work with standard libevm types

I also added main_test.go with a TestMain that automatically runs all RPC tests twice - once with C-Chain type registration and once with Subnet-EVM type registration.

This works by putting TestMain in package rpc_test (an external test package) instead of package rpc, which lets us import the emulate package without creating a circular dependency. We can then use emulate.CChain() and emulate.SubnetEVM() to properly register/cleanup types between test runs.

Thus when you run go test (which also automatically happens via CI -- you can check for yourself!), it:

1. Runs all tests with C-Chain types registered via emulate.CChain()
2. Cleans up the registration
3. Runs all tests again with Subnet-EVM types via emulate.SubnetEVM()
4. Reports if either variant fails

How this was tested

CI

Need to be documented in RELEASES.md?

No

[alarso16]

Don't we need to register the types for our tests? I think that was the only thing preventing us from using the geth rpc package

[JonathanOppenheimer]

Don't we need to register the types for our tests? I think that was the only thing preventing us from using the geth rpc package

The only place we registered was in an additional main file just in subnet-evm, and that was for test retrying for flakes. I removed that as we should not be doing test retries in the monorepo, and should just skip flaky tests entirely if they're flaky. Is there registration anywhere else?

[alarso16]

Don't we need to register the types for our tests? I think that was the only thing preventing us from using the geth rpc package

The only place we registered was in an additional main file just in subnet-evm, and that was for test retrying for flakes. I removed that as we should not be doing test retries in the monorepo, and should just skip flaky tests entirely if they're flaky. Is there registration anywhere else?

No, we didn't just retry failed tests. They also registered customtypes in each respective repo, so that's why the contents of the block changed when you moved them over. I'm not sure if we still need to be testing that functionality

[JonathanOppenheimer]

Don't we need to register the types for our tests? I think that was the only thing preventing us from using the geth rpc package

The only place we registered was in an additional main file just in subnet-evm, and that was for test retrying for flakes. I removed that as we should not be doing test retries in the monorepo, and should just skip flaky tests entirely if they're flaky. Is there registration anywhere else?

No, we didn't just retry failed tests. They also registered customtypes in each respective repo, so that's why the contents of the block changed when you moved them over. I'm not sure if we still need to be testing that functionality

I don't think this is true. There are two places in this PR where I removed customtypes.Register().

1. For a test retry loop. As explained above, I this this is unnecessary.
2. In TestMain of subnet-evm's subscription test. This differed from coreth, and only appeared to be present as we were still importing the old custom types instead of the libevm types package. I removed it to align the two, as I assume subnet-evm just didn't get updated.

Are there other important places in which I removed it?

[ARR4N]

If customtypes.Register() isn't called then the test isn't testing coreth / subnet-evm functionality in its entirety. I'd recommend using evm.RegisterLibEVMExtras() (which calls customtypes.Register()) instead of the individual registration as this guarantees that all hooks/payloads are registered.

This can alternately be done with emulate.CChain() and emulate.SubnetEVM() if you need to test both in the same package (i.e. the same test binary). If there is commonality between the testing then the best approach is probably something like:

func testCChainAndSubnetEVM(t *testing.T, testFn func(*testing.T)) {
  fn := func() error {
    testFn(t)
    return nil
  }

  t.Run("cchain", func(t *testing.T) {
    _ = emulate.CChain(fn)
  })
  t.Run("subnetEVM", func(t *testing.T) {
    _ = emulate.SubnetEVM(fn)
  })
}

func TestFoo(t *testing.T) {
  testCorethAndSubnetEVM(t, testFoo)
}

func testFoo(t *testing.T) {
  // this is where the actual testing occurs; i.e. just rename any existing TestFoo to testFoo
}

func TestBar(t *testing.T) {
  testCorethAndSubnetEVM(t, testBar)
}

func testBar(t *testing.T) {
  // as with testFoo()
}

The linter is going to tell you to call t.Helper() but it's better not to in this case otherwise all errors will be reported as being on the single line of the TestFoo() implementation, instead of on the specific line in testFoo().

[alarso16]

I believe we have decided to enforce that we don't want cyclic dependencies between coreth and evm, so we would probably have to "uplift" the customtypes packages first. Also, I have tried this in #4688, and trying to register all EVM imports in this package causes a cyclic dependency (unless you make a test package?)

[JonathanOppenheimer]

I believe we have decided to enforce that we don't want cyclic dependencies between coreth and evm, so we would probably have to "uplift" the customtypes packages first. Also, I have tried this in #4688, and trying to register all EVM imports in this package causes a cyclic dependency (unless you make a test package?)

Yeah I am trying this right now and was about to comment about the cyclic imports. The shared graft/evm/rpc package is a dependency used by coreth, which creates a circular dependency if we try to use vms/evm/emulate (which depends on coreth) in the RPC tests. This can be avoided if we just call the custom types registration directly or put it in a seperate test package.

I definitely think this + in conjunction with some sort of test main would be a good model for how to combine packages while testing separately. I've committed a new main_test.go which does this to some extent although simpler. What are the thoughts on this approach?

[alarso16]

Suggested change

	if cchainCode != 0 {
	return cchainCode
	}
	if subnetCode != 0 {
	return subnetCode
	}
	if cchainCode != 0 || subnetCode != 0 {
	return 1
	}

Don't just apply this, but is there any reason we care about the specific code?

[JonathanOppenheimer]

Not really, -- more work could be done to determine which of the two test variants failed, by returning extra information in these if cases.

[JonathanOppenheimer]

I am going to apply this but leave the comment open if you have further comments.

[JonathanOppenheimer]

I just learned Go does not have a ternary operator. Lame. 5 lines could be one!

[alarso16]

Why do you prefix all these with newlines?

[JonathanOppenheimer]

So there can be a blank line in between these two lines.

[JonathanOppenheimer]

it's just for style

[alarso16]

I think you shouldn't. Run go test -v ./... to see how go does it (there's no blank lines)

[alarso16]

Open question: Will this indirect dependency cause a giant headache down the road?

[JonathanOppenheimer]

I can't foresee any issue - do you have something in mind. It's because of importing emulate which imports each of the two repositories.

[alarso16]

The reason that we didn't want to allow evm to import coreth is because of the dependency tree. Even if it doesn't import it directly, this doesn't seem to avoid the problem

[JonathanOppenheimer]

I thought we didn't want to allow graft/evm to import coreth because it would prevent a direct uplift to vms/evm. An indirect dependency (esp that is a result of importing vms/evm itself) doesn't prevent tthat. What are your concerns regarding the dependency tree?

[alarso16]

Why is this in a separate file? You should be able to just keep the test there, since any conflicts when updating will be easier to see

[JonathanOppenheimer]

Because it has to be in a separate package, rpc_test to prevent import cycles.

[JonathanOppenheimer]

and if we change the whole old test file to be in rpc_test it doesn't work as a tonnn more stuff would either need to get exported to be accessible, or moved. This was the minimal change I could make.

[JonathanOppenheimer]

This is also why https://github.com/ava-labs/avalanchego/blob/72ea713ecc8af00b4264ec28bfe32cb054d622f4/graft/evm/rpc/testing.go exists now.

[alarso16]

Ahhh, I didn't realize that you didn't change all of the tests to run in that package.

[alarso16]

This works, but some of the package management stuff feels hacky. I don't have any better solutions, so maybe get an opinion from @ARR4N or @ceyonur. I'm not confident enough that this is the route we want to take to give a real approval

My concerns are:

• Importing coreth and subnet-evm was intended to be avoided in this package, but it has to be done to enable registrations. Part of this messiness is because we decided not to directly import coreth and subnet-evm, but it would be simpler if we could (see @ARR4N's suggestion above)
• This makes the dependency tree between the four modules a strongly connected graph, which is less than ideal. Would it be better to move the coreth/subnet-evm type registrations into the evm package first?
• Running two separate packages of tests in the same folder is quite gross (tbh didn't even know Go supports it)