Define Execution Run Method to Compute Machine Hashes With Step Size for BOLD #2392
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| package server_arb | ||
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/offchainlabs/nitro/validator" | ||
| ) | ||
|
|
||
| type mockMachine struct { | ||
| gs validator.GoGlobalState | ||
| totalSteps uint64 | ||
| } | ||
|
|
||
| func (m *mockMachine) Hash() common.Hash { | ||
| if m.gs.PosInBatch == m.totalSteps-1 { | ||
| return machineFinishedHash(m.gs) | ||
| } | ||
| return m.gs.Hash() | ||
| } | ||
|
|
||
| func (m *mockMachine) GetGlobalState() validator.GoGlobalState { | ||
| return m.gs | ||
| } | ||
|
|
||
| func (m *mockMachine) Step(ctx context.Context, stepSize uint64) error { | ||
| for i := uint64(0); i < stepSize; i++ { | ||
| if m.gs.PosInBatch == m.totalSteps-1 { | ||
| return nil | ||
| } | ||
| m.gs.PosInBatch += 1 | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (m *mockMachine) CloneMachineInterface() MachineInterface { | ||
| return &mockMachine{ | ||
| gs: validator.GoGlobalState{Batch: m.gs.Batch, PosInBatch: m.gs.PosInBatch}, | ||
| totalSteps: m.totalSteps, | ||
| } | ||
| } | ||
| func (m *mockMachine) GetStepCount() uint64 { | ||
| return 0 | ||
| } | ||
| func (m *mockMachine) IsRunning() bool { | ||
| return m.gs.PosInBatch < m.totalSteps-1 | ||
| } | ||
| func (m *mockMachine) ValidForStep(uint64) bool { | ||
| return true | ||
| } | ||
| func (m *mockMachine) Status() uint8 { | ||
| if m.gs.PosInBatch == m.totalSteps-1 { | ||
| return uint8(validator.MachineStatusFinished) | ||
| } | ||
| return uint8(validator.MachineStatusRunning) | ||
| } | ||
| func (m *mockMachine) ProveNextStep() []byte { | ||
| return nil | ||
| } | ||
| func (m *mockMachine) Freeze() {} | ||
| func (m *mockMachine) Destroy() {} | ||
|
|
||
| func Test_machineHashesWithStep(t *testing.T) { | ||
| mm := &mockMachine{} | ||
| e := &executionRun{} | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ctx := context.Background() | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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. |
||
| machStartIndex := uint64(0) | ||
| stepSize := uint64(0) | ||
| numRequiredHashes := uint64(0) | ||
| _, err := e.machineHashesWithStepSize(ctx, machStartIndex, stepSize, numRequiredHashes) | ||
| if !strings.Contains(err.Error(), "step size cannot be 0") { | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| t.Fatal("Wrong error") | ||
| } | ||
| stepSize = uint64(1) | ||
| _, err = e.machineHashesWithStepSize(ctx, machStartIndex, stepSize, numRequiredHashes) | ||
| if !strings.Contains(err.Error(), "required number of hashes cannot be 0") { | ||
| t.Fatal("Wrong error") | ||
| } | ||
| }) | ||
| t.Run("machine at start index 0 hash is the finished state hash", func(t *testing.T) { | ||
| mm.gs = validator.GoGlobalState{ | ||
| Batch: 1, | ||
| } | ||
| machStartIndex := uint64(0) | ||
| stepSize := uint64(1) | ||
| numRequiredHashes := uint64(1) | ||
| e.cache = &MachineCache{ | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| buildingLock: make(chan struct{}, 1), | ||
| machines: []MachineInterface{mm}, | ||
| finalMachine: mm, | ||
| } | ||
| go func() { | ||
| <-time.After(time.Millisecond * 50) | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| e.cache.buildingLock <- struct{}{} | ||
| }() | ||
| hashes, err := e.machineHashesWithStepSize(ctx, machStartIndex, stepSize, numRequiredHashes) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| expected := machineFinishedHash(mm.gs) | ||
| if len(hashes) != 1 { | ||
| t.Fatal("Wanted one hash") | ||
| } | ||
| if expected != hashes[0] { | ||
| t.Fatalf("Wanted %#x, got %#x", expected, hashes[0]) | ||
rauljordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }) | ||
| t.Run("can step in step size increments and collect hashes", func(t *testing.T) { | ||
| initialGs := validator.GoGlobalState{ | ||
| Batch: 1, | ||
| PosInBatch: 0, | ||
| } | ||
| mm.gs = initialGs | ||
| mm.totalSteps = 20 | ||
| machStartIndex := uint64(0) | ||
| stepSize := uint64(5) | ||
| numRequiredHashes := uint64(4) | ||
| e.cache = &MachineCache{ | ||
| buildingLock: make(chan struct{}, 1), | ||
| machines: []MachineInterface{mm}, | ||
| finalMachine: mm, | ||
| } | ||
| go func() { | ||
| <-time.After(time.Millisecond * 50) | ||
| e.cache.buildingLock <- struct{}{} | ||
| }() | ||
| hashes, err := e.machineHashesWithStepSize(ctx, machStartIndex, stepSize, numRequiredHashes) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| expectedHashes := make([]common.Hash, 0) | ||
| for i := uint64(0); i < 4; i++ { | ||
| if i == 0 { | ||
| expectedHashes = append(expectedHashes, machineFinishedHash(initialGs)) | ||
| continue | ||
| } | ||
| gs := validator.GoGlobalState{ | ||
| Batch: 1, | ||
| PosInBatch: uint64(i * stepSize), | ||
| } | ||
| expectedHashes = append(expectedHashes, gs.Hash()) | ||
| } | ||
| if len(hashes) != len(expectedHashes) { | ||
| t.Fatal("Wanted one hash") | ||
| } | ||
| for i := range hashes { | ||
| if expectedHashes[i] != hashes[i] { | ||
| t.Fatalf("Wanted at index %d, %#x, got %#x", i, expectedHashes[i], hashes[i]) | ||
| } | ||
| } | ||
| }) | ||
| 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 |
||
| initialGs := validator.GoGlobalState{ | ||
| Batch: 1, | ||
| PosInBatch: 0, | ||
| } | ||
| mm.gs = initialGs | ||
| mm.totalSteps = 20 | ||
| machStartIndex := uint64(0) | ||
| stepSize := uint64(5) | ||
| numRequiredHashes := uint64(10) | ||
| e.cache = &MachineCache{ | ||
| buildingLock: make(chan struct{}, 1), | ||
| machines: []MachineInterface{mm}, | ||
| finalMachine: mm, | ||
| } | ||
| go func() { | ||
| <-time.After(time.Millisecond * 50) | ||
| e.cache.buildingLock <- struct{}{} | ||
| }() | ||
| hashes, err := e.machineHashesWithStepSize(ctx, machStartIndex, stepSize, numRequiredHashes) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| expectedHashes := make([]common.Hash, 0) | ||
| for i := uint64(0); i < 4; i++ { | ||
| if i == 0 { | ||
| expectedHashes = append(expectedHashes, machineFinishedHash(initialGs)) | ||
| continue | ||
| } | ||
| gs := validator.GoGlobalState{ | ||
| Batch: 1, | ||
| PosInBatch: uint64(i * stepSize), | ||
| } | ||
| expectedHashes = append(expectedHashes, gs.Hash()) | ||
| } | ||
| // The rest of the expected hashes should be the machine finished hash repeated. | ||
| for len(expectedHashes) < 10 { | ||
| expectedHashes = append(expectedHashes, machineFinishedHash(validator.GoGlobalState{ | ||
| Batch: 1, | ||
| PosInBatch: mm.totalSteps - 1, | ||
| })) | ||
| } | ||
| if len(hashes) != len(expectedHashes) { | ||
| t.Fatal("Wanted one hash") | ||
| } | ||
| for i := range hashes { | ||
| if expectedHashes[i] != hashes[i] { | ||
| t.Fatalf("Wanted at index %d, %#x, got %#x", i, expectedHashes[i], hashes[i]) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is
numRequiredHasesessentiallynumIterations?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.
numRequiredHashessounds 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feedback! Renamed to num iterations