BLUE-130 Global txreceipt verification #80

jairajdev · 2024-09-11T17:57:21Z

No description provided.

github-actions · 2024-09-11T17:58:26Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review Code Complexity The method `validateArchiverReceipt` has significantly increased in complexity with multiple nested conditions and loops, making it harder to maintain and understand. Consider refactoring to reduce complexity and improve readability. Error Handling The error handling in `validateArchiverReceipt` logs errors but does not propagate them or handle them in a way that allows for recovery or alternative flows. This could lead to unhandled states in production. Possible Bug The condition `if (isValid)` in the method `verifyReceiptData` seems incorrect. It logs an error if `isValid` is true, which is counterintuitive. This might be a logic error. Code Duplication The method `verifyGlobalTxAccountChange` contains duplicated logic for checking account hashes before and after a transaction. This could be abstracted into a separate method to avoid duplication and potential errors in updates.

src/Data/Collector.ts

arhamj · 2024-10-24T15:43:21Z

src/Data/Collector.ts

+      const { signs } = appliedReceipt
+      // Refer to https://github.com/shardeum/shardus-core/blob/7d8877b7e1a5b18140f898a64b932182d8a35298/src/p2p/GlobalAccounts.ts#L397
+      let votingGroupCount = cycleShardData.shardGlobals.nodesPerConsenusGroup
+      if (votingGroupCount > cycleShardData.nodes.length) {


Should we have a lower bound to the acceptable voting group count?

arhamj · 2024-10-24T15:43:52Z

src/Data/Collector.ts

+      if (votingGroupCount > cycleShardData.nodes.length) {
+        votingGroupCount = cycleShardData.nodes.length
+      }
+      let isReceiptMajority = (signs.length / votingGroupCount) * 100 >= 60


Can we make the percentage for majority configurable?

config requiredMajorityVotesPercentage introduced for the same

arhamj · 2024-10-24T15:49:50Z

src/Data/Collector.ts

+      for (const sign of signs) {
+        const { owner: nodePubKey } = sign
+        // Get the node id from the public key
+        const node = cycleShardData.nodes.find((node) => node.publicKey === nodePubKey)


Is there a way to avoid this find operation on every iteration? This loop feels suboptimal.

using Map now for reduced complexity

arhamj · 2024-10-24T15:50:32Z

src/Data/Collector.ts

+        }
+        uniqueSigners.add(nodePubKey)
+      }
+      isReceiptMajority = (uniqueSigners.size / votingGroupCount) * 100 >= 60


Move to majority number to config here too.

arhamj · 2024-10-24T15:52:51Z

src/Data/Collector.ts

+    const { signs, tx } = appliedReceipt
+    // Using a map to store the good signatures to avoid duplicates
+    const goodSignatures = new Map()
+    for (const sign of signs) {


Do we not need to exclude signatures from nodes not part of the consensus group here like on the previous file? Or are we filtering the signatures earlier and they are not reaching here if they are from invalid nodes?

100 - consensus group size
60 - required

Not part of the node list: 10
Not part of the partition group: 5
85>60

sign verify
85>60

The signatures were not getting filtered earlier, only the required no of signatures was being calculated based on the valid node count. Have introduced the filtering in this section now.

arhamj · 2024-10-24T15:53:55Z

src/shardeum/verifyGlobalTxReceipt.ts

+  Penalty = 12,
+}
+
+export const verifyGlobalTxAccountChange = (


Can we write unit tests for this function?

… debug logs for account query Adding global-txreceipt-verification Added global tx receipt validation and verification Added account verification for global tx receipt Updated the new poqo receipt change

fix receipt verification

src/Data/Collector.ts

+        (signs.length / votingGroupCount) * 100 >= config.requiredMajorityVotesPercentage
+      if (!isReceiptMajority) {
+        Logger.mainLogger.error(
+          `Invalid receipt globalModification signs count is less than ${config.requiredMajorityVotesPercentage}% of the votingGroupCount, ${signs.length}, ${votingGroupCount}`


To fix the log injection issue, we need to sanitize the user input before logging it. Specifically, we should remove any newline characters from the config.requiredMajorityVotesPercentage value before including it in the log message. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

arhamj · 2024-10-25T11:27:15Z

src/Data/Collector.ts

+        return result
+      }
+
+      const nodeMap = new Map<string, P2PTypes.NodeListTypes.Node>()


Since this map is only going to change once per cycle we can move this up the level for further optimisation. But for now LGTM.

* Added composite indexes ( cycle and timesamp ) on each data and added debug logs for account query Adding global-txreceipt-verification Added global tx receipt validation and verification Added account verification for global tx receipt Updated the new poqo receipt change * fix empty cycleInfo fix receipt verification * enhance node validation * shardus/types version update- 1.2.21 * fix(lint): fixed list error on votingGroupCount --------- Co-authored-by: Sonali Thakur <[email protected]> Co-authored-by: Arham Jain <[email protected]>

github-actions bot added the Review effort [1-5]: 4 label Sep 11, 2024

github-advanced-security bot found potential problems Sep 11, 2024

View reviewed changes

src/Data/Collector.ts Dismissed Show dismissed Hide dismissed

S0naliThakur force-pushed the global-txreceipt-verification branch 5 times, most recently from 93334f1 to 170baf0 Compare October 18, 2024 17:22

mhanson-github force-pushed the global-txreceipt-verification branch from fea0f4e to b4d2042 Compare October 24, 2024 01:59

arhamj reviewed Oct 25, 2024

View reviewed changes

jairajdev and others added 2 commits October 25, 2024 13:16

Added composite indexes ( cycle and timesamp ) on each data and added…

6d566ec

… debug logs for account query Adding global-txreceipt-verification Added global tx receipt validation and verification Added account verification for global tx receipt Updated the new poqo receipt change

fix empty cycleInfo

5e1c1d7

fix receipt verification

S0naliThakur force-pushed the global-txreceipt-verification branch from b4d2042 to 5e1c1d7 Compare October 25, 2024 07:46

enhance node validation

ccaff34

github-advanced-security bot found potential problems Oct 25, 2024

View reviewed changes

arhamj reviewed Oct 25, 2024

View reviewed changes

arhamj previously approved these changes Oct 25, 2024

View reviewed changes

shardus/types version update- 1.2.21

a14ffae

S0naliThakur dismissed arhamj’s stale review via a14ffae October 25, 2024 12:06

arhamj previously approved these changes Oct 25, 2024

View reviewed changes

fix(lint): fixed list error on votingGroupCount

2208b6e

arhamj dismissed their stale review via 2208b6e October 25, 2024 12:14

arhamj approved these changes Oct 25, 2024

View reviewed changes

arhamj merged commit 8145e6f into dev Oct 25, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLUE-130 Global txreceipt verification #80

BLUE-130 Global txreceipt verification #80

jairajdev commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

arhamj Oct 24, 2024

S0naliThakur Oct 25, 2024

arhamj Oct 24, 2024

S0naliThakur Oct 25, 2024

arhamj Oct 24, 2024

S0naliThakur Oct 25, 2024

arhamj Oct 24, 2024

S0naliThakur Oct 25, 2024

arhamj Oct 24, 2024

arhamj Oct 25, 2024

S0naliThakur Oct 25, 2024

arhamj Oct 24, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

arhamj Oct 25, 2024

BLUE-130 Global txreceipt verification #80

BLUE-130 Global txreceipt verification #80

Conversation

jairajdev commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

PR Reviewer Guide 🔍

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment