Skip to content

Conversation

aniketdivekar
Copy link
Contributor

@aniketdivekar aniketdivekar commented Aug 7, 2025

PR Type

Enhancement, Tests


Description

  • Add support for storing certificates in cycles table

  • Update database schema and logic for certificates field

  • Refactor validation to handle certificates correctly

  • Add and extend unit tests for certificates handling

image

Changes walkthrough 📝

Relevant files
Enhancement
6 files
Collector.ts
Store certificates in cycle data records                                 
+1/-0     
Cycles.ts
Refactor validation to ignore certificates field                 
+8/-4     
Data.ts
Preserve certificates during cycle data collection             
+4/-1     
cycles.ts
Add certificates column to cycles insert/update logic       
+4/-3     
index.ts
Add certificates column to cycles table schema                     
+2/-0     
types.ts
Update Cycle and DbCycle types to include certificates     
+2/-0     
Tests
6 files
Collector.test.ts
Add tests for storing cycles with/without certificates     
+53/-0   
Cycles.test.ts
Add and extend tests for cycle validation with certificates
+48/-8   
cycles.test.ts
Add tests for inserting cycles with/without certificates 
+54/-0   
index.test.ts
Update test for cycles table schema change                             
+1/-1     
types.test.ts
Update mock data to use certificates array                             
+9/-7     
index.test.ts
Update mock cycle data to use certificates array                 
+9/-7     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    github-actions bot commented Aug 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🏅 Score: 92
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Migration Safety

    The PR adds a new certificates column to the cycles table using an ALTER TABLE statement. Reviewers should ensure that this migration is safe for all supported environments, especially for existing deployments with large tables, and that the application handles cases where the column may not exist yet (e.g., during rolling upgrades).

      await runCreate(cycleDatabase, 'ALTER TABLE `cycles` ADD COLUMN `certificates` JSON')
    }
    Data Integrity

    The logic for removing certificates and marker from the cycle record before validation and marker computation has changed. Reviewers should verify that this does not introduce subtle bugs or inconsistencies, especially if other fields are added or removed in the future.

    const cycleRecordCopy = { ...cycleRecord }
    // remove certificates from the cycle record
    delete (cycleRecordCopy as subscriptionCycleData).certificates
    
    const err = Utils.validateTypes(cycleRecordCopy, {
      activated: 'a',
      activatedPublicKeys: 'a',
      active: 'n',
      apoptosized: 'a',
      archiverListHash: 's',
      counter: 'n',
      desired: 'n',
      duration: 'n',
      expired: 'n',
      joined: 'a',
      joinedArchivers: 'a',
      joinedConsensors: 'a',
      leavingArchivers: 'a',
      lost: 'a',
      lostSyncing: 'a',
      marker: 's',
      maxSyncTime: 'n',
      mode: 's',
      networkConfigHash: 's',
      networkId: 's',
      nodeListHash: 's',
      previous: 's',
      refreshedArchivers: 'a',
      refreshedConsensors: 'a',
      refuted: 'a',
      removed: 'a',
      returned: 'a',
      standbyAdd: 'a',
      standbyNodeListHash: 's',
      standbyRemove: 'a',
      start: 'n',
      syncing: 'n',
      target: 'n',
      archiversAtShutdown: 'a?',
      lostArchivers: 'a',
      refutedArchivers: 'a',
      removedArchivers: 'a',
    })
    if (err) {
      Logger.mainLogger.error('Invalid Cycle Record', err)
      return false
    }
    
    // remove marker from the cycle record
    delete cycleRecordCopy.marker
    
    const computedMarker = computeCycleMarker(cycleRecordCopy)
    Logger.mainLogger.debug(

    jintukumardas
    jintukumardas previously approved these changes Aug 7, 2025
    @mssabr01
    Copy link
    Contributor

    mssabr01 commented Aug 7, 2025

    do security review

    Copy link
    Contributor

    github-actions bot commented Aug 7, 2025

    🔍 Security Review Report

    Click to expand security review details
    Traceback (most recent call last):
      File "/home/runner/work/archiver/archiver/argus-agent/agent.py", line 15, in <module>
        from agno.agent import Agent
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/agent/__init__.py", line 1, in <module>
        from agno.agent.agent import (
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/agent/agent.py", line 34, in <module>
        from agno.memory.agent import AgentMemory, AgentRun
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/memory/__init__.py", line 1, in <module>
        from agno.memory.agent import AgentMemory
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/memory/agent.py", line 7, in <module>
        from agno.memory.classifier import MemoryClassifier
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/memory/classifier.py", line 6, in <module>
        from agno.models.base import Model
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/models/base.py", line 27, in <module>
        from agno.models.response import ModelResponse, ModelResponseEvent, ToolExecution
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/models/response.py", line 8, in <module>
        from agno.tools.function import UserInputField
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/tools/__init__.py", line 1, in <module>
        from agno.tools.decorator import tool
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/tools/decorator.py", line 4, in <module>
        from agno.tools.function import Function, get_entrypoint_docstring
      File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/agno/tools/function.py", line 7, in <module>
        from packaging.version import Version
    ModuleNotFoundError: No module named 'packaging'
    
    

    Generated by argus-agent security review

    @mssabr01
    Copy link
    Contributor

    mssabr01 commented Aug 7, 2025

    do security review

    Copy link
    Contributor

    github-actions bot commented Aug 7, 2025

    🔍 Security Review Report

    Click to expand security review details
    Fetching pull request(s) from github...
    https://github.com/shardeum/archiver/pull/286
    https://github.com/shardeum/archiver/pull/286
    Processing PR 1/1: https://github.com/shardeum/archiver/pull/286
    Code review...
    ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
    ┃                         Security Code Review Report                          ┃
    ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
    
    
                                        Summary                                     
    
    Ticket: Add certificates to cycle data                                          
    
    Description: This pull request introduces changes to store cycle certificates.  
    The certificates are received from consensus nodes and are now persisted in the 
    archiver's database along with the cycle data. This change enables more robust  
    validation of cycle records.                                                    
    
    The modifications span data collection, processing, storage, and checkpointing  
    logic. The key aspect is the addition of a certificates field to the cycle data 
    structure. This field is correctly handled by stripping it during               
    security-sensitive operations like marker validation and checkpoint hashing to  
    maintain data integrity and prevent validation bypasses.                        
    
    
                                    List of changes                                 
    
    The pull request https://github.com/shardeum/archiver/pull/286 includes the     
    following changes:                                                              
    
     • src/dbstore/index.ts: Adds a new certificates column of type JSON to the     
       cycles table using a non-breaking ALTER TABLE ... ADD COLUMN IF NOT EXISTS   
       statement. This ensures backward compatibility for existing database schemas.
     • src/dbstore/types.ts: Updates the Cycle and DbCycle types to include the new 
       optional certificates field, aligning the application types with the database
       schema.                                                                      
     • src/dbstore/cycles.ts: Modifies the insertCycle, bulkInsertCycles, and       
       updateCycle functions to handle the new certificates column. The database    
       operations continue to use parameterized queries, preventing SQL injection   
       vulnerabilities.                                                             
     • src/Data/Data.ts: Alters collectCycleData to preserve the certificates field 
       on the cycle object, allowing it to be passed through the data processing    
       pipeline for storage. Previously, this field was deleted.                    
     • src/Data/Collector.ts: Updates storeCycleData to include the certificates    
       from the cycle record into the object that is being prepared for database    
       insertion.                                                                   
     • src/Data/Cycles.ts: Modifies validateCycleData to create a copy of the cycle 
       record and remove the certificates field from the copy before computing the  
       cycle marker and performing validation. This is a critical security measure  
       to ensure validation is performed only on consensus-critical data.           
     • src/checkpoint/CycleData.ts: Introduces a stripNonConsensusFields function to
       remove the certificates field from cycle data before generating checkpoint   
       hashes and during checkpoint validation. This ensures that non-consensus data
       does not affect the integrity of checkpoints.                                
    
    
                              Overall Security Assessment                           
    
    The review focused on data integrity, validation logic, and secure data         
    handling. The changes are well-implemented from a security perspective. The most
    critical security consideration—ensuring that the new certificates data does not
    interfere with the existing cycle marker validation—has been addressed correctly
    by stripping this field before any hashing or validation occurs. Database       
    interactions remain secure against SQL injection. The schema migration is       
    handled safely.                                                                 
    
    
                                   Security Findings                                
    
    No vulnerabilities were found in this pull request. The design correctly        
    isolates non-consensus data (certificates) from consensus-critical validation   
    logic (cycle marker computation).                                               
    
    
                                   Security Concerns                                
    
    There are no major security concerns with this implementation. A minor point on 
    code robustness is the use of type assertion in src/Data/Collector.ts:          
    
                                                                                    
     certificates: (cycleRecord as unknown as subscriptionCycleData).certificates   
                                                                                    
    
    While this is safe within the current data flow, it relies on the dynamic       
    presence of the certificates property. Future code changes that modify the data 
    flow into storeCycleData must ensure this property is present to avoid potential
    runtime errors. This is a code quality consideration rather than a security     
    vulnerability.                                                                  
    
    
                                         Resume                                     
    
    The pull request is well-structured and securely implements the desired         
    functionality. The changes enhance the archiver's ability to validate cycle data
    without introducing new security risks. The code demonstrates a good            
    understanding of security principles, particularly regarding the separation of  
    consensus-critical data from auxiliary validation data.                         
    
    NO MAJOR SECURITY CONCERNS FOUND                                                
    Done
    
    

    Generated by argus-agent security review

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants