feat(precompiles): implement TIP-1011 restrictions#3385
feat(precompiles): implement TIP-1011 restrictions#3385legion2002 merged 20 commits intotanishk/tip-1011-primitives-exactfrom
Conversation
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 476b865f8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_selector_rules( |
There was a problem hiding this comment.
it seems that this check is entirely stateless and is something that we would want to do in the pool so ideally we have it as a helper on CallScope
9f1ed91 to
5434753
Compare
| let mut limit_state = self.spending_limits[limit_key][token].read()?; | ||
| limit_state.remaining = limit_state.remaining.saturating_add(amount); |
There was a problem hiding this comment.
Currently, if a period rolls over resetting remaining to max and then a fee refund arrives for a transaction charged in the previous period, remaining gets pushed above max because saturating_add has no upper bound. We should clamp remaining to min(remaining, amount) after the add when period > 0 so that refunds never grant more budget than the owner originally authorized per period.
There was a problem hiding this comment.
yeah I had thought about this, but don't think we can ever reach this path.
the refund is an atomic operation, so period rollover cannot happen in the middle of a tx, and the refund can never be more than the initial spending limit.
But I can add a min() clamp as a defense-in-depth improvement if you want, because it should be cheap to do the check.
5434753 to
42d8683
Compare
| /// Key-level call scope. | ||
| #[derive(Debug, Clone, Storable, Default)] | ||
| pub struct KeyScope { | ||
| pub is_scoped: bool, |
There was a problem hiding this comment.
I understand that we need this flag for KeyScope to distinguish between the key not allowing any calls and the key allowing all calls
however, for TargetScope and SelectorScope this flag seems redundant? given that we track Set of keys that actually exist it should not be possible to confuse a non-existing selector/target scope with an existing one because we would always perform the inclusion check via the Set?
There was a problem hiding this comment.
my intuition here was to keep things symmetrical, and explicit to understand.
If we remove the flag from TargetScope/SelectorScope. An empty array in these scopes means "allow-all", but an empty array in "KeyScope" would mean deny-all.
Having thought about it more, I am changing my mental model to -
- it doesn't make logical sense to keep a scope around if all the selectors/entries in that scope are blocked. So in this case the default meaning of an empty list is "allow-all"
But we have an exception in call scope, because it is the topmost level, so we can't just remove the entry, for the deny-all case.
In this case we add an explicit bool for "allow-all", and the default empty list value is "deny-all"
I think this is still slightly confusing in the implementation, but people would not be interacting with storage slots directly in most cases.
And I have also added explicit scoping bools to the get_allowed_calls return value to make this more clear.
| let target = scope.target; | ||
|
|
||
| if !scope.selectorRules.is_empty() { | ||
| self.validate_selector_rules(target, &scope.selectorRules)?; | ||
| } | ||
|
|
||
| if !self.key_scopes[account_key].targets.contains(&target)? { | ||
| let count = self.key_scopes[account_key].targets.len()?; | ||
| if count >= MAX_CALL_SCOPES as usize { | ||
| return Err(AccountKeychainError::scope_limit_exceeded().into()); | ||
| } |
There was a problem hiding this comment.
upsert_target_scope doesn't reject Address::ZERO as target. IIUC Address::ZERO is used as the sentinel value in get_allowed_calls to represent "scoped but no targets".
If a user calls setAllowedCalls with target = address(0), it'll be inserted into the targets set, corrupting the sentinel distinction, get_allowed_calls would return it as a real target, and validate_call_scope_for_transaction would allow calls to address(0).
| if scopes.is_empty() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
hmm i don't think it's expected that we'd just ignore this call if scopes are empty?
There was a problem hiding this comment.
we have a separate endpoint "removeAllowedCalls", if people want to remove scopes.
do you mean we should revert here instead?
We can't iterate over the scopes and remove them ourselves, because we don't have an enumerable map for this onchain. We expect that people will call removeAllowedCalls with the scopes instead. Or disable the key altogether.
There was a problem hiding this comment.
then empty scopes should probably make the key unrestricted?
| // Check key is valid (exists and not revoked) | ||
| let current_timestamp = self.storage.timestamp().saturating_to::<u64>(); | ||
| let key = self.load_active_key(account, key_id, current_timestamp)?; |
There was a problem hiding this comment.
couldn't load_active_key fetch timestamp internally?
There was a problem hiding this comment.
there are a couple of places where the timestamp is already available, so we don't have to fetch it again, like the validate_keychain_authorization fn
There was a problem hiding this comment.
it's not expensive to fetch it at all, we carry it around as a u64
a29e4af to
e51b9cf
Compare
## Stack Context This is the top PR in the TIP-1011 stack. - Parent: [#3385](#3385) `feat(precompiles): implement TIP-1011 restrictions` - Child: none ## What This PR Does This PR enforces TIP-1011 at runtime: - handler-side validation and intrinsic gas accounting for `key_authorization` - execution-time enforcement of periodic limits and call scopes - rejection of invalid access-key contract-creation or scoped calls before execution - txpool validation so invalid transactions are filtered before propagation This is the integration layer where the protocol shape from PR1 and the precompile semantics from PR2 are actually enforced by the node. ## How To Review 1. Start with `crates/revm/src/handler.rs`. This is the core of the PR: hardfork gating, intrinsic gas, same-transaction authorization behavior, and per-call enforcement. 2. Then review `crates/transaction-pool/src/validator.rs` and confirm txpool admission matches the runtime rules closely enough to reject bad transactions early without diverging from execution. 3. Review the remaining runtime tests and regressions after that. 4. Think about the stack as a whole while reviewing this PR: how the signed payload from [#3384](#3384) feeds the precompile state from [#3385](#3385), and how this handler and validator layer consumes both. ## Note If you want the full end-to-end diff in one place, see [#3323](#3323). It is useful for seeing how the ideas interact across the stack, and Cyclops also runs there.
d604c15
into
tanishk/tip-1011-primitives-exact
Stack Context
This is the middle PR in the TIP-1011 stack.
feat(primitives): add TIP-1011 wire format and ABIfeat(node): enforce TIP-1011 restrictionsWhat This PR Does
This PR implements TIP-1011 inside the
AccountKeychainprecompile:This is the PR where the spec becomes concrete precompile and storage behavior. It intentionally stops short of handler and txpool runtime enforcement, which lands in PR3.
How To Review
Review this like a smart-contract PR.
crates/precompiles/src/account_keychain/mod.rsand verify the storage model and invariants: uniqueness, rollover behavior, normalization, selector and recipient validation, and any assumptions about reads and writes.authorizeKey,setAllowedCalls,removeAllowedCalls, andupdateSpendingLimit.crates/precompiles/src/account_keychain/dispatch.rsfor ABI routing and fork gating.targets/target_scopespath more efficient. It is worth keeping in mind when evaluating this layout.Note
If you want the full end-to-end diff in one place, see #3323. It is useful for seeing how the ideas interact across the stack, and Cyclops also runs there.