-
Notifications
You must be signed in to change notification settings - Fork 27
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
Reuse Existing Compiled Code from Equivalent Program if Exists #67
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just a couple of tiny things
defer cleanup() | ||
|
||
programAddress := deployWasm(t, ctx, auth, l2client, rustFile("fallible")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separating the deploy function from the setup so that I can test reusing existing compilation at two different program addresses
system_tests/common_test.go
Outdated
@@ -929,6 +929,7 @@ func deployContract( | |||
) common.Address { | |||
deploy := deployContractInitCode(code, false) | |||
basefee := GetBaseFee(t, client, ctx) | |||
basefee.Mul(basefee, big.NewInt(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I don't understand why it's necessary specifically for this change.
TestProgramMemory fails about 50% of the time while trying to deploy the grow-120 contract with an error that PricePerGas is lower than baseFee. This is specifically for this test and contract (the contract cannot compile - but I don't understand how that's relevant)
Increasing baseFee like that does the trick, but there might be better solutions.
precompiles/ArbWasm.go
Outdated
func (con ArbWasm) ProgramVersion(c ctx, _ mech, program addr) (uint16, error) { | ||
return c.State.Programs().ProgramVersion(program) | ||
func (con ArbWasm) ProgramVersion(c ctx, evm mech, program addr) (uint16, error) { | ||
return c.State.Programs().ProgramVersion(evm.StateDB.GetCodeHash(program)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should charge for this
contractAddress: scope.Contract.Address(), | ||
msgSender: scope.Contract.Caller(), | ||
msgValue: common.BigToHash(scope.Contract.Value()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contract
is scope.Contract
func (p Programs) getProgram(contract *vm.Contract) (Program, error) { | ||
address := contract.Address() | ||
if contract.CodeAddr != nil { | ||
address = *contract.CodeAddr | ||
} | ||
return p.deserializeProgram(address) | ||
|
||
return p.deserializeProgram(contract.CodeHash) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be collapsed
system_tests/program_test.go
Outdated
func TestProgramKeccakArb(t *testing.T) { | ||
keccakTest(t, false) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicative of TestProgramArbitratorKeccak
system_tests/program_test.go
Outdated
func TestProgramCompilationReuse(t *testing.T) { | ||
t.Parallel() | ||
testCompilationReuse(t, true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analogue is missing in stylus_test.go
func setupProgramTest(t *testing.T, jit bool) ( | ||
context.Context, *arbnode.Node, *BlockchainTestInfo, *ethclient.Client, bind.TransactOpts, func(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of 1 or 0, we should probs take a list of filenames for generality
system_tests/program_test.go
Outdated
arbOwner, err := precompilesgen.NewArbOwner(types.ArbOwnerAddress, l2client) | ||
Require(t, err) | ||
|
||
ensure(arbOwner.SetInkPrice(&auth, 1e2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was 1e2
intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I've added a few commits that are worth taking a look at that address most of the feedback. I decided not to change programs.go
to minimize merge conflicts with
One important optimization that can lead to massive gas savings pointed out by @rachel-bousfield is reusing compiled code that already exists in the state DB when a user is trying to compile a program they deploy. If they have deployed the same keccak program, for example, at address 0xA than one that already exists at another address, we can short circuit the compilation and reuse the existing compiled code from the other program address.
Here's the difference in speed for keccak
This PR also depends on stylus-geth's PR here: OffchainLabs/stylus-geth#17
This PR changes functions associated with WASM programs and the state DB to deal with codehashes internally instead of purely program addresses. This allows us to reuse compiled programs. Changes are also added to the recording DB to ensure we can reuse code in there as well.
We include a comprehensive test that checks several invariants we expect from these changes. Here's how our test works:
We deploy the same keccak WASM code to two different program addresses
We attempt to call them and expect a failure as they have not been compiled
We then:
Here are some of the details of our changelog:
Thanks to @rachel-bousfield for the fantastic guidance along the way.