Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an active-validator membership gate to the ERC20 bridge vote_epoch write path (nonce==0), rejecting non-validator callers before vote persistence and reusing a precomputed validator address→power map. Also disables the local validator signer broadcast loop on nodes that are not active validators. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Caller"
participant Meta as "MetaExtension\n(vote_epoch)"
participant ValStore as "ValidatorStore\n(build map)"
participant Signer as "Local Signer\n(getValidatorSigner)"
rect rgba(200,200,255,0.5)
Caller->>Meta: submit vote_epoch(nonce==0, addr)
Meta->>ValStore: buildValidatorEthAddressMap()
ValStore-->>Meta: validatorAddr→power map
Meta->>Meta: isLocalNodeActiveValidator(map, localIdentity)
alt local not active
Meta->>Caller: return error (not active validator)
else local active
Meta->>Signer: getValidatorSigner()
Signer-->>Meta: signer (non-nil)
Meta->>Meta: sumEpochVotingPower(using map)
Meta->>ValStore: persist epoch vote
Meta->>Caller: success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
node/exts/erc20-bridge/erc20/meta_extension.go (1)
1355-1367: Solid defense-in-depth gate; consider sharing the validator→address derivation.The early rejection before the DB query and signature verification is correct and the comment block clearly motivates it. One nit: each
nonce==0vote_epoch call now derives an Ethereum address (Keccak256 + pubkey decompress) for every validator insideisActiveValidatorAddress, and on the same handler invocationsumEpochVotingPower(lines ~3297–3325) walks the validator set again and rebuilds the sameeth address → powermap. For larger validator sets you could compute the map once per handler call (or memoize per block) and reuse it both for this membership check and the BFT power summation.Not a blocker — just an easy follow-up if this becomes a hot path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/exts/erc20-bridge/erc20/meta_extension.go` around lines 1355 - 1367, The early membership check in the vote_epoch handler redundantly derives validator Ethereum addresses twice (once inside isActiveValidatorAddress and again inside sumEpochVotingPower); refactor to compute the validator-address→power map once per handler invocation (or memoize per block) and pass that map into isActiveValidatorAddress and sumEpochVotingPower (or change isActiveValidatorAddress to accept the precomputed map) so both the membership gate and BFT power summation reuse the same address derivation and avoid duplicated Keccak/decompress work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node/exts/erc20-bridge/erc20/meta_extension.go`:
- Around line 1355-1367: The early membership check in the vote_epoch handler
redundantly derives validator Ethereum addresses twice (once inside
isActiveValidatorAddress and again inside sumEpochVotingPower); refactor to
compute the validator-address→power map once per handler invocation (or memoize
per block) and pass that map into isActiveValidatorAddress and
sumEpochVotingPower (or change isActiveValidatorAddress to accept the
precomputed map) so both the membership gate and BFT power summation reuse the
same address derivation and avoid duplicated Keccak/decompress work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 592a3d9b-17b5-4b9c-b89d-eca013893ee1
📒 Files selected for processing (2)
node/exts/erc20-bridge/erc20/meta_extension.gonode/exts/erc20-bridge/erc20/validator_membership_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
node/exts/erc20-bridge/erc20/meta_extension.go (1)
1355-1373: LGTM — solid defense-in-depth at the action boundary.Building the validator eth-address map once and reusing it for the membership gate and the BFT power sum is the right shape: cheap-checks-first ordering (length → caller parse →
canVoteEpoch→ membership → signature verify), and rejecting beforevoteEpochkeeps non-validator rows out ofepoch_votesso withdrawal proofs stay clean.One tiny note: a validator using a non-secp256k1 key gets the same
"not in the active validator set"error here even though they technically are in the set — they just can't participate in non-custodial voting. Optional, but the message could be slightly more diagnostic, e.g.- return fmt.Errorf("vote_epoch: caller %s is not in the active validator set", from.Hex()) + return fmt.Errorf("vote_epoch: caller %s is not an active secp256k1 validator", from.Hex())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/exts/erc20-bridge/erc20/meta_extension.go` around lines 1355 - 1373, The current membership rejection returns a generic "not in the active validator set" when the eth address lookup via validatorEthMap (built by buildValidatorEthAddressMap) fails; update the error to be more diagnostic by detecting whether the caller's node exists in the validator set but lacks a derived secp256k1 eth address (i.e. non-secp256k1 key) versus truly absent, and return distinct messages accordingly (e.g., "caller %s is a validator but cannot derive secp256k1 eth address; cannot participate in non-custodial voteEpoch" vs the existing "caller %s is not in the active validator set"), using symbols nonce/nonCustodialNonce, from, validatorEthMap, buildValidatorEthAddressMap and voteEpoch to locate the check to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/exts/erc20-bridge/erc20/meta_extension.go`:
- Around line 3193-3203: getValidatorSigner currently gates starting the
vote_epoch loop at OnStart using isLocalNodeActiveValidator, so a node promoted
after startup never begins signing; modify getValidatorSigner (or the startup
flow that calls it) to periodically re-check membership (e.g., spawn a
background ticker/goroutine that calls isLocalNodeActiveValidator and starts the
bridge signer when it becomes true) and ensure this background check respects
the same action-side gate around epoch_votes/vote_epoch to avoid races, or
alternatively add clear operator documentation to the runbook describing the
restart requirement for late-promoted validators; reference getValidatorSigner,
OnStart, isLocalNodeActiveValidator, vote_epoch and epoch_votes when
implementing or documenting the change.
---
Nitpick comments:
In `@node/exts/erc20-bridge/erc20/meta_extension.go`:
- Around line 1355-1373: The current membership rejection returns a generic "not
in the active validator set" when the eth address lookup via validatorEthMap
(built by buildValidatorEthAddressMap) fails; update the error to be more
diagnostic by detecting whether the caller's node exists in the validator set
but lacks a derived secp256k1 eth address (i.e. non-secp256k1 key) versus truly
absent, and return distinct messages accordingly (e.g., "caller %s is a
validator but cannot derive secp256k1 eth address; cannot participate in
non-custodial voteEpoch" vs the existing "caller %s is not in the active
validator set"), using symbols nonce/nonCustodialNonce, from, validatorEthMap,
buildValidatorEthAddressMap and voteEpoch to locate the check to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67a50d18-ece4-41f8-a425-1ba8b247d10a
📒 Files selected for processing (1)
node/exts/erc20-bridge/erc20/meta_extension.go
1 participant
resolves: https://github.com/truflation/website/issues/3752
Summary by CodeRabbit