fix(sdk): enforce wrapped intent safety preflights - #3100
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE HIGH account-age scrutiny (49-day-old account), moderated by admin permission, substantial contributions, matching author/committer, and no known Gittensor association; fix/sdk-intent-safety-preflights-ready → release-448. FindingsNo findings. ConclusionStatic analysis found no security vulnerability, malicious behavior, or AI-review trust-boundary modification. The previously reviewed bounded canonical-receipt handling remains addressed. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Established repository admin with substantial recent contributions; no trusted gittensor allowlist match (UNKNOWN). The implementation matches the substantive PR description and includes focused coverage for the safety-sensitive paths. PR #3097 overlaps the single-hotkey root-claim quote fix, but this PR provides the broader integrated solution and stronger coverage. This PR is the better candidate. Recommend closing #3097. Checks:
FindingsNo findings. ConclusionThe wrapped-origin, claim-admission, shielded-finality, and storage-invariant changes are coherent and adequately tested. No substantive merge-blocking issue was found. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| finalized = await self.substrate.finalized_block_number() | ||
| except Exception: | ||
| finalized = block - 1 | ||
| if finalized >= block: |
There was a problem hiding this comment.
[MEDIUM] Finalization RPC failure causes an infinite wait
Every finalized_block_number() failure is converted to block - 1, so a permanently unavailable or unsupported RPC leaves this while True loop sleeping forever. Propagate the RPC failure instead, or apply an explicit bounded retry policy, so wait_for_finalization=True cannot hang indefinitely.
| finalized = await self.substrate.finalized_block_number() | |
| except Exception: | |
| finalized = block - 1 | |
| if finalized >= block: | |
| finalized = await self.substrate.finalized_block_number() |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| # Finality applies to the canonical block at this height. | ||
| # Re-resolve after a reorg instead of returning a receipt | ||
| # observed on a fork before the finalized head caught up. | ||
| canonical_hash = await self.substrate.block_hash(block) |
There was a problem hiding this comment.
[MEDIUM] Canonical-hash RPC can defeat the finalization bound
After finality is detected, this newly added RPC is awaited without a timeout. A stalled or adversarial endpoint can therefore hang wait_for_finalization=True indefinitely, bypassing the bounded polling immediately above. Apply the same per-attempt timeout and convert exhaustion into ChainError; the subsequent receipt lookup also needs a bound if it can block independently.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| ) | ||
| raise ChainError(message) from canonical_hash_error | ||
| if canonical_hash != block_hash: | ||
| inner = await self.substrate.find_extrinsic(inner_hash, canonical_hash) |
There was a problem hiding this comment.
[MEDIUM] Canonical receipt lookup remains unbounded
After a pre-finality reorg, this newly introduced RPC await has no timeout. A node that returns the canonical hash but never completes find_extrinsic can therefore hang a shielded submission indefinitely despite the surrounding finalization bounds. Apply a bounded timeout/retry policy here as well, and fail clearly or resume scanning when it is exhausted.
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
1 participant
Why
This closes six SDK safety gaps found while reviewing the
release-448behavior around shielded submissions, root-claim preflights, wrapped execution, claim-then-unstake, and finalization. It also includes the focused CI and correctness fixes found while validating the release branch.Original findings addressed
1. Shielded submissions now enforce intent hard stops
Previously
submit_shielded()appliedPolicychecks but bypassed the hard stops returned by an intent.This change:
IntentPreflightresult containing effects, warnings, hard stops, and claim affordability dataBatch, with child indexes preserved in messages2. Root claims now preflight
RootClaimTooHeavyThe runtime rejects root claims when either:
hotkey_count × existing_network_count > 256, orThe SDK now mirrors that admission envelope before signing:
NetworksAdded=truenetworks, including the root minimumclaim_root_with_hotkeywhen a coldkey-wide claim is too large3.
--claim --allnow removes newly claimed root yieldThe old flow resolved
allfrom the pre-claim stake and then batched a claim before that fixed-amount unstake, leaving the newly claimed yield staked.For root
claim=Trueplusamount_alpha="all", the SDK now:u64::MAXto plainremove_stakeremove_stakecap to resolve the amount from the live post-claim position inside the atomic batchRemoveStakeLimiton root after validating that its root limit is validRootStakeUnlockIntervalis active, because a successful claim refreshes the hold and the following unstake cannot succeedThis keeps claim and full exit atomic while removing principal and yield together.
4. Wrapped execution now separates dispatch origin from fee payer
Proxy and multisig wrappers can make the account whose state changes different from the member or delegate paying the extrinsic fee.
The executor now derives both accounts explicitly:
register_subnetcompletion use the semantic operation and actual dispatch-origin owner, rather than the wrapper operation or member addressThe final registration-completion item was added after review found that origin separation had not yet been carried through the post-submission path.
5. Shielded APIs now honor and safely bound finalization requests
The shielded raw-call and multisig approval paths now propagate
wait_for_finalizationinstead of hardcoding it to false.When finalization is requested, decrypted inner-extrinsic resolution now:
ChainErrorafter four consecutive finalized-head RPC failuresChainErrorwhen either RPC remains unavailable or never returnsThis prevents a finalized shielded API call from returning only an inclusion-level inner receipt, and prevents a permanently unavailable, non-returning, or stalled finality RPC from making the call wait forever.
6. Coldkey-wide thresholds are now evaluated per validator
The runtime applies
RootClaimableThresholdseparately to each validator basket. The SDK previously compared the threshold with one aggregate coldkey-wide owed value.The quote now:
get_root_basket_positionsA review follow-up also fixed the zero-entitlement case: validators omitted by
get_root_basket_positionshave no owed shares. They are no longer misclassified as below-threshold positions or charged basket scan work in the quote.Additional correctness and CI fixes
UserDatadecoding now rejects trailing bytes instead of accepting a valid SCALE prefix and ignoring leftover ciphertext. This removes a probabilistic decode path that made the randomized ciphertext test flaky and applies strict decoding to reveal, inner-ciphertext, decrypt, and signed-decrypt paths.TotalAlphaStakednow removes its storage key when a live delta reduces the aggregate to zero. This restores the expected invariant after a final unstake or subnet dissolution without changing the storage read/write cost.rootDirectorycontract while retaining the existing trust boundary.git diff --checkwhile retaining an exact live-node drift gate.Additional safety behavior
Design scope
The existing
IntentPreflightabstraction is intentionally retained. A broader redesign of how generic wrappers merge balance requirements was considered during review, but it is not needed to fix a demonstrated bug and would materially expand this correctness-focused PR.Validation
uv run --no-sync pytest tests/unit/test_intents_table.py tests/unit/test_root_claim_fee.py tests/unit/test_multisig_safety.py -q— 566 passeduv run --no-sync ruff check .uv run --no-sync ruff format --check .uv run --no-sync python ../../website/apps/bittensor-website/scripts/generate.py --checkuv run --no-sync python -m codegen.check --coverageuv run --no-sync python -m codegen.check --namesuv run --no-sync python -m codegen.check --unitsuv run --no-sync python -m codegen.check --namespacesTotalAlphaStakedfinal-unstake and subnet-dissolution regressionspython -m codegen.check --drift ws://127.0.0.1:9944against the exact CI-built v448 node — no driftcargo fmt --check --allgit diff --checkDocumentation
move_stake_limit, dispatch-origin versus fee-payer semantics, and bounded canonical-finality waits.moveStakeLimitABI, units, and non-payable status, plus WASM chain-extension function IDs 37 and 38.RootClaimTooHeavy, root-claim fee, staking-index, and limit-order references; regenerated transaction/query/error pages, catalogs, and namespace stubs.