Unify HistoryCommitmentRequest assertion metadata #697
Conversation
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 is not a 100% endorsement of these tests correctly testing the correct implementation. It is more a snapshot at which they are once again exercising the current implementation on the branch.
eljobe
force-pushed
the
unify-req-meta
branch
from
0721244 to
0b91f28
Compare
The problem with using the claimed assertion's `AfterState.Batch` is that, in the case of assertions which overflow the maximum number of blocks for an assertion, the `AfterState.Batch` will be lower than the actual upper limit on batches. And, if a history commitment is calculated based on that lower limit, it might not include all of the blocks to which the assertion should commit.
PlasmaPower
commented
Oct 25, 2024
PlasmaPower
commented
Oct 25, 2024
eljobe
reviewed
Oct 28, 2024
The problem with removing it is that the history commitments don't always commit to leaves starting from the first leaf, `FromHeight=0`. When there is a bisection, of a challenge edge, the history commitment of one edge should be `FromHeight=0` `UpToHeight=(parentHeight/2)+1` and the second edge should be `FromHeight=(parentHeight/2)+1` and `UpToHeight=parentHeight`.
This change also updates the documentation to explain why the FromHeight may never have a value other than 0, and it simplifies the code around calculating how many hashes are needed, and at which opcode the machine hash computation should start.
This will keep us from colliding if there are history commitments requeseted starting from different messages in the sequencer inbox in the same batch. This most commonly happens when handling overflow assertions.
This commit makes two minor improvements to logging: 1. When the tracker is unable to submit a one step proof, it now logs at the Error level instead of Trace. 2. When the watcher logs events, it also includes the validator name.
Without this, the tracker would post twice the number of one-step proofs as needed for correct behavior. That would be wasteful of gas.
It turns out that most of the information in the AssociatedAssertionMetadata is needed in various places in the correct implementation of the state provider. So, go ahead and pass it through.
Before this change, there were some flaky tests in the assertion manager package. In part, this was because it was difficult to reason about the responsibilities of each component. Some of the work that the assertion manager was doing by making calls to its logical parent (the challenge maanger) has been moved up to the challenge manager with a very narrow interface remaning between the two. Also, the construction of the challenge manager and its dependencies has been slightly decoupled. Now the challenge manager takes in an almosst completetely constructed assertion manager and registers itself as the RivalHandler for the assertion manager. It also sets the correct address for the assertion manager which is still being fetched from an on-chain contract during the construction of the challenge manager. But, this can also be pulled out in a future commit and passed into both objects (as well as the watcher and tracker objects.) Another small code-quality improvement was pulling out the configuration that affects the timing of various things the assertion manager is responsible for into a separate private `times` struct helps to document the behavior of those timers and also makes it easier to pass consistent values when testing. Next up, I'd really like to pull some of the other types which are currently being constructed during the construction of a challenge manager out and improve on our ability to share the wiring across objects.
These were all good catches. Remmber to check your errors boys and girls.
This package is where clients should go to create a challenge manager and the stack of dependencies that need to work together to implement the BoLD protocol.
The call doesn't need a context and cannot return an error. It's a simple lookup of a field on the AssertionChain. This simplifies a lot of things.
The assertion manager already has an AssertionChain, and it is trivial to get the challenge manager's address from that abstraction.
This slightly simplifies the construction of a chllenge manager.
This further simplifies the constructor for the challenge manager and makes it easier to test independently.
This one was pretty gnarly, but I'm thinking it's improved the readabiliy, testibility, and genearl separation of concerns.
This more clearly differentiates it from the challenge manager's constructor.
There was an annoying race that was occurring when both alice and charlie tried to post the same rival assertion to bob's evil assertion.
Each of the fields removed from the struct really do have a more natural home in other parts of the system.
I almost wonder if we should invert these two in the object graph and essentially say that what nitro is doing is adding a BoLD API server that just happens to manage all the challenge tracking as well.
This essentially removes the last remaining issue that was keeping things from being easy to wire together, and removes the comments.
rauljordan
reviewed
Nov 19, 2024
|
I reviewed every file and the only thing the stood out to me was the movement of the condition for transition to the one step proving state in the FSM. I found more context about the change elsewhere and I feel comfortable with it, and I would love to see all these changes live in bold testnet v2 this week. Superb job with so much attention to detail in polishing up the codebase |
Specifically: 1. Move some methods to the Mode type 2. Rename assertionMetadataCache 3. Fix two typos in comments 4. Update LRU caches to 1500 ~15 days for Arbitrum One
These values live on-chain, and should be read and cached during the intialization of the chain abstraction to make them trival to retrieve in the places where they are needed. This commit also changes the type of the members of the LayerZeroHeights to a protocol.Height and transitions to returning a LayerZeroHeights value instead of a pointer to keep the shared pointer from being available to code that might accidentally mutate it.
The `ExecutionStateAfterPreviousState` method doesn't need to take in the `maxNumberOfBlocks` because it can always find the same value in `chain.SpecChallengeManager().LayerZeroHeights().BlockChallengeHeight` The assumption is that all implementations of an `protocol.ExecutionProvider` will also be constucted with a `protocol.AssertionChain`
We can add it back if we decide that multiple validators colliding in a congested parent chain is an actual problem. But, for now, we think the delay is not really solving the problem, and just adding time to getting challenges opened.
There were too many places where it was cumbersome to keep converting to and from this data type and the `common.Hash` that it wraps. By moving the conversion down a layer in abstraction, many lines of code were made simpler.
The main problem was that several tests were the wrong size. But, there were also some tests which were still pointing at direct dependencies they no longer needed, and the //logs/ephemeral:ephemeral_test target hadn't even been generated.
This commit was created by: 1. Copying over nitro's .golangci.yml 2. Running golangci-lint run --fix 3. Addressing the remaining issues 4. For the G115 issues, use the safecast library
rauljordan
reviewed
Nov 20, 2024
rauljordan
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Unifies a few fields about the Assertion being challenged into a struct that can be used in place of individually passing around the fields.
This PR also cleans up some tests and restricts the expressiveness of HistoryCommitmentRequests to always begin at the first index (0) relative to the challenge level for which the commitment is being requested. The edge tracker component (which is the main place from which history commitments are requested never needs a history commitment that doesn't start at the first machine state hash relative to challenge level.) This also simplifies a couple of utility methods related to calculating machine hash ranges.
This PR also now includes a big refactoring moving us closer to dependency injection, and fixes the flakiness in one of the assertion tests.
Fixes https://linear.app/offchain-labs/issue/NIT-2947/clean-up-flaky-tests-in-the-boldassertions-package