Repository navigation
Conversation
CI Summary
Per-section details omitted: combined body exceeded the 65000-char comment limit. Follow the View Report links above for each section's full output. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
GHkrishna
left a comment
There was a problem hiding this comment.
Overall looks good, just a few things:
- All bench users are fresh, so nobody has a LabelStore and the store-write path never runs. In #348 that path was the worst (91% for a mainnet claim). Could we add rows where the user already has a store (run claimLabelStore in setup), plus one where the user leaves the front of another queue while reserving?
- Also, since several rows are expected to fail for now, could we either mark them as known failures, so the job only goes red on new failures or regressions, or send failures somewhere we could keep an eye on? Otherwise it’ll be red every Monday and we might miss em.
Please lmk wdyt??
| function leftEntries(base, count) { | ||
| const users = Array.from({ length: count }, user); | ||
| return { | ||
| join: users.map((u) => reserve(u, base)), | ||
| leave: users.map((u) => reserve(u, label())), |
There was a problem hiding this comment.
If I’m reading leftEntries right, each user is at the front when they leave, so the queue gets cleared as they go and the measured call has nothing left to clear. Could we assert reservationMeta shows head < tail with empty entries before measuring? To actually leave 4 empty entries in front, you could queue a front user, then 7 who leave, then the target, and have the front user leave last. Their exit clears 4 and leaves 4 for the measured call.
There was a problem hiding this comment.
On 1.1.0, leaving through a gateway reservation doesn't clear the old queue: _keepOrDropReservation calls _dropReservation with 0 steps, so the entry is deleted where it is and the head doesn't move. After the last user leaves, all 4 emptied entries are still in front, and the measured call clears them. The figures show it: on 1.1.0 the "behind 4 left entries" rows cost more than the fresh ones (29.5% against 27.1%, and 79.2% against 76.3% on Paseo). On 1.0.0 they cost the same, because 1.0.0 cleared an entry on leave.
| const grantBatch = Number(await read(WHITELIST, "maxGrantBatch")); | ||
| const claimants = Number(await read(WHITELIST, "maxClaimants")); |
There was a problem hiding this comment.
Out of curiosity could the whitelist rows measure at WHITELIST_MAX_ _LIMIT instead of the live value? That’s what the new bounds promise ig, and it would make the build rows green as soon as the code is right, instead of waiting on governance.
There was a problem hiding this comment.
Donein 05fd7d1: with --build, the whitelist rows now run at the build's WHITELIST_MAX_*_LIMIT, read from DotnsConstants
| const label = () => { | ||
| let n = BigInt(keccak256(toHex(`${seed}-${++counter}`))); | ||
| let out = "zq"; | ||
| for (let i = 0; i < 10; i++, n /= 26n) out += String.fromCharCode(97 + Number(n % 26n)); | ||
| return out; | ||
| }; |
There was a problem hiding this comment.
The grant rows use 12-character labels. Since the whitelist stores each label, a 40-name batch of long labels could cost noticeably more. Should the bound be measured with max-length labels, the same way reasons use the 256-byte maximum?
There was a problem hiding this comment.
Good catch, changed in 05fd7d1 to max-length labels. This showed that length 40 names no longer fit (100.3% on Paseo, 101.4% on Polkadot), so the bound is now 36 (90.3% and 91.3%). The claims bound moved from 32 to 30 (90.0% and 90.1%).
There was a problem hiding this comment.
I was wondering what happens when a name has more claims on the live network, would these upper bound still hold?
The cap only stops new requests, so a name with ~30+ existing claims couldn’t be granted in one call. Should we check live claimantCount before, or note that governance would need to reject some first?
There was a problem hiding this comment.
Also can we add respective dev comments where required with reason like so, "one call fits a referendum’s weight limit" or smthg like this??
There was a problem hiding this comment.
Right, the cap only stops new requests, so a name that collected more claims under a higher cap needs governance to reject some first, one per call, before a grant fits. That's now in the setMaxClaimants NatSpec. Paseo has 4 active names; I'll check Polkadot's claim counts before its upgrade.
Each bound also has a @dev note now: one governance call at the bound still fits the weight a referendum can execute, and the benchmark checks it.
…he longest labels, list known failures
Agreed on both, done in 05fd7d1
|
|
Updated the figures in the PR description: all four runs are from today, on this branch after the merge of #349, with Paseo on 1.1.0, the new store-path and leave-front rows, maximum-length labels, the 95% threshold, and the new bounds (36 names, 30 claims). |
Description
Adds a scheduled benchmark of the critical dotNS paths on pallet-revive, and lowers the whitelist caps that it shows don't fit a governance call.
pallet-revive benchmark
Foundry runs the contracts on its own EVM, which doesn't meter weight and proof size the way pallet-revive does, so until now these limits were only checked by hand.
scripts/bench/revive-bench.mjsdry-runs each path as a Root batch against live Asset Hub state at the finalized head, and compares it with the limit it runs under in production, read from the chain:DotnsGateway.MaxContractCallWeight): device-name issuance (first in a container, later in one, with and without a reservation, and for a user who already has a LabelStore), reservations (fresh, behind left entries, and leaving the front of another queue), and personhood claims(fresh, behind left entries, and into an existing LabelStore);claimLabelStoreon first settlement, andexpireReservationon a full queue;grantNamesbatch, andgrantNameclearing the maximum number of claims, with labels at the 63-byte DNS maximum and reasons at the 256-byte maximum.With
--build, every proxy, and the store beacons throughStoreFactory, is first upgraded to this checkout's implementations inside the same dry run, and the whitelist rows run at the build's bounds. Without it, they run at the live caps.A path fails when it doesn't fit its limit, goes over
--fail-at(95% by default), or fails outright. Paths expected to fail for now are listed inKNOWN_FAILURES, each with what clears it: they are reported as known, and the run fails on any other failure, or when a listed path passes.The workflow runs every Monday and on demand, on Paseo Asset Hub Next and Polkadot Asset Hub, against the deployed contracts and against the build. It has read-only permissions and uses no secrets.
Current results (share of the limit, bold above the threshold):
claimLabelStore, first settlementexpireReservation, 64 left entriesgrantNames, live cap / boundgrantNameclearing claims, live cap / boundPaseo has every device container prepared, so it has no first-in-container row.
Whitelist caps
The live caps let governance queue calls no referendum can execute: 100 names in one
grantNamestake about 250% of a governance call's weight, and clearing 64 claims about 190%. The bounds governance could raise them to (256 and 128) were higher still.maxGrantBatchmaxClaimantsFigures are Paseo / Polkadot, with labels at 63 bytes and reasons at 256 bytes. The bounds are the largest values that stay under the threshold.
Type
Scope
Related Issues
Fixes
Closes #287.
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
Breaking changes:
How to test
The workflow can only be dispatched once it is on master; its first run will be after merge.
Notes
The new defaults only apply to fresh deployments, since
initializesets them. On Paseo and Polkadot, governance sets the live values withsetMaxGrantBatch(32)andsetMaxClaimants(24), which the deployed contracts already accept; the new bounds apply after the whitelist is upgraded. The cap only stops new requests, so a name that already holds more claims needs governance to reject some before a grant fits.Known failures today, all listed in
KNOWN_FAILURES:00to99, as on Paseo, removes the row.On 1.0.0, leaving a queue clears the entry straight away, so the "left entries" rows cost the same as a fresh queue there.