Define Execution Run Method to Compute Machine Hashes With Step Size for BOLD #2392
Conversation
cla-bot
bot
added
the
s
|
Ready again @tsahee thanks for the great review! |
…s/nitro into get-machine-hashes-with-step
system_tests/validation_mock_test.go
Outdated
| @@ -126,6 +126,10 @@ func (r *mockExecRun) GetStepAt(position uint64) containers.PromiseInterface[*va | |||
| }, nil) | |||
| } | |||
|
|
|||
| func (r *mockExecRun) GetMachineHashesWithStepSize(machineStartIndex, stepSize, numRequiredHashes uint64) containers.PromiseInterface[[]common.Hash] { | |||
There was a problem hiding this comment.
Is numRequiredHases essentially numIterations?
That is, you're telling the machine to start at position x, and then perform y iterations of s opcodes per iteration.
The fact that this also produces y hashcodes (one for each iteration) is more related to the return value than the arguments. Right?
The reason I'm making a big deal of the naming, is that I actually was confused when I first read the signature. numRequiredHashes sounds like it might be independent from how far through the machine execution progresses. But, I don't think it actually is. Without a doc comment, or something explicit, it isn't clear that these "required hashes" are the machine hash after each iteration of size stepSize.
Also, "required" sort of makes me think that it's possible that the function will error out because it wasn't able to provide enough hashes, or that it should fill in the returned slice with some "filler" hash if the machine finishes before enough hashes are generated.
There was a problem hiding this comment.
Great feedback! Renamed to num iterations
| return machineHashes, nil | ||
| } | ||
|
|
||
| logInterval := requiredNumHashes / 20 // Log every 5% progress |
There was a problem hiding this comment.
Optional: Maybe this should be configurable? It seems like something we might want to be more noisy in the beginning, but then turn way down low later?
| e := &executionRun{} | ||
| ctx := context.Background() | ||
|
|
||
| t.Run("basic argument checks", func(t *testing.T) { |
There was a problem hiding this comment.
Why are all these different test cases being run using t.Run instead of just being separate top-level test functions?
There was a problem hiding this comment.
Personal preference here. I find it more expressive to write subcases in plain english instead of having to wrangle the Go function naming convention. It also groups functionality into one place, but I'm happy to change it if the alternative is highly preferred
There was a problem hiding this comment.
I looked in the CI and saw that it does display these runs separately and nicely.
The advantage of separate functions is that you can run one by itself, and out CI will re-execute every failed test once to see if it first failed due to system flakiness.
I think for short low-footprint tests like these using t.Run is fine, and for anything in system_test I would still prefer separate functions.
| } | ||
| } | ||
| }) | ||
| t.Run("if finishes execution early, simply pads the remaining desired hashes with the machine finished hash", func(t *testing.T) { |
There was a problem hiding this comment.
Well, this answers my earlier question. Why is this the protocol? Why not just return fewer hashes if the machine finishes? Seems like it could reduce the size of the data passed back to the callers?
There was a problem hiding this comment.
Agreed...will let the caller pad if they need to
|
Ready again @eljobe , thanks for the review |
|
Hi @eljobe thanks again! Needs reapproval after the last naming change |
| if err := machine.Step(ctx, stepSize); err != nil { | ||
| return nil, fmt.Errorf("failed to step machine to position %d: %w", absoluteMachineIndex, err) | ||
| } | ||
| if i%logInterval == 0 || i == maxIterations-1 { |
There was a problem hiding this comment.
not a must..
I found that logging every x seconds (remembering when you last logged and logging again if enough time passed) is better than logging every X iterations
| e := &executionRun{} | ||
| ctx := context.Background() | ||
|
|
||
| t.Run("basic argument checks", func(t *testing.T) { |
There was a problem hiding this comment.
I looked in the CI and saw that it does display these runs separately and nicely.
The advantage of separate functions is that you can run one by itself, and out CI will re-execute every failed test once to see if it first failed due to system flakiness.
I think for short low-footprint tests like these using t.Run is fine, and for anything in system_test I would still prefer separate functions.
|
Ready again @tsahee , thank you ! |
Background
As part of Arbitrum BOLD, validators will need to ask a stateless execution server for a list of hashes of executing an Arbitrator machine with some configuration. Validators can specify the start index at which to load the machine, how many times to step over it, and the step size in its execution. For instance:
"I'm in a BOLD challenge, and I need to get the hashes of executing the machine for message number 70, starting at program counter 10, and stepping through it in steps of size 1M. Give me 1024 hashes from that computation"
This PR keeps the implementation simple and adds unit tests to check the invariants of the function. This function is unused until BOLD is merged into Nitro