Notes from onboarding as an aggregator — and a couple of contract things I'd like a second opinion on #79
Replies: 10 comments 1 reply
|
@Santiagocetran — thanks for going through this cold and writing it up in this much detail, genuinely useful. Went through everything below line by line against the actual code (both Why 12Your batching read is exactly right, and I traced it against The actual reasoning behind 12: GI 1 had 9 clients register and submit, and I'm targeting roughly the same ~9 for GI 2 (I close LM submissions once ~9 have come in). 12 aggregators was sized around that — with 9 approved models, T1 takes 3 batches of 3 (using 9 of the 12), leaving exactly 3 for T2. That's the "8-10 approved models -> T2 yes" row in your table, and 9 is the number I was actually planning around. So the 12 wasn't an arbitrary throughput target, but it also isn't derived from a documented invariant anywhere — it's me holding "~9 clients" and "12 aggregators" in my head as a pair and never writing down why they have to move together. Your point stands: if approved models drift to 11-12, T2 silently gets skipped with no error, and it'd be easy for someone running this without that context to hit the cliff and not notice. Agree it's worth an explicit check in dincli rather than relying on the two numbers being remembered in sync. Does the T1/T2 batching arithmetic match intent, or are you missing something? Answered above — your reading of the code is correct, and the 12 was intentional (sized to ~9 expected clients), just never written down anywhere as an explicit invariant. GI1 -> GI2 overlapNot churn — GI 1's 12 aggregators were all accounts I operate myself, so there was never any external roster to retain. GI 2 is the first round with real outside participation: 4 external aggregators registered plus you as the 5th. So you may well be the first external aggregator to actually go through this pipeline — which is a big part of why your notes here are useful. The slashing bugConfirmed, and you've got it right.
Given this touches what's live, we're not patching it in isolation, and it's not just "accumulate fixes and redeploy" — that undersells the actual cost. There are two real paths, and neither is free:
So this isn't a settled plan yet, and shouldn't be rushed — we need to actually decide between these (or find a third option) rather than treat it as a simple accumulate-and-redeploy. Until it's resolved, the practical mitigation is making sure GI 2's batch of 3 doesn't hit the failure path — so if you know who's in your batch, worth a direct nudge to make sure submissions land clean. Issue #37 — aggregator/auditor
|
|
@umeradl thanks for going through all of it line by line — the corrections were useful, particularly the First PR is up: #80 — docs plus CLI help text, based on It covers the docs bundle from your list: the walkthrough as the primary sequence, bridge funding with the gas gotcha and the Two things in there I'd like your call on, both flagged at the top of the PR:
Two new bugs found while writing it:
One correction to your scoping: the And one refinement on the chain-id check: I'd put it in One last thought, not PR-related No urgency on this, but flagging it while it's fresh. Whenever you get to settling the pinning question, it'd be really useful to have a version string posted here that the GI 2 aggregators can match — even a provisional one. I'm on 3.13.5 and the docs said 3.12.3, so there's a fair chance the five of us are on slightly different stacks at the moment. Your point about GI 1 having no environment diversity is what makes me think it's worth doing before T1 rather than after: it's less that something's broken and more that this path has genuinely never been exercised. And if someone does diverge, the effect right now is that the GI can't be closed rather than anyone losing stake — so a bit of coordination up front saves everyone the headache. Totally happy to rebuild my node to whatever you land on, and to help chase the other four if that's useful. It also doesn't need to wait on the redeploy decision, so it's an easy one to knock out independently if you want a quick win. Next up: the whitelist one-liner, then the backports (#37 + the Python floor), UX polish, chain-id, and the IPFS GET fix. I'll keep them as separate PRs unless you'd rather they were bundled — just say the word. |
|
#82 is up — the whitelist one-liner, based on That's the " +1/−1, verified against an isolated config with no wallet. One note in the PR body: the |
|
#83 is up — the chain-id check, based on That's the "chain-id check ( Two notes on it. I put the check in Verified live against
|
|
#84 is up — the IPFS retrieval fix, based on That's the "IPFS POST-vs-GET fix (needed on both branches)" item from your "needs more care" list, plus the One correction to the diagnosis, though. We both read this as POST-vs-GET and you confirmed it line by line — but that's not what produces the error I reported. Two things worth your attention in the PR:
Also fixed two things found while implementing: CIDs weren't validated or fully escaped before going into a URL, and a mid-stream download failure left a partial file in the destination — which callers treat as a cache hit, so a truncated model would be silently reused. Downloads are atomic now. The That's four of the items from your list now: docs (#80), whitelist (#82), chain-id (#83), IPFS (#84). Still to come: the |
|
#85 is up — the bridge command, based on That's part 2 of your funding ask: "a
That closes every PR-able item on your list except the four small CLI ones — |
File map — PRs #80, #82–#86 (all
|
| PR | Title | Head SHA | Files touched |
|---|---|---|---|
| #80 | docs: fold tested onboarding walkthrough into participant docs | 470a62f |
5 |
| #82 | fix(cli): repair the system callback skip-list (configure-ipfs missing, two dead entries) | c5ed47c |
1 |
| #83 | fix(cli): validate the RPC chain id on every connection | 9a3f855 |
4 |
| #84 | fix(cli): correct IPFS retrieval endpoint handling; opt-in public gateway fallback | 205ae02 |
4 |
| #85 | feat(cli): add dincli system bridge-eth — fund OP Sepolia from Ethereum Sepolia L1 |
1562534 |
3 |
| #86 | fix(cli): honor stake amount, retry buy balance, quiet password lookups | 0bcf945 |
6 |
Files unique to a single PR
| File | PR | Lines |
|---|---|---|
Documentation/GettingStarted.md |
#80 | +229/-192 |
Documentation/Model-workflow.md |
#80 | +11/-4 |
Documentation/common.md |
#80 | +8/-2 |
Documentation/setup.md |
#80 | +307/-51 |
dincli/cli/core.py |
#83 | +19/-0 |
dincli/config/din_info.json |
#83 | +2/-0 |
tests/test_chain_id_validation.py |
#83 | +280/-0 |
dincli/services/cid_utils.py |
#84 | +17/-1 |
dincli/services/ipfs.py |
#84 | +180/-30 |
tests/test_ipfs_retrieval.py |
#84 | +570/-0 |
dincli/services/bridge.py |
#85 | +423/-0 |
tests/test_bridge_eth.py |
#85 | +675/-0 |
dincli/cli/aggregator.py |
#86 | +23/-7 |
dincli/cli/auditor.py |
#86 | +23/-7 |
pyproject.toml |
#86 | +1/-1 |
tests/test_cli_small_fixes.py |
#86 | +419/-0 |
Files touched by more than one PR
| File | PRs | What each does | Merge result (tested) |
|---|---|---|---|
dincli/cli/system.py |
#80, #82, #85, #86 | #80: connect-wallet help text only (lines ~165-190). #82: fixes the subcommand skip-list — read_wallet→read-wallet, show_index→show-index, adds missing configure-ipfs (line ~56). #85: adds bridge-eth command (+150/-1) and appends "bridge-eth" to that same skip-list line (line ~54). #86: configure-demo default flips to "no" (line ~126), a demo_key_is_public flag around connect-wallet (lines ~206-213), and the demo-save warning text (lines ~268-271). |
#80/#82/#85 unchanged from before. #86 stays clear of the contested skip-list line — verified by stacking it on top of an already-reconciled #82+#85 merge (union skip-list committed, then #86 applied): clean, no new conflicts, on both system.py and utils.py. Also verified clean standalone against current main (which already carries #82/#83/#84) and against #80. |
dincli/cli/utils.py |
#83, #84, #86 | #83: chain-id validation, resolve_network_value (line ~195). #84: IPFS retrieval helper, resolve_ipfs_config (line ~88). #86: new read_after_write retry helper plus password-lookup fixes (lines ~6, ~301, ~330, ~578-580). |
Clean — three disjoint regions, no line overlap. Verified: sequential merge of #83 then #84 then #86 completes with git's ort strategy, no conflicts. |
Bottom line: #80, #83, #84, #86 are all independent of each other and merge in any order without incident — including #86, confirmed clean whether stacked before or after the #82/#85 reconciliation below. #82 and #85 are still the one pairing that needs a human to reconcile dincli/cli/system.py's skip-list by hand — whichever of the two merges second should not just accept git's auto-merge, since it can silently drop the other's fix. That reconciliation is unaffected by #86, which edits different lines of the same file.
|
This file map is really useful — thank you for building it. Having the overlap laid out per-file is much easier to act on than five PRs to hold in your head at once, and you spotted the #86 is up — the last four small CLI items ( Good news for your sequencing: #86 stays clear of the contested line. Its One friendly correction on the silent-drop, because I think it's better news than it looks. I couldn't reproduce it — git raised
I think it comes down to the So your conclusion stands — a human does need to combine that line — but the merger can trust git to stop them. Worth knowing, since "don't trust auto-merge here" would make every future merge in this repo slower than it needs to be. The union line, if it's handy to have ready: if ctx.invoked_subcommand in ["connect-wallet", "init", "welcome", "where", "configure-network",
"configure-demo", "configure-ipfs", "read-wallet", "show-index", "din-info",
"configure-logging", "dump-abi", "reset-all", "todo", "dataset", "bridge-eth"]:#82's corrections plus #85's entry. One thing worth your eye before 0.2.0, in #86's description: the Python floor may need to be 3.12, not 3.10. No rush on any of it — happy to take the #82/#85 reconciliation myself if that's easier than doing it at merge time, just say which one you'd rather keep as the base. |
Status update — local
|
| PR | Merged into local main? |
Review comment | Merge-proposal comment | Notes |
|---|---|---|---|---|
| #80 | ✅ Yes | link | link | Fast-forwarded cleanly, byte-identical. |
| #82 | ✅ Yes | link | link | Clean, no conflicts. |
| #83 | ✅ Yes | link | link | Clean, no conflicts. click dependency gap flagged in the review comment — now fixed, see below. |
| #84 | ✅ Yes | link | link | Clean, no conflicts. |
| #85 | ✅ Yes | link | link | dincli/cli/system.py's skip-list conflict with #82 resolved by hand using the union line you posted — configure-ipfs/read-wallet/show-index (#82's corrections) plus bridge-eth (#85's addition), all in one list. Verified: no leftover conflict markers, ast.parse clean. |
| #86 | ✅ Yes | link | link | Clean, no conflicts — confirmed its system.py hunks stay clear of the skip-list, as you predicted. Running all four PRs' test suites together against the merged tree showed the #83 click gap also blocking collection of test_cli_small_fixes.py and test_bridge_eth.py (both use CliRunner against the real app) — same root cause, wider blast radius than #83's own 2 failures. Now fixed, see below. |
click dependency — fixed and verified
Added "click>=8.1.0" to pyproject.toml's dependencies, committed on top of the merge sequence (42f19ca — "fix(cli): declare click as an explicit dependency"). To actually verify rather than just declare it, installed click into the torchenv venv (the one that was missing it — typer 0.27 there no longer pulls click in transitively) and re-ran everything:
tests/test_chain_id_validation.py .......... [11] ← includes the 2 that were failing
tests/test_ipfs_retrieval.py .......... [54]
tests/test_bridge_eth.py .............. [38] ← previously failed to even collect
tests/test_cli_small_fixes.py ........ [28] ← previously failed to even collect
131 passed, 0 failed
Local main is now 12 commits ahead of origin/main (e83c589 → 42f19ca), nothing pushed yet. All six PRs merged, click fix applied and tested — no open items left before a push, pending your go-ahead.
Deviation proposals — all 18 files touched across #80, #82–#86Full file list on local
Line counts are Local Placeholder for now, per Umer — will fill in per-file as each deviation gets decided. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
We only had 4 aggregators registered for GI 2 and wanted 12, so I figured the most
useful thing I could do was actually go through the onboarding myself and see how it goes. I set one up
on a clean headless box, following
setup.mdandGettingStarted.mdonmainthe way a newparticipant would. It worked — I'm registered as aggregator #5 — but it took me a couple of hours
rather than the 10–15 minutes the docs suggest, and I wrote down everything that tripped me up on the
way.
Then I got curious about why 12 specifically, went and read
DINTaskCoordinator, and ended up findinga couple of things in the deployed contracts that I don't think are intentional. That's the last half
of this post. If you only have time for one bit, jump to "the slashing code" — I think it can wedge
GI 2, and I'd really like someone who knows these contracts to tell me I'm wrong.
Fair warning that I'm new to this part of the codebase, so some of this might be me missing context
rather than an actual problem. Happy to be corrected on any of it.
Setting it up: what tripped me up
Two of these I genuinely couldn't get past on my own, so I'll start there.
Funding a fresh address is basically impossible now
Every faucet in
GettingStarted.mdhas grown a gate since the docs were written. Chainlink andconsole.optimism.ioboth want you to connect MetaMask or WalletConnect (a couple asked for mobileverification). Alchemy doesn't need a wallet connection but wants ≥0.001 ETH on Ethereum Mainnet plus
some real transaction history. A key you generated on a server five minutes ago has none of that, and
there's no browser on the box to connect anything with anyway.
I got around it by funding on Ethereum Sepolia L1, where the faucets are much less picky, and then
bridging. That turns out to be easy: OP's
L1StandardBridge.receive()takes plain ETH from an EOA andcredits the same address on L2, so it's just a signed transfer, no ABI encoding, and you can script it
with the
web3thatdinclialready pulls in. The bridge on Sepolia L1 is0xFBb0621E0B23b5478B630BD55a5f21f67730B0F1, and my 0.03 ETH showed up on L2 about 75 seconds later.One gotcha if anyone copies this: the deposit burned 616,165 gas, so if you set a 21,000 limit thinking
it's a normal transfer it'll just fail. And don't bother checking the address via
OTHER_BRIDGE()likeI first did — every OP-Stack chain returns the same
0x4200…0010predeploy, so it doesn't actuallytell you you're on the right one.
Might be worth putting the bridge route in the docs, since I suspect anyone running a headless node
will hit the same wall.
You need a paid IPFS account just to read public data
dincli task gi show-state 0fails outright if you haven't configured a provider yet:Which surprised me, because the manifest is public —
GET https://ipfs.io/ipfs/<cid>returns ithappily. The problem is that the default provider (
ipfs node,services/ipfs.py:69) sends a POSTin path-style form, and every gateway I tried rejects that (ipfs.io and trustless-gateway 405, pinata
500, dweb.link 301) while the GET works fine.
What made me think this is just a bug rather than a design decision: the Filebase branch a few lines
down at
:177already uses kubo's correctcat?arg=<cid>form. The two branches in the same functiondisagree with each other.
The documented escape hatch doesn't work either —
--provider customneedsipfs_service_pathin theconfig, but nothing in the CLI ever writes that value, only reads it (
:125,:183). So it alwaysthrows unless you hand-edit
config.json.To be clear, an aggregator does genuinely need a real upload-capable provider — you can't submit
aggregation results to a read-only gateway. So Filebase is on the critical path regardless. It just
feels wrong that you need an account to read public state.
The ones that fail quietly
The CLI never checks the RPC is on the right chain. I swapped in a dedicated Infura endpoint and
typed
optimism-mainnetinstead ofoptimism-sepolia. One word.dincliaccepted it without a murmur,and the only symptom was
ETH Balance: 0— which is exactly what you expect to see before you'vefunded anything, so I spent a while hunting the wrong problem.
din-infodoesn't help either, since itprints addresses from the local JSON rather than reading the chain. We can't currently check this even
if we wanted to, because
din_info.jsondoesn't carry a chain id for any network. Adding one andfailing loudly on mismatch is about five lines, and of everything in this post it's the change I'd
most like to see.
configure-demodefaults to--mode yes.setup.mddoes tell you to pass--mode no, twice, anddincli system initleaves demo mode off — so the docs and the default are both fine, credit whereit's due. But if you run the bare command just to see what it does (which I nearly did), you've now
switched it on, and from then on
connect-wallet --account 0quietly loads a Hardhat dev key whoseprivate key is public and writes the wallet to disk in plaintext. Nothing warns you at that point.
Defaulting the flag to
no, or warning whenconnect-walletresolves a demo key, would close it.Bare
connect-walletis a trap if you already have a key. Without--accountit prompts you topaste a private key, and the docstring labels that form "(Recommended)". If you've already put a key in
.envand you paste something else at that prompt, you get a keystore for a completely differentaddress than the one you funded, and the only symptom is a zero balance later on. I got as far as the
password step before I realised.
Nothing says Ethereum Sepolia ETH won't work. The contracts are on Optimism Sepolia and the two
faucet URLs differ by one path segment. I sent 0.05 ETH to the wrong layer on my first try. A bold line
in the faucet section would have saved me a cooldown.
Smaller stuff
None of these are serious on their own, but they all cost me time, and they're mostly one-liners:
GettingStarted.mdbuy 0.00001mints exactly 10 DIN, which is exactlyMIN_STAKE— no margin at all0.0001(~100 DIN)cli/system.py:56configure-ipfsrefuses to run without a connected wallet it never uses. Same list has"read_wallet"/"show_index"with underscores, but Typer registers them with dashes, so both entries do nothing"configure-ipfs", fix the two dead entriescli/utils.pyDIN_WALLET_PASSWORDprints a red ❌ that looks fatal — it isn't, it just prompts you insteadcli/aggregator.py:28buyprintedbalance: 0straight after a successful 100 DIN purchase (read-after-write against a load-balanced RPC, not a units bug). Easy to conclude it failed and buy twicesetup.md--api-key <your_api_key>—<is shell redirection, so pasting it verbatim dies with a confusing "no such file" errorYOUR_API_KEYwithout bracketssetup.mddinclicli/utils.py:118.envis read from the current working directory, which isn't documented anywhere. Runaggregate-t1from the wrong folder and you silently submit nothingpyproject.toml:10>=3.9, butpy-cidneeds>=3.10, so a 3.9 host installs and then breaksfix/pyproject-python-floor) that hasn't landedcli/system.py:56read-walletneeds a working RPC just to show you your own address (knock-on from the dead entry above)The order that actually worked for me, if it's useful to anyone
Why 12, and what I found reading the contracts
Everything below I checked against the live devnet with
eth_callrather than just reading source —chain
11155420, coordinator0x6Ce502a2…753d, stake0x5088D7a5…139a, auditor0xA09D8CA6…66e9.All reads, nothing signed. I should say upfront that I haven't run an actual T1 or T2 aggregation yet,
since GI 2 is still in registration, so none of this is observed in a live round.
The number 12 turns out to be load-bearing, but not in the way I assumed
I'd assumed 12 was a throughput target. Reading
autoCreateTier1AndTier2, it looks more like acorrectness threshold. T1 batches take aggregators in exact groups of 3, and the T2 batch is only
created if at least 3 are still left over afterwards. So with the 5 we have now, one T1 batch takes 3,
2 remain, and no T2 batch gets created at all — and T2 is the step that combines the T1 outputs into
the new global model.
The part I didn't expect is that T1 fills greedily first, so whether T2 gets anything depends on how
many client models were approved, not just on how many aggregators registered:
Those last two rows are the ones that worry me a bit — recruiting more clients could break T2 at a
fixed aggregator count, so the two numbers need to move together. If I'm reading it right, 6 is the
real minimum for a GI that can produce a global model, and 12 only works while approved models stay
around 8–10.
What makes this hard to spot is that nothing complains. When there are no T2 batches,
finalizeT2Aggregationjust loops zero times and moves the state along. Every transition succeeds andthe GI completes, it simply never produces a new global model.
For what it's worth, GI 1 fits this exactly — it ran with 12 aggregators and produced 3 T1 batches plus
1 T2 batch, all finalized. So the arithmetic has played out once already.
I'd like someone who knows the intent here to confirm I've got this right, and if so, maybe it's worth
an explicit check in
dinclirather than leaving it to arithmetic.The slashing code
This is the bit I'd most like a second pair of eyes on.
The coordinator has its own
minStakefield, separate from the one on the stake contract, and it's thevalue it passes when slashing:
But
DinValidatorStake.slash()starts withif (amount < MIN_STAKE) revert InsufficientStake();, andMIN_STAKEthere is10 * 1e18. So the amount being passed is about twelve orders of magnitude belowthe floor, and the call can't ever go through. I checked it against the deployed contracts rather than
trusting my reading:
The obvious consequence is that no aggregator can actually be slashed right now. The one that worries
me more is that
slashAggregatorsis the only way into theAggregatorsSlashedstate, andendGIrequires exactly that state. So if a single aggregator in any batch misses their submission or produces
a CID that doesn't match the other two, that call reverts every time it's tried and the GI can't be
ended at all.
I looked for a way to fix it without a redeploy and couldn't find one —
minStakehas no setter, andthe coordinator isn't behind a proxy (both EIP-1967 slots read zero).
The good news is that
developalready has this right: it usesdinvalidatorStakeContract.minStake()and the newer
slash()clamps to available stake instead of reverting. So it looks like the devnet isjust running an older build rather than anything needing to be designed.
Worth saying why nobody's hit this yet: GI 1 must have completed, since
startGIonly acceptsGIendedandGI()now reads 2. Which means all 12 of GI 1's aggregators submitted matching CIDs. Soit's not that the code works, it's that nobody has missed a submission yet. With 5 registered for GI 2
we'd get one batch of 3, and all three would need to be clean again.
While I was in there I noticed the auditor side has a different problem —
slashAuditorson thecoordinator is a stub with a
// The Actual Slashing logic maybe implemented herecomment, and thedeployed
DINTaskAuditordoesn't have aslashAuditorsfunction in its bytecode at all (I checked theselector). It is registered as an authorised slasher on the stake contract, though.
develophas thereal implementation. Not urgent in the way the above is, but it does mean auditors currently have no
economic accountability either, which feels worth knowing if we're describing it to people as a staked
role.
One other thing I noticed
I pulled GI 1's aggregator list to check the batching arithmetic, and the roster surprised me:
Not one of GI 1's twelve came back for GI 2. I genuinely don't know what to make of that — if those
were team accounts spun up to exercise the pipeline then it's not churn at all and I'm reading too much
into it. But if a decent number were external, then we've had 12 before and lost them, and recruiting
eight more gets us back to where we already were.
I'm not sure the onboarding friction above explains it either, since it's about why people don't get
started, not why people who already managed it don't come back. One thought: re-registration is a
per-GI thing rather than one-time setup, so if nobody's being pinged when a new registration window
opens, that alone might be most of it.
What I can help with
Happy to open a PR for the doc fixes and the small CLI ones — the
configure-ipfswhitelist, the<...>placeholders, the venv note, the.envcwd behaviour, the Python floor, plus theGettingStarted.mdchanges for demo mode, the L1/L2 warning and the buy amount. I'd also like to addthe chain-id check if there are no objections, since it's small and it turns a silent failure into a
startup error.
The contract stuff I'd rather talk through than PR, since it touches what's deployed. Mainly:
slashAggregatorsreading right, and if so is it worth redeploying the task contracts fromdevelopbefore the T1 window? If not, it's probably worth making sure the three aggregators who endup in GI 2's batch are people we can reach directly.
it. Related: what Python and torch versions is everyone else running? I'm on 3.13.5 and
setup.mdsays 3.12.3, and a numerics difference would show up as a mismatched CID.
I kept longer notes with every file and line reference while I was going through this, so if any of the
above is worth digging into properly I can share those.
@umeradl @abrahamnash @robertocarlous
All reactions