Config and refactor

[austinabell]

Creating config for Atlantis specific functionality and testing, maps ETH test forks to corresponding ETC chain config (or RuleSet).

Refactored jump_table.go to allow for adding other variables for the functionality of upcoming EIPs and reduce merge conflicts when implementing the variables to map to each opcode.

For example for adding the writes variable to map which variables modify state for STATICCALL implementation, the type can be easily changed to:

type jumpPtr struct {
	fn    instrFn
	valid bool

	writes bool
}

and a state modifying opcode can be easily tagged as this variable with:

	jumpTable[SSTORE] = jumpPtr{
		...
		writes: true,
	}

also, can correctly just add the opcodes per Atlantis block by putting them in a

	if ruleset.IsHomestead(blockNumber) {
		jumpTable[...] = jumpPtr{
			...
		}
	}

function.

[GregTheGreek]

LGTM -- Really nice refactor

[soc1c]

utACK + 2 questions

[soc1c]

Can you elaborate on this? why is EIP-158 mapped to Homestead and Atlantis here?

[austinabell]

Essentially, when the HomesteadBlock and AtlantisBlock are defined, it enables the Homestead and Atlantis specific interactions through the RuleSet interface implementations of

func (r RuleSet) IsHomestead(n *big.Int) bool {
	return n.Cmp(r.HomesteadBlock) >= 0
}

func (r RuleSet) IsAtlantis(n *big.Int) bool {
	return n.Cmp(r.AtlantisBlock) >= 0
}

In this case. I had mapped EIP158 to Atlantis because we were including the state clearing functionality (EIP158/161) in the Atlantis fork. Now that I think about it, I should just skip the EIP158 fork subtests altogether, because if a test uses an opcode included in Atlantis for example, it will fail those specific tests. I will change this now along with other changes.

I will make this change in a fixing commit but will hopefully get input into what the best way to go about this is.

[soc1c]

There is no Byzantium, Constantinople, and Petersburg in Classic. Only Atlantis and Agharta.

[austinabell]

Yes, I am aware. the point of this mapping is because the tests submodule we are using for testing this, separates the "post" subtests into their respective fork, and I am making config map to the respective ETC RuleSet.

	// Ruleset mapping from ETH network to ETC
	ruleSet, ok := Forks[subtest.Fork]
	if !ok {
		return UnsupportedForkError{subtest.Fork}
	}

Now that I think about it though, it may be a better experience for the people implementing Agharta to just hit the UnsupportedForkError when they remove the skipping of Constantinople and Petersburg tests. Right now, these fork mappings are ignored because they are intentionally skipped in state_test.go

					// Not supported implementations to test
					if subtest.Fork == "Constantinople" || subtest.Fork == "ConstantinopleFix" {
						continue
					}

but when the functionality does get implemented, the mapping would end up looking something like this:

...
"Constantinople": {
	HomesteadBlock: big.NewInt(0),
	AtlantisBlock:  big.NewInt(0),
	AghartaBlock:  big.NewInt(0),
},
...

[austinabell]

For reference, ETC previously hard coded in a RuleSet for each individual test:

func TestEIP150HomesteadBounds(t *testing.T) {
	ruleSet := RuleSet{
		HomesteadBlock:           new(big.Int),
		HomesteadGasRepriceBlock: big.NewInt(2457000),
	}

	fn := filepath.Join(stateTestDir, "EIP150", "Homestead", "stBoundsTest.json")
	if err := RunStateTest(ruleSet, fn, StateSkipTests); err != nil {
		t.Error(err)
	}
}

and I had previously hard coded in a RuleSet which defaulted HomesteadBlock to 0, but I had always planned on changing to mapping to specific configurations to handle the edge cases and test combinations. This was necessary when testing the new opcode EIPs because to be completely backwards compatible, those opcodes should only be active when the IsAtlantis(..) function returns true and the tests before Atlantis (mapping from Byzantium in tests) should not utilize those opcode functionalities.

Sorry if I did a bad job explaining all of this, I can try to clarify here or on a call if anyone is confused.

[austinabell]

Also one other note, haven't needed to yet but may need to include these blocks in the fork config mapping:

HomesteadGasRepriceBlock *big.Int
DiehardBlock             *big.Int
ExplosionBlock           *big.Int

Reason I hadn't is because those gas calculations haven't mattered for the tests we are using/ functionality we are implementing. I haven't looked too in depth, but we may want to chat about what exactly the configuration should be to make sure tests function as intended.

[austinabell]

Hold on for a second before reviewing, I need to change how Frontier is mapped since it throws an exception when a test file with that fork subtest is called. (I am not sure in go how to create the struct without any values defined but looking into it now)

Edit: My guess was not the problem, it was the previous implementation of those methods assuming their hard coded config was always set properly

[noot]

getting an error when running tests:

# github.com/eth-classic/go-ethereum/core/vm/runtime [github.com/eth-classic/go-ethereum/core/vm/runtime.test]
core/vm/runtime/runtime.go:70:15: cannot use ruleSet literal (type ruleSet) as type vm.RuleSet in assignment:
	ruleSet does not implement vm.RuleSet (missing IsAtlantis method)

looks like you need to implement IsAtlantis somewhere else?

[austinabell]

getting an error when running tests:

# github.com/eth-classic/go-ethereum/core/vm/runtime [github.com/eth-classic/go-ethereum/core/vm/runtime.test]
core/vm/runtime/runtime.go:70:15: cannot use ruleSet literal (type ruleSet) as type vm.RuleSet in assignment:
	ruleSet does not implement vm.RuleSet (missing IsAtlantis method)

looks like you need to implement IsAtlantis somewhere else?

Ah yes, my bad. Hadn't noticed the vm runtime one since I hadn't run the full test suite since I had added that function to the interface. I'll commit that fix right now.