Bold fixes #2770
Merged
Bold fixes #2770
+1,137
−323
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 just a commit on a branch to give Lee a picture of where Pepper was when he stopped trying to get the bold-review branch's tests passing again. Essentially, I think there is at least one (but maybe several) off-by-one issues with the current implementation in the bold_state_provider. I would recommend trying to get the `bold_state_provider_test.go` (specifically, `TestChallengeProtocolBOLD_StateProvider`) to pass before moving to the other tests. I think it is attempting to validate more tightly-scoped behavior than the other tests in this package. BTW, I'm not actually 100% confident that the whole system is wired together correctly before calling the state provider. But, I do believe that errors there are less-likely than in the implementation. In my heart, I think Raul had these tests passing at some point in history. We probably just silently broke them and never noticed. Thanks for looking into this.
The big problem is still the end-to-end TestChallengeProtocolBOLD test.
Now that the virtual padding is handled in the BoLD protocol itself and not in the creation of the hash leaves being fed into the history committer, the number of hashes read from the cache doesn't need to equal the number of leaves (including virtual leaves.)
This is the current unify-req-meta branch which fixes a bug in one-step proof calculation.
This *should* be able to wrap an arbitrator machine and do the special handling for the BoLD protocol to make it look like there is one more machine state at the front of processing a machine.
This still doesn't get the test passing, but it's bound to be closer.
The CollectProof and CollectMachineHashes functions were both susceptible to challenges where it was possible that the rival would have committed to more messages than this validator. And, in that case, it would attempt to look up a message number which was greater than the highest messge number it had verified as part of the batch in which the challenge originated.
Bold start step test
Before this change, if there was a block challenge at a height in any block above the batchLimit, then the validator was not correctly creating inclusion proofs because it was attempting to fetch execution results for blocks which didn't really exist. Now, the code detects that situation and simply returns the hash of an arbitrator machine in the FINISHED state (since the Virtual leaf hashes) are all in that state by virtue of their being repeated copies of the end state of a block.
cla-bot
bot
added
the
s
Previously, they couldn't produce correct inclusion proofs for block-level challenges on blocks in the virtual range. That is to say, if this validaotor processed n L2Blocks when creating an assertion and a rival processed >= n+1 L2Blocks, this validator would attempt to lookup a block index for which no real block existed. Now, the code properly catches this case and the last machine state for the last real block is used for all virtual L2Blocks.
This is probably not where we ultimately want to be. Too much boilerplate is escaping from the bold system. Maybe, before introducing a dependency injection framework, I should just introduce something manual that would instatiate all the instances that the challenge manager, watcher, assertion manager, etc. need and then wires them together in the "default" way using the constructors from each package.
rauljordan
reviewed
Nov 19, 2024
rauljordan
reviewed
Nov 19, 2024
The first is to sort the imports in mock_machine_test.go. The second is to rearrange the bold machine to avoid the downcast.
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.
These changes clean up a few problems with the BoLD implementation and add some test coverage for some missing cases and also the fixes for them.
Specifically, there were two important problems that are now fixed:
Both have been fixed and we're growing more confident that this is all working.