record db cost for precompile view functions#2757
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: author has write permission and substantive prior contribution history; no Gittensor allowlist hit observed; branch record-db-cost-precompile -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionVULNERABLE because both simulated swap precompile paths can still execute rollbacked PalSwap initialization writes while charging only read gas. I found no malicious indicators. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY by contribution heuristic; author has write permission and extensive prior subtensor/precompile history. Overlapping PRs #2723, #2685, and #2610 share files but appear unrelated in scope. The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it. The PR body is template-heavy, so I populated Duplicate-work check: overlapping PRs #2723, #2685, and #2610 do not appear to target this DB-cost accounting fix by title/scope, so I have no close recommendation. I did not run cargo tests; the blocking issue is visible from the static swap call graph. Findings
Prior-comment reconciliation
ConclusionBlocking because the prior swap simulation storage-undercharge issue is still present. The simulation precompiles need to charge the full bounded storage footprint, or be refactored into a read-only quote path with gas-accounting tests. 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| tao: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated tao-for-alpha swap omits rollback-write gas
sim_swap runs through SwapEngine::swap(..., should_rollback = true), and swap_inner calls maybe_initialize_palswap before the transaction is rolled back. On a subnet where PalSwapInitialized is false, that path writes SwapBalancer and PalSwapInitialized, but this precompile charges only reads. Because rollback leaves the subnet uninitialized, callers can repeat the undercharged write work through this view call. Charge the rollback writes as well.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
| alpha: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated alpha-for-tao swap omits rollback-write gas
This simulation path has the same rollback-write gap: sim_swap can initialize PalSwap inside a rolled-back transaction, writing SwapBalancer and PalSwapInitialized while the precompile records only read gas. Since the rollback means the initialization is not persisted, the write work can be repeatedly forced at a read-only gas price. Charge the two possible rollback writes before entering the simulation.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
open-junius
requested review from
JohnReedV,
evgeny-s,
gztensor,
l0r1s and
sam0x17
| tao: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated tao-for-alpha swap omits rollback-write gas
sim_swap is not a pure read path for uninitialized PalSwap subnets: it invokes the swap engine with rollback, and maybe_initialize_palswap can write both SwapBalancer and PalSwapInitialized before the transaction rolls back. Charging only reads lets callers make the EVM execute two storage writes without paying write gas. Charge the worst-case rollback writes before the simulated swap.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
| alpha: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated alpha-for-tao swap omits rollback-write gas
This simulated swap uses the same rollbacked swap engine path as the tao-for-alpha case. On an uninitialized PalSwap subnet, maybe_initialize_palswap can insert SwapBalancer and PalSwapInitialized before rollback, but this precompile charges only read gas. Charge the two possible rollback writes as well.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated tao-for-alpha swap omits rollback-write gas
sim_swap calls the swap engine with should_rollback = true, but mechanism-1 swaps still run maybe_initialize_palswap inside that rollbacked transaction. On an uninitialized subnet that writes SwapBalancer and PalSwapInitialized, so this precompile path can perform rollbacked storage writes while charging only read gas. Charge the worst-case write cost before entering sim_swap.
| // SubnetMechanism + swap simulation reads | |
| handle.record_db_reads::<R>(9)?; | |
| // SubnetMechanism + swap simulation reads + rollbacked PalSwap initialization writes | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated alpha-for-tao swap omits rollback-write gas
This simulation path has the same undercharge as the tao-for-alpha path: sim_swap can initialize PalSwap inside a rollbacked transaction, causing SwapBalancer and PalSwapInitialized writes that are not reflected in the recorded gas. Charge the worst-case write cost before the simulation call.
| // SubnetMechanism + swap simulation reads | |
| handle.record_db_reads::<R>(9)?; | |
| // SubnetMechanism + swap simulation reads + rollbacked PalSwap initialization writes | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| tao: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated tao-for-alpha swap omits rollback-write gas
sim_swap still routes through the dynamic-subnet swap path, where swap_inner calls maybe_initialize_palswap. On an uninitialized subnet that inserts both SwapBalancer and PalSwapInitialized before the surrounding transaction rolls back, so this precompile can perform write-cost storage work while charging only 9 reads. Charge the rollback writes too, or refactor simulation into a strictly read-only quote path.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
| alpha: u64, | ||
| ) -> EvmResult<U256> { | ||
| // SubnetMechanism + swap simulation reads | ||
| handle.record_db_reads::<R>(9)?; |
There was a problem hiding this comment.
[MEDIUM] Simulated alpha-for-tao swap omits rollback-write gas
This path has the same undercharge as simSwapTaoForAlpha: SwapHandler::sim_swap can initialize PalSwap state inside the rollback transaction, writing SwapBalancer and PalSwapInitialized, but the precompile records only read gas before entering the call. Charge those possible writes or make the simulation path read-only.
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_reads::<R>(9)?; | |
| handle.record_db_writes::<R>(2)?; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
2 participants
Description
Record the db cost used in the precompile view function.
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.