Subnet sale offer#2745
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: l0r1s has write permission, an established account, substantial prior contribution history, and matching PR author/commit authors. Branch subnet-sale-offer -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY by repo-history heuristic; author has write access and substantial recent subtensor contribution history, so review focused on runtime correctness rather than intent. PR body is substantive and matches the implementation at a high level. No duplicate-work recommendation from the overlapping PR list. I did not run tests; the blocking issues are static weight-accounting problems. I also could not auto-verify the devnet Findings
ConclusionBlocking on weight accounting: the new sale-offer extrinsics and dispatch guard need benchmarked/generated weights before this runtime change is safe to merge. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| return Err(Error::<T>::ColdkeyLockedDuringSale.into()); | ||
| } | ||
|
|
||
| if is_sale_frozen_owner_hotkey && !is_mev_protected { |
There was a problem hiding this comment.
[MEDIUM] Seller cannot cancel when coldkey is also owner hotkey
Creating an offer freezes both the seller coldkey and the current owner hotkey. If those are the same account, the earlier coldkey branch allows cancel_sale_offer, but this owner-hotkey branch immediately rejects the same cancellation with HotkeyLockedDuringSale. The existing owner-hotkey setter only rejects system accounts, so this state is reachable and leaves the seller unable to cancel without root intervention. Let cancel_sale_offer pass this branch too; do_cancel_sale_offer still enforces that a signed caller must be the offer seller.
| if is_sale_frozen_owner_hotkey && !is_mev_protected { | |
| if is_sale_frozen_owner_hotkey && !is_sale_cancel && !is_mev_protected { |
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
requested review from
JohnReedV,
evgeny-s,
gztensor,
open-junius and
sam0x17
| return Err(Error::<T>::ColdkeyLockedDuringSale.into()); | ||
| } | ||
|
|
||
| if is_sale_frozen_owner_hotkey && !is_mev_protected { |
There was a problem hiding this comment.
[MEDIUM] Seller cannot cancel when coldkey is also owner hotkey
do_create_sale_offer allows seller == owner_hotkey, then freezes the same account in both maps. For that account, cancel_sale_offer passes the coldkey check but is rejected here as a frozen owner hotkey, leaving the seller unable to use the only non-root cancellation path while both keys remain frozen. Let sale cancellation pass this guard too; do_cancel_sale_offer already enforces that only the seller or root can actually cancel.
| if is_sale_frozen_owner_hotkey && !is_mev_protected { | |
| if is_sale_frozen_owner_hotkey && !is_sale_cancel && !is_mev_protected { |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| fn weight(_call: &CallOf<T>) -> Weight { | ||
| T::DbWeight::get().reads(2) |
There was a problem hiding this comment.
[HIGH] Benchmark the new dispatch extension weight
CheckSubnetSale runs on every runtime dispatch and is also included in SubtensorTransactionExtension::weight, but it charges only T::DbWeight::get().reads(2). The existing subtensor dispatch guards use generated WeightInfo entries with measured ref_time and proof size; this new guard reads two account-keyed storage maps and needs the same treatment. Add a check_subnet_sale_extension benchmark beside the other guard benchmarks, regenerate weights.rs, and return that generated weight here.
| fn create_sale_offer() -> Weight { | ||
| Weight::from_parts(25_000_000, 0) |
There was a problem hiding this comment.
[HIGH] Regenerate weights for the new sale-offer extrinsics
The new create_sale_offer and cancel_sale_offer weights are handwritten constants with proof size 0, while the comments claim measured storage proofs. These extrinsics add/read multiple storage maps, including new unbounded maps, so they need generated benchmark output with measured ref_time and proof-size components. Under-accounted runtime weights can let blocks exceed intended execution/proof limits. Run the pallet benchmarks and replace both SubstrateWeight and () implementations with the generated values.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| .saturating_add(T::DbWeight::get().reads(7_u64)) | ||
| .saturating_add(T::DbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] create_sale_offer undercharges storage I/O
do_create_sale_offer calls get_next_sale_offer_id(), which reads and writes NextSubnetSaleOfferId, before inserting the sale offer and freeze maps. This weight accounts for only 7 reads and 3 writes, omitting that counter access. A user can therefore submit this extrinsic with less declared DB weight than it actually consumes. Regenerate the benchmark weights or account for the extra read/write here.
| .saturating_add(T::DbWeight::get().reads(7_u64)) | |
| .saturating_add(T::DbWeight::get().writes(3_u64)) | |
| .saturating_add(T::DbWeight::get().reads(8_u64)) | |
| .saturating_add(T::DbWeight::get().writes(4_u64)) |
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | ||
| .saturating_add(RocksDbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] Fallback create_sale_offer weight also undercharges storage I/O
The fallback WeightInfo for () implementation has the same undercount: it omits the NextSubnetSaleOfferId read/write performed by get_next_sale_offer_id(). Keep this in sync with the runtime weight so tests and fallback runtimes do not preserve the undercharge.
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(3_u64)) | |
| .saturating_add(RocksDbWeight::get().reads(8_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(4_u64)) |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .saturating_add(T::DbWeight::get().reads(7_u64)) | ||
| .saturating_add(T::DbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] create_sale_offer undercharges storage I/O
do_create_sale_offer now calls get_next_sale_offer_id(), which reads and writes NextSubnetSaleOfferId in addition to the seven reads and three sale-state writes documented here. The worst-case successful path performs 8 reads and 4 writes, so this generated weight still undercharges every successful sale-offer creation. Regenerate the benchmarked weights or at least include the missing counter I/O.
| .saturating_add(T::DbWeight::get().reads(7_u64)) | |
| .saturating_add(T::DbWeight::get().writes(3_u64)) | |
| .saturating_add(T::DbWeight::get().reads(8_u64)) | |
| .saturating_add(T::DbWeight::get().writes(4_u64)) |
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | ||
| .saturating_add(RocksDbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] Fallback create_sale_offer weight also undercharges storage I/O
The fallback WeightInfo for () has the same accounting gap: get_next_sale_offer_id() adds one NextSubnetSaleOfferId read and one write, so the fallback should charge 8 reads and 4 writes for the successful path.
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(3_u64)) | |
| .saturating_add(RocksDbWeight::get().reads(8_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(4_u64)) |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .saturating_add(T::DbWeight::get().reads(7_u64)) | ||
| .saturating_add(T::DbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] create_sale_offer undercharges storage I/O
do_create_sale_offer calls get_next_sale_offer_id(), which reads and writes NextSubnetSaleOfferId on every successful create path. This generated weight still charges only the seven existing reads and three writes, omitting the counter I/O, so the public extrinsic is undercharged. Charge 8 reads and 4 writes here and regenerate the benchmark metadata.
| .saturating_add(T::DbWeight::get().reads(7_u64)) | |
| .saturating_add(T::DbWeight::get().writes(3_u64)) | |
| .saturating_add(T::DbWeight::get().reads(8_u64)) | |
| .saturating_add(T::DbWeight::get().writes(4_u64)) |
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | ||
| .saturating_add(RocksDbWeight::get().writes(3_u64)) |
There was a problem hiding this comment.
[MEDIUM] Fallback create_sale_offer weight also undercharges storage I/O
The fallback WeightInfo for () implementation has the same missing NextSubnetSaleOfferId read/write as the generated implementation. Any runtime using this fallback will undercharge the same public extrinsic. Keep the fallback in sync at 8 reads and 4 writes.
| .saturating_add(RocksDbWeight::get().reads(7_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(3_u64)) | |
| .saturating_add(RocksDbWeight::get().reads(8_u64)) | |
| .saturating_add(RocksDbWeight::get().writes(4_u64)) |
|
🔄 AI review updated — Skeptic: VULNERABLE |
1 participant
Summary
This PR adds subnet sale offers to the Subtensor pallet.
The goal is to let a subnet owner announce that a subnet is for sale, optionally
restricted to a specific buyer coldkey. Once an offer exists, a later crowdloan
lease flow can target that offer, pay the seller, and transform the subnet into a
lease atomically.
Creating an offer stores the sale terms by
netuidand freezes the seller coldkeyand current subnet owner hotkey while the offer is active. This prevents the seller
from mutating the relevant accounts during the sale. The seller or root can cancel
the offer, and sale-offer state is also cleaned up if the subnet is removed.
Sale offer identity
Each offer is assigned a unique, monotonically increasing
SaleOfferId. Offers arekeyed by
netuid, so the id lets a consumer (such as the future crowdloan leaseflow) bind to the exact offer it agreed to: if the seller cancels and recreates the
offer with different terms, the recreated offer carries a new id and the stale
binding no longer matches, rather than being silently satisfied by an offer with
modified terms.
Freeze enforcement
A
CheckSubnetSaledispatch extension rejects frozen sale accounts at dispatch time:Because this is a dispatch extension (not a transaction extension), the check fires
at every
call.dispatch(origin)site, including indirectly dispatched calls (e.g.proxy) with the resolved signer, so the freeze cannot be bypassed by wrapping a call
in another dispatch layer.
To keep the freeze consistent,
swap_hotkeyalso refuses to swap a hotkey that iscurrently frozen by a sale offer, which would otherwise move the subnet owner hotkey
out from under the offer (the owner hotkey can be owned by a coldkey other than the
frozen seller, so the coldkey freeze alone does not cover this).
Same-account owner
When the subnet owner coldkey and owner hotkey are the same account, only the coldkey
is frozen. Freezing the same account in the hotkey map as well would block
cancellation (a coldkey action) and permanently lock the seller out of their own
offer.
Other changes
SubnetSaleOffers,SubnetSaleFrozenColdkeys,SubnetSaleFrozenHotkeys, and theNextSubnetSaleOfferIdcounter.create_sale_offerandcancel_sale_offerextrinsics, with the associatedevents and errors.
CheckSubnetSaledispatchextension.
removal, the same-account case, the
swap_hotkeyinteraction, and thedispatch-extension guard (including proxied and nested-proxy dispatch).
Follow-up
The buyer/finalize side is intentionally not included yet. When it lands,
finalization should re-verify that the subnet owner coldkey and hotkey still match
the values snapshotted in the offer (alongside the
SaleOfferId) as a fail-safeagainst any owner-identity drift during the sale.