Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contracts/ccip/capability: per (donId, pluginType) active config indexes #1488

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

makramkd
Copy link
Contributor

@makramkd makramkd commented Oct 4, 2024

Motivation

Using a single s_activeConfigIndex for all configurations leads to buggy outcomes (see below for an example trace).

for commit:
setCandidate(): 
    s_activeConfigIndex == 0
    candidateIndex := _getCandidateIndex(): 0 ^ 1 == 1
    s_configs[donId][pluginType][candidateIndex] = newConfig

promoteCandidate():
    s_activeConfigIndex == 0
    _getCandidateIndex(): 0 ^ 1 == 1
    s_activeConfigIndex ^= 1; => s_activeConfigIndex == 1

call getActiveDigest(): s_configs[donId][pluginType][1].configDigest returns correct digest

for exec:
setCandidate():
    s_activeConfigIndex == 1
    candidateIndex := _getCandidateIndex(): 1 ^ 1 == 0
    s_configs[donId][pluginType][candidateIndex] = newConfig

promoteCandidate(): 
    s_activeConfigIndex == 1
    _getCandidateIndex(): 1 ^ 1 == 0
    s_activeConfigIndex ^= 1; => s_activeConfigIndex == 0

call getActiveDigest(donId, commit):
    // commit's active is at index 1 (see above)
    // but since s_activeConfigIndex is set to zero, we access the array at 0
    // but this config is empty because it gets deleted in commit promoteCandidate
    s_configs[donId][commit][0].configDigest // 0x0 digest

Solution

Make s_activeConfigIndex per-donId per-plugin instead.

Copy link
Contributor

github-actions bot commented Oct 4, 2024

LCOV of commit 48bb4fc during Solidity Foundry #8518

Summary coverage rate:
  lines......: 97.8% (2263 of 2313 lines)
  functions..: 95.3% (426 of 447 functions)
  branches...: 93.6% (537 of 574 branches)

Files changed coverage rate: n/a

@makramkd makramkd marked this pull request as ready for review October 4, 2024 14:12
@makramkd makramkd requested review from elatoskinas, RayXpub and a team as code owners October 4, 2024 14:12
@makramkd makramkd enabled auto-merge (squash) October 4, 2024 17:25
@makramkd makramkd merged commit e97914d into ccip-develop Oct 4, 2024
127 checks passed
@makramkd makramkd deleted the mk/CCIP-3665 branch October 4, 2024 19:00
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

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.

3 participants