Fix bugs of addLocalData() and add tests for edge cases #41
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
dajuguan
commented
Jul 19, 2024
dajuguan
commented
- Add tests for attacking the mid-right and right most branch and fix related bug
qizhou
reviewed
Jul 19, 2024
| @@ -517,7 +517,12 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| value_ = starting.raw(); | |||
| } else if (_ident == LocalPreimageKey.DISPUTED_OUTPUT_ROOT) { | |||
| // Load the disputed proposal's output root | |||
| Claim disputed = getClaim(disputedRoot.raw(), disputedPos, _daItem); | |||
| Claim disputed; | |||
| if (disputedPos.raw() == 1) { | |||
| @@ -1115,7 +1120,7 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| if (ancestorPos_.depth() == SPLIT_DEPTH + N_BITS) { | |||
| ancestorClaim_ = ancestorClaimRoot; | |||
| } else { | |||
| ancestorClaim_ = getClaim(ancestorClaimRoot.raw(), _pos, _daItem); | |||
| ancestorClaim_ = getClaim(ancestorClaimRoot.raw(), Position.wrap(_pos.raw() - 1), _daItem); | |||
There was a problem hiding this comment.
Should _pos be replaced by ancestorPos_?
There was a problem hiding this comment.
Actually, ancestorPos_ should be the right context, but the ancestorPos_ value is not right. Maybe we should rename _pos to ancestorPos_?
| @@ -1156,10 +1161,14 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| // Walk up the DAG to find a claim that commits to the same trace index as `_pos`. It is | |||
| // guaranteed that such a claim exists. | |||
| ClaimData storage ancestor_ = claimData[_start]; | |||
| while (ancestor_.position.raw() != traceAncestorPosValue) { | |||
| ancestor_ = claimData[ancestor_.parentIndex]; | |||
| if (traceAncestorPosValue == 0) { | |||
There was a problem hiding this comment.
If traceAncestorPosValue should the claim be starting one? Or in what situation that traceAncestorPosValue is zero?
There was a problem hiding this comment.
Because we offset it by 1, so it's 0 here. For consistency, I've made changes to the offset logic and let the traceAncestorPosValue be 1. 74242dc#r1683762415
dajuguan
commented
Jul 19, 2024
| @@ -518,8 +518,9 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| } else if (_ident == LocalPreimageKey.DISPUTED_OUTPUT_ROOT) { | |||
| // Load the disputed proposal's output root | |||
| Claim disputed; | |||
| // If the pos is 1, then the rootclaim itself is the output hash. | |||
| if (disputedPos.raw() == 1) { | |||
dajuguan
commented
Jul 19, 2024
| @@ -1154,14 +1155,15 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| : _firstValidRightIndex(_pos.traceAncestorBounded(SPLIT_DEPTH), N_BITS); | |||
|
|
|||
| uint256 offset = ancestorPos_.raw() % (1 << N_BITS); | |||
| if (MAX_ATTACK_BRANCH == offset) { | |||
| // If ancestorPos_.raw() == 1 (rootclaim), we don't offset it | |||
| if (MAX_ATTACK_BRANCH == offset || ancestorPos_.raw() == 1) { | |||
There was a problem hiding this comment.
Let offset be 0 when its rootClaim, so the meaning of traceAncestorPosValue will be consistent.
dajuguan
force-pushed
the
po/develop-find-outputs
branch
from
74242dc
to
4a98049
Compare
qizhou
approved these changes
Jul 20, 2024
| @@ -1149,8 +1155,9 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, ISemver { | |||
| : _firstValidRightIndex(_pos.traceAncestorBounded(SPLIT_DEPTH), N_BITS); | |||
|
|
|||
| uint256 offset = ancestorPos_.raw() % (1 << N_BITS); | |||
| if (MAX_ATTACK_BRANCH == offset) { | |||
| offset = 0; | |||
| // If ancestorPos_.raw() == 1 (rootclaim), we don't offset it | |||
dajuguan
force-pushed
the
po/develop-find-outputs
branch
from
4a98049
to
adc68c0
Compare
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.