Skip to content

fix: ensure task_id is monotonic (incrementing) (#13) - #321

Open
Rodrigoue9 wants to merge 1 commit into
Vero-protocol:mainfrom
Rodrigoue9:fix/issue-13-monotonic-task-id
Open

Rodrigoue9 wants to merge 1 commit into
Vero-protocol:mainfrom
Rodrigoue9:fix/issue-13-monotonic-task-id

Conversation

@Rodrigoue9

@Rodrigoue9 Rodrigoue9 commented Oct 3, 2026 •

Copy link
Copy Markdown

Summary

What was wrong

Previously, VeroContract allowed callers with the TaskManager role to specify arbitrary, sparse task_id values when registering tasks. This exposed the protocol to task ID collisions, out-of-order IDs, sequence gaps, and potential ID resurrection attacks if a purged task ID were re-registered after removal from storage.

What was done & Alternatives Considered

This pull request resolves Issue #13 by introducing a strictly monotonic sequential counter stored in instance storage (DataKey::TaskCounter). Task registration now enforces that each incoming task_id strictly equals current_counter + 1. The counter starts at 0 (meaning the first registered task is ID 1) and advances strictly by 1 per task. The counter is preserved across task lifecycle transitions (including purge_task), permanently preventing ID reuse or resurrection.

  • Alternative considered: Auto-generating and assigning task_id inside register_task without requiring caller to pass it. Rejected because altering the signature register_task(admin, task_id, min_votes) would break public ABI compatibility, existing BatchCall::RegisterTask enum variants, and external client integrations. Enforcing sequential IDs against current_counter + 1 satisfies the monotonic guarantee while preserving backward-compatible signatures.
  • Alternative considered: Allowing gaps as long as task_id > last_id. Rejected because the issue acceptance criteria strictly specified "IDs increment by exactly 1" to ensure dense contiguous sequencing.

What to look at

  • Storage permanence: Reviewers should verify that purge_task removes the task data but intentionally leaves DataKey::TaskCounter untouched, ensuring purged IDs cannot be revived.
  • Batch atomicity: In register_tasks, sequence checks are performed up-front before the reentrancy lock, and state changes (including DataKey::TaskCounter) are applied atomically. A gap or out-of-order ID anywhere in a batch aborts the entire registration without updating the counter.
  • Public getter: Added public entrypoint get_task_counter(env: Env) -> u64 to allow clients and frontends to determine the next expected task ID before submitting transactions.
  • Test updates: Existing integration tests in archive_task_auth.rs, consensus_delegation.rs, integration.rs, and multi_contract_isolation.rs that previously used arbitrary mock IDs (e.g. 42, 100) were updated to use sequential IDs (1, 2, 3) to comply with the monotonic invariant.

How verified

  • Added 10 dedicated integration tests in tests/monotonic_task_id.rs covering counter initialization, sequential increments, gap rejection, zero ID rejection, duplicate/lower ID rejection, full lifecycle transitions, purge non-reuse, multi-contract isolation, batch atomic rollback, and RBAC/pause validation.
  • Added unit test test_register_tasks_batch_monotonic in src/task.rs.
  • Ran full test suite across the workspace: 216 tests passed.
  • Ran cargo fmt --all -- --check: 100% compliant.
  • Ran cargo clippy --workspace --all-targets -- -D clippy::all: 0 warnings, 0 errors.

Related Issue

Closes #13

Changes Made

  • Storage Layout (src/contracts/storage_layout.rs):
    • Added DataKey::TaskCounter variant in instance storage tracking the highest registered task sequence ID.
  • Task Management (src/task.rs):
    • Implemented pub fn get_task_counter(env: &Env) -> u64 reading DataKey::TaskCounter (defaulting to 0).
    • Updated register_tasks to validate that incoming task IDs strictly increment by 1 from current_counter.
    • Atomically commits the updated counter under the reentrancy lock.
    • Added unit test test_register_tasks_batch_monotonic verifying sequential batches, internal gap rejection, and out-of-order rejection.
  • Contract Entrypoint (src/contracts/proxy_entry/entry_tasks.rs):
    • Added documented public entrypoint pub fn get_task_counter(env: Env) -> u64.
  • Integration Test Suite (tests/monotonic_task_id.rs):
    • Created 10 integration tests validating counter lifecycle, edge cases, purge non-reuse, multi-contract isolation, and rollback semantics.
  • Existing Test Mock IDs:
    • Updated mock task IDs in tests/archive_task_auth.rs, tests/consensus_delegation.rs, tests/integration.rs, and tests/multi_contract_isolation.rs to start sequentially from ID 1.

Testing Performed

  • Ran cargo test --test monotonic_task_id: 10/10 tests passed.
  • Ran cargo test --lib task::tests: passed.
  • Ran full test suite across the workspace: 216 tests passed.
  • Ran cargo fmt --all -- --check: formatting 100% compliant.
  • Ran cargo clippy --workspace --all-targets -- -D clippy::all: 0 warnings, 0 errors.

Checklist

  • I have linked the related issue with Closes #13.
  • I have tested my changes locally.
  • I have run cargo fmt --check.
  • I have run cargo clippy -- -D warnings where applicable.
  • I have updated relevant documentation.

Enforce strictly incrementing sequential task IDs via DataKey::TaskCounter to prevent ID collisions, gaps, and ID reuse.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: ensure task_id is monotonic (incrementing)

1 participant