Implement z_sendfromaccount#1
Conversation
|
Review Complete Files Reviewed: 29 By Severity:
PR introduces new RPC methods ( Files Reviewed (29 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟡 Medium (48/100) — 1 medium finding, 3 low · 3265 LOC across 29 files
Summary
This PR adds three new JSON-RPC methods (z_proposetransaction, z_finalizetransaction, z_sendfromaccount), a fork-detection Depth API chain reader, a RequiredPolicyCache for privacy policy enforcement, CI workflow updates, and supporting types. The review found 5 findings (2 medium, 3 low).
Medium Severity
-
minconf=0 treated as more restrictive than minconf=1 (
z_propose_transaction.rs:94,z_send_from_account.rs:89):NonZeroU32::new(0)returnsNone, causing minconf=0 to enforcerequire_confirmed_spend=truewith 1 confirmation, while minconf=1 usesrequire_confirmed_spend=false. Violates the zcashd convention where increasing minconf monotonically increases restrictiveness. -
TOCTOU race in
find_fork_point(reader.rs:209-241): Replaces the atomicFindForkPointrequest with separateTip+Depthcalls. If the chain tip advances between calls, the computed fork point height uses a staletip_height. TheZebraChainViewalready holds a snapshot-consistent tip but the reader ignores it.
Low Severity
RequiredPolicyCachere-insertion doesn't refresh eviction order (payments.rs:280):insertskipsorder.push_backfor existing keys, so re-inserted keys stay at the front of the FIFO eviction queue and can be prematurely evicted.- Privacy policy enforcement bypassed on cache miss (
z_finalize_transaction.rs:77): When theRequiredPolicyCachemisses (eviction or restart),is_compatible_withenforcement is silently skipped. Acknowledged as tracking issue zcash#217. - Stale CI runner comment (
.github/workflows/build-arm64-nix.yml:56): Runner label changed fromubuntu-24.04-armtotenki-standard-medium-4c-8gbut inline comment was not updated.
All findings relate to code introduced or modified by this PR. No findings block merging but the two medium-severity bugs should be addressed.
| let confirmations_policy = match minconf { | ||
| Some(minconf) => NonZeroU32::new(minconf).map_or( | ||
| ConfirmationsPolicy::new_symmetrical(NonZeroU32::MIN, true), | ||
| |c| ConfirmationsPolicy::new_symmetrical(c, false), | ||
| ), |
There was a problem hiding this comment.
🟡 minconf=0 treated as more restrictive than minconf=1 in new RPC methods (bug)
In z_propose_transaction.rs (lines 94-98) and z_send_from_account.rs (lines 89-93), the minconf parameter handling uses NonZeroU32::new(minconf).map_or(...). When a caller passes minconf=0 (intending to allow unconfirmed outputs per zcashd convention), NonZeroU32::new(0) returns None, falling through to ConfirmationsPolicy::new_symmetrical(NonZeroU32::MIN, true) — enforcing 1 confirmation with require_confirmed_spend=true. However, minconf=1 uses new_symmetrical(1, false) with require_confirmed_spend=false. The result: minconf=0 is more restrictive than minconf=1, which is counterintuitive and violates the zcashd API contract where higher minconf values should monotonically increase restrictiveness. Both files are new in this PR, introduced by copy-pasting the same pattern from pre-existing code in z_send_many.rs.
💡 Suggestion: Map minconf=0 to the same behavior as minconf=1 (new_symmetrical(NonZeroU32::MIN, false)) so restrictiveness is monotonic. Alternatively, explicitly reject minconf=0 with an error: minconf=0 (unconfirmed outputs) is not supported; use minconf=1 or higher.
📋 Prompt for AI Agents
In zallet/src/components/json_rpc/methods/z_propose_transaction.rs lines 94-98 and zallet/src/components/json_rpc/methods/z_send_from_account.rs lines 89-93, change the None branch of NonZeroU32::new(minconf).map_or() from ConfirmationsPolicy::new_symmetrical(NonZeroU32::MIN, true) to ConfirmationsPolicy::new_symmetrical(NonZeroU32::MIN, false). This makes minconf=0 behave identically to minconf=1 (both use 1 confirmation with require_confirmed_spend=false), maintaining monotonicity. The pre-existing instances in z_send_many.rs and other files should also be fixed but are out of scope for this PR.
| fn insert(&mut self, key: [u8; 32], policy: PrivacyPolicy) { | ||
| // Re-inserting an existing key just refreshes the policy without growing the cache. | ||
| if self.by_pczt.insert(key, policy).is_none() { | ||
| self.order.push_back(key); | ||
| while self.order.len() > self.capacity { | ||
| if let Some(evicted) = self.order.pop_front() { | ||
| self.by_pczt.remove(&evicted); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 RequiredPolicyCache re-insertion does not refresh eviction order (bug)
In payments.rs lines 280-290, RequiredPolicyCache::insert correctly updates the policy value for an existing key via HashMap::insert, but the eviction-position refresh (order.push_back(key)) is guarded by .is_none(), meaning it only executes for new insertions. On re-insertion of an existing key, the key remains at its original position in the FIFO eviction queue. The documentation comment at line 281 says 're-inserting an existing key just refreshes the policy without growing the cache', which accurately describes the current behavior but may not match the expected semantics of a 'refresh' — typically, refreshing a cache entry should also refresh its eviction priority. A key that is repeatedly proposed-then-re-proposed could be prematurely evicted by 255 intervening proposals from other accounts.
💡 Suggestion: On re-insertion, remove the key from its current position in self.order and push it to the back, so recently-refreshed keys get eviction priority.
📋 Prompt for AI Agents
In zallet/src/components/json_rpc/payments.rs lines 280-290, modify RequiredPolicyCache::insert to refresh the eviction position of re-inserted keys. Instead of skipping order.push_back when by_pczt.insert returns Some (re-insertion), remove the key from its current position in self.order using retain and push it to the back. This ensures that re-inserted keys are moved to the most-recent position in the eviction order.
| build-arm64: | ||
| name: Build aarch64-musl via Nix | ||
| runs-on: ubuntu-24.04-arm # native arm64; free for public repos | ||
| runs-on: tenki-standard-medium-4c-8g # native arm64; free for public repos |
There was a problem hiding this comment.
🟢 Misleading inline comment on runner selection after runner label change (style)
In .github/workflows/build-arm64-nix.yml line 56, the runs-on field was changed from ubuntu-24.04-arm (GitHub's free native arm64 runner) to tenki-standard-medium-4c-8g (a self-hosted runner), but the inline comment # native arm64; free for public repos was not updated. The comment is now potentially inaccurate: the self-hosted runner's architecture is not indicated by its label, and self-hosted runners are not 'free for public repos' in the same way GitHub-hosted runners are. This could mislead future maintainers about the runner's capabilities.
💡 Suggestion: Update the comment to accurately describe the new runner, e.g., # self-hosted arm64 runner or remove the comment entirely since the runner label should be self-documenting.
📋 Prompt for AI Agents
In .github/workflows/build-arm64-nix.yml at line 56, change the inline comment from # native arm64; free for public repos to # self-hosted arm64 runner or simply remove the comment after the runner label.
| if let Some(required) = cached_required_policy(&pczt_policy_key(&pczt.serialize())) { | ||
| if !privacy_policy.is_compatible_with(required) { | ||
| return Err(LegacyCode::InvalidParameter.with_message(format!( | ||
| "The privacy policy {privacy_policy} does not permit this transaction, which \ | ||
| requires at least {required}." | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Privacy policy enforcement silently bypassed on RequiredPolicyCache eviction or restart (suggestion)
In z_finalize_transaction.rs lines 72-84, the call to cached_required_policy checks the RequiredPolicyCache for the required privacy policy of a proposed PCZT. On a cache hit, the user-supplied policy is validated against the required policy via is_compatible_with. On a cache miss — caused by FIFO eviction after 256 proposals (payments.rs:280-290), process restart, or a PCZT proposed by another node — the if let Some(required) = ... block is skipped entirely, and any caller-supplied policy is accepted without validation. This means a caller who knows their transaction requires NoPrivacy can wait for cache eviction and supply FullPrivacy to bypass the acknowledgement check. The privacy policy lattice (meet, is_compatible_with) is correctly implemented, but enforcement is structurally bypassable via the cache miss path. Acknowledged by the code comment referencing zcash#217.
💡 Suggestion: On cache miss, either (1) re-derive the required policy from the PCZT if feasible, (2) reject with an error requiring the caller to re-propose, or (3) at minimum, log a warning so the bypass is auditable. Persisting the cache to disk or using a larger non-evictable cache would also mitigate the issue.
📋 Prompt for AI Agents
In zallet/src/components/json_rpc/methods/z_finalize_transaction.rs at lines 77-84, add an else branch to the if let Some(required) = cached_required_policy(...) block. In the else branch, log a warning: warn!("PCZT required-policy cache miss; accepting supplied privacy policy {} as acknowledgment without compatibility check", privacy_policy);. Optionally, if the required policy can be derived from the PCZT bytes, compute and enforce it. Also update the OpenRPC documentation to note that when the PCZT was not recently proposed by this node, the privacy_policy parameter is accepted as acknowledgment without enforcement.
1 participant
Mirror-of: zcash#531
zcash#531