fixes for removing verfication failed errors for fact* bugs #550
Conversation
This file contains hidden or 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
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
12
to
52
| transactionGroupSize: number, | ||
| note = '' | ||
| ): number[] { | ||
| if (logFlags.verbose) { | ||
| // if (logFlags.verbose) { | ||
| console.log( | ||
| `getCorrespondingNodes ${note} ${ourIndex} ${startTargetIndex} ${endTargetIndex} ${globalOffset} ${receiverGroupSize} ${sendGroupSize} ${transactionGroupSize}` | ||
| ) | ||
| } | ||
| let wrappedIndex: number | ||
| let targetNumber: number | ||
| let found = false | ||
|
|
||
| let unWrappedEndIndex = -1 | ||
| // handle case where receiver group is split (wraps around) | ||
| if (startTargetIndex > endTargetIndex) { | ||
| unWrappedEndIndex = endTargetIndex | ||
| endTargetIndex = endTargetIndex + transactionGroupSize | ||
| } | ||
| //wrap our index to the send group size | ||
| ourIndex = ourIndex % sendGroupSize | ||
|
|
||
| //find our initial staring index into the receiver group (wrappedIndex) | ||
| for (let i = startTargetIndex; i < endTargetIndex; i++) { | ||
| wrappedIndex = i | ||
| if (i >= transactionGroupSize) { | ||
| wrappedIndex = i - transactionGroupSize | ||
| } | ||
| targetNumber = (i + globalOffset) % receiverGroupSize | ||
| if (targetNumber === ourIndex) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if (!found) { | ||
| //return empty array | ||
| return [] | ||
| } | ||
|
|
||
| // } | ||
|
|
||
| const destinationNodes: number[] = [] | ||
| //this loop is at most O(k) where k is receiverGroupSize / sendGroupSize | ||
| //effectively it is constant time it is required so that a smaller | ||
| //group can send to a larger group | ||
| while (targetNumber < receiverGroupSize) { | ||
| //send all payload to this node | ||
| const destinationNode = wrappedIndex | ||
|
|
||
| destinationNodes.push(destinationNode) | ||
| //console.log(`sender ${ourIndex} send all payload to node ${destinationNode} targetNumber ${targetNumber} `) | ||
|
|
||
| // //in-place verification check | ||
| // let sendingNodeIndex = ourIndex | ||
| // let receivingNodeIndex = destinationNode | ||
| // //extra step here, remove in production | ||
| // verifySender(receivingNodeIndex, sendingNodeIndex) | ||
|
|
||
| //this part is a bit tricky. | ||
| //we are incrementing two indexes that control our loop | ||
| //wrapped index will have various corrections so that it can | ||
| //wrap past the end of a split span, or wrap within the range | ||
| //of the receiver group | ||
| targetNumber += sendGroupSize | ||
| wrappedIndex += sendGroupSize | ||
|
|
||
| //wrap to front of transaction group | ||
| if (wrappedIndex >= transactionGroupSize) { | ||
| wrappedIndex = wrappedIndex - transactionGroupSize | ||
| } | ||
| //wrap to front of receiver group | ||
| if (wrappedIndex >= endTargetIndex) { | ||
| wrappedIndex = wrappedIndex - receiverGroupSize | ||
| const normalizedSenderIndex = ourIndex % sendGroupSize | ||
|
|
||
| // Calculate logical position of first receiver | ||
| let logicalPosition = 0 | ||
| let currentIndex = startTargetIndex | ||
|
|
||
| // Iterate through receiver group | ||
| for (let count = 0; count < receiverGroupSize; count++) { | ||
| // Calculate which sender this logical position maps to | ||
| const mappedSenderIndex = ((logicalPosition + globalOffset) % receiverGroupSize) % sendGroupSize | ||
|
|
||
| if (mappedSenderIndex === normalizedSenderIndex) { | ||
| destinationNodes.push(currentIndex) | ||
| } | ||
| //special case to stay in bounds when we have a split index and | ||
| //the unWrappedEndIndex is smaller than the start index. | ||
| //i.e. startTargetIndex = 45, endTargetIndex = 5 for a 50 node group | ||
| if (unWrappedEndIndex != -1 && wrappedIndex >= unWrappedEndIndex) { | ||
| const howFarPastUnWrapped = wrappedIndex - unWrappedEndIndex | ||
| wrappedIndex = startTargetIndex + howFarPastUnWrapped | ||
|
|
||
| // Move to next receiver | ||
| logicalPosition++ | ||
| currentIndex++ | ||
|
|
||
| // Handle wrap-around using modular arithmetic | ||
| if (currentIndex === transactionGroupSize) { | ||
| currentIndex = 0 | ||
| } | ||
| } | ||
| if (logFlags.verbose) { | ||
|
|
||
| // if (logFlags.verbose) { | ||
| console.log(`note: ${note} destinationNodes ${destinationNodes}`) | ||
| } | ||
| // } | ||
| return destinationNodes | ||
| } |
There was a problem hiding this comment.
Suggestion: The new implementation of getCorrespondingNodes does not correctly handle cases where the receiver group wraps around the end of the transaction group (i.e., when startTargetIndex > endTargetIndex). This can cause incorrect mapping of sender indices to receiver nodes, leading to missed or extra assignments. Restore logic to handle wrapped receiver groups by adjusting endTargetIndex and iterating over the correct range. [possible issue, importance: 8]
New proposed code:
export function getCorrespondingNodes(
ourIndex: number,
startTargetIndex: number,
endTargetIndex: number,
globalOffset: number,
receiverGroupSize: number,
sendGroupSize: number,
transactionGroupSize: number,
note = ''
): number[] {
...
const normalizedSenderIndex = ourIndex % sendGroupSize
-...
- // Iterate through receiver group
- for (let count = 0; count < receiverGroupSize; count++) {
- // Calculate which sender this logical position maps to
+
+ let adjustedEndTargetIndex = endTargetIndex
+ let unWrappedEndIndex = -1
+ if (startTargetIndex > endTargetIndex) {
+ unWrappedEndIndex = endTargetIndex
+ adjustedEndTargetIndex = endTargetIndex + transactionGroupSize
+ }
+
+ const destinationNodes: number[] = []
+ let logicalPosition = 0
+ let currentIndex = startTargetIndex
+
+ for (let i = startTargetIndex; i < adjustedEndTargetIndex; i++) {
+ let wrappedIndex = i
+ if (i >= transactionGroupSize) {
+ wrappedIndex = i - transactionGroupSize
+ }
const mappedSenderIndex = ((logicalPosition + globalOffset) % receiverGroupSize) % sendGroupSize
-
if (mappedSenderIndex === normalizedSenderIndex) {
- destinationNodes.push(currentIndex)
+ destinationNodes.push(wrappedIndex)
}
-
- // Move to next receiver
logicalPosition++
- currentIndex++
-
- // Handle wrap-around using modular arithmetic
- if (currentIndex === transactionGroupSize) {
- currentIndex = 0
- }
}
...
+ return destinationNodes
}
Comment on lines
62
to
123
| shouldUnwrapSender = false, | ||
| note = '' | ||
| ): boolean { | ||
| if (logFlags.verbose) { | ||
| // if (logFlags.verbose) { | ||
| console.log( | ||
| `verifyCorrespondingSender ${note} ${receivingNodeIndex} ${sendingNodeIndex} ${globalOffset} ${receiverGroupSize} ${sendGroupSize} ${receiverStartIndex} ${receiverEndIndex} ${transactionGroupSize}` | ||
| ) | ||
| } | ||
| //note, in the gather case, we need to check the address range of the sender node also, to prove | ||
| //that it does cover the given account range | ||
| let unwrappedReceivingNodeIndex = receivingNodeIndex | ||
|
|
||
| // handle case where receiver group is split (wraps around) | ||
| if (receiverStartIndex > unwrappedReceivingNodeIndex) { | ||
| unwrappedReceivingNodeIndex = unwrappedReceivingNodeIndex + transactionGroupSize | ||
| } | ||
| let unwrappedSendingNodeIndex = sendingNodeIndex | ||
| if (shouldUnwrapSender) { | ||
| unwrappedSendingNodeIndex = sendingNodeIndex + transactionGroupSize | ||
| } | ||
|
|
||
| // use unwrappedReceivingNodeIndex to calculate the target index | ||
| const targetIndex = ((unwrappedReceivingNodeIndex + globalOffset) % receiverGroupSize) % sendGroupSize | ||
| const targetIndex2 = unwrappedSendingNodeIndex % sendGroupSize | ||
| if (targetIndex === targetIndex2) { | ||
| if (logFlags.verbose) | ||
| // } | ||
|
|
||
| // Calculate logical position of receiver in its group | ||
| let logicalPosition: number | ||
|
|
||
| if (receiverStartIndex <= receiverEndIndex) { | ||
| // Non-wrapped case | ||
| if (receivingNodeIndex >= receiverStartIndex && receivingNodeIndex < receiverEndIndex) { | ||
| logicalPosition = receivingNodeIndex - receiverStartIndex | ||
| } else { | ||
| // Receiver not in group | ||
| console.log( | ||
| `note: ${note} verification passed ${targetIndex} === ${targetIndex2} ${unwrappedSendingNodeIndex}->${receivingNodeIndex}` | ||
| `note: ${note} receiver not in group ${receivingNodeIndex} ${receiverStartIndex} ${receiverEndIndex}` | ||
| ) | ||
| return true | ||
| return false | ||
| } | ||
| } else { | ||
| console.log( | ||
| `note: ${note} X verification failed ${targetIndex} !== ${targetIndex2} sender: ${unwrappedSendingNodeIndex} receiver: ${receivingNodeIndex}` | ||
| ) | ||
| return false | ||
| // Wrapped case | ||
| if (receivingNodeIndex >= receiverStartIndex) { | ||
| logicalPosition = receivingNodeIndex - receiverStartIndex | ||
| } else if (receivingNodeIndex < receiverEndIndex) { | ||
| logicalPosition = (transactionGroupSize - receiverStartIndex) + receivingNodeIndex | ||
| } else { | ||
| // Receiver not in group | ||
| console.log( | ||
| `note: ${note} receiver not in group ${receivingNodeIndex} ${receiverStartIndex} ${receiverEndIndex}` | ||
| ) | ||
| return false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Calculate expected sender using pure math | ||
| const expectedSenderIndex = ((logicalPosition + globalOffset) % receiverGroupSize) % sendGroupSize | ||
| const actualSenderIndex = sendingNodeIndex % sendGroupSize | ||
|
|
||
| const result = expectedSenderIndex === actualSenderIndex | ||
|
|
||
| // if (logFlags.verbose) { | ||
| if (result) { | ||
| console.log( | ||
| `note: ${note} verification passed ${expectedSenderIndex} === ${actualSenderIndex} ${sendingNodeIndex}->${receivingNodeIndex}` | ||
| ) | ||
| } else { | ||
| console.log( | ||
| `note: ${note} X verification failed ${expectedSenderIndex} !== ${actualSenderIndex} sender: ${sendingNodeIndex} receiver: ${receivingNodeIndex}` | ||
| ) | ||
| } | ||
| // } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Suggestion: The calculation of logicalPosition in the wrapped case is incorrect when the receiver group wraps around the end of the transaction group. This can result in wrong sender verification. Adjust the calculation to correctly compute the logical position for wrapped receiver groups by adding transactionGroupSize to the receiver index before subtracting receiverStartIndex. [possible issue, importance: 7]
New proposed code:
export function verifyCorrespondingSender(
receivingNodeIndex: number,
sendingNodeIndex: number,
globalOffset: number,
receiverGroupSize: number,
sendGroupSize: number,
receiverStartIndex = 0,
receiverEndIndex = 0,
transactionGroupSize = 0,
shouldUnwrapSender = false,
note = ''
): boolean {
...
let logicalPosition: number
if (receiverStartIndex <= receiverEndIndex) {
// Non-wrapped case
if (receivingNodeIndex >= receiverStartIndex && receivingNodeIndex < receiverEndIndex) {
logicalPosition = receivingNodeIndex - receiverStartIndex
} else {
// Receiver not in group
console.log(
`note: ${note} receiver not in group ${receivingNodeIndex} ${receiverStartIndex} ${receiverEndIndex}`
)
return false
}
} else {
// Wrapped case
if (receivingNodeIndex >= receiverStartIndex) {
logicalPosition = receivingNodeIndex - receiverStartIndex
} else if (receivingNodeIndex < receiverEndIndex) {
- logicalPosition = (transactionGroupSize - receiverStartIndex) + receivingNodeIndex
+ logicalPosition = (receivingNodeIndex + transactionGroupSize) - receiverStartIndex
} else {
// Receiver not in group
console.log(
`note: ${note} receiver not in group ${receivingNodeIndex} ${receiverStartIndex} ${receiverEndIndex}`
)
return false
}
}
...
devendra-shardeum
force-pushed
the
hotfix-rc-fact-fix
branch
from
0631c4f to
92ee998
Compare
devendra-shardeum
changed the title
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.
PR Type
Bug fix, Enhancement
Description
Refactored and fixed
getCorrespondingNodesandverifyCorrespondingSenderlogic.Improved logging for FACT* algorithm debugging and validation.
Updated function calls to pass new log/debug parameters.
Enhanced error reporting and debug output for sender/receiver validation.
Changes walkthrough 📝
fastAggregatedCorrespondingTell.ts
Refactor and fix corresponding node/sender logic with better loggingsrc/utils/fastAggregatedCorrespondingTell.ts
getCorrespondingNodesfor clearer, more robust logic.verifyCorrespondingSenderto improve correctness andclarity.
TransactionQueue.ts
Update FACT sender/correspondence validation and loggingsrc/state-manager/TransactionQueue.ts
getCorrespondingNodesandverifyCorrespondingSenderwith new debug/log parameters.
CachedAppDataManager.ts
Update FACT correspondence calls with enhanced loggingsrc/state-manager/CachedAppDataManager.ts
getCorrespondingNodesandverifyCorrespondingSendercalls toinclude new debug/log parameters.