feat(evm-module): OT-RFC-51 Publishing Allocation — committed-budget node scoring#1195
Merged
Conversation
…ocation - PublishingConvictionStorage: Account gains primaryNode + lastPrimaryNodeChangeEpoch, setPrimaryNodeData setter (10.0.2->10.0.3) - EpochStorage: rename producedKnowledgeValue -> publishingAllocation everywhere, add net-zero moveEpochPublishingAllocation (10.0.3->10.0.4) - RandomSampling: P(t) = K_n/K_total over single current epoch (10.0.4->10.0.5) - KnowledgeAssetsLifecycle: stop crediting K_n on realized publish/delta (10.0.5->10.0.6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 61e18bd4e467d6a46dfaaac0b038f32ac62fdfea)
… + NFT + test - PublishingConviction: add ShardingTableStorage dep; createAccount gains primaryNode (validated + prorate-seeded); setPrimaryNode moves future epochs net-zero on K_total via shared _perEpochAmount schedule anchored to stored fields; bump 10.0.2->10.0.3 - DKGPublishingConvictionNFT: createAccount forwards primaryNode, add setPrimaryNode owner-gated wrapper, widen accounts() tuple; bump 10.0.2->10.0.3 - IDKGPublishingConvictionNFT: widen accounts() tuple for the two new fields - deploy 052b: add ShardingTableStorage dependency - test: focused happy-path covering seed sum == committedTRAC, net-zero future-epoch move, and realized-publish-off (no K_n credit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 1a656ee5aedf8647fa4d6bf4e6ece50b55fb5e6c)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 11e90699bc28d94f9c21aca899e52691824aab4b)
… block) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit ee979b9ddbbc4de9a647a4c71bb323c0f07588a4)
…ields The two PCA-eligibility reads in publish()/update() destructure the DKGPublishingConvictionNFT.accounts() tuple positionally; add the two trailing slots for primaryNode + lastPrimaryNodeChangeEpoch (OT-RFC-51). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 12f9ee95a3119ebf2adaf54e082361422b035543)
…nsumers Mechanical producedKnowledgeValue -> publishingAllocation rename in the non-archive TS tests that call the renamed EpochStorage getters, so they typecheck against the regenerated typechain. NOTE: Profile.test.ts and RandomSampling.test.ts also assert the OLD realized-publishing-credits behavior (now removed by RFC-51), so they will fail SEMANTICALLY at runtime even though they now typecheck -- out of scope for this first pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 2316f2394f09f67b133e676925c10ebde292528f)
…ove loops
PublishingConviction.createAccount and setPrimaryNode previously called
_perEpochAmount(acct, e) inside their per-epoch loops, and _perEpochAmount
recomputed PublishingMathLib.prorateActiveSink + two chronos reads
(timestampForEpoch, epochLength) on EVERY iteration -- O(lockDurationEpochs)
redundant work for a result that is loop-invariant (it depends only on the
stored Account, never on the per-epoch cursor e).
Split into:
- _scheduleFor(acct): computes timeRemainingInCurrentEpoch from STORED
fields + calls prorateActiveSink ONCE; returns the ActiveSinkRange[3].
- _amountForEpoch(ranges, committedTRAC, epochs, e): PURE per-epoch reader
over the precomputed schedule (no external reads).
Both createAccount and setPrimaryNode now compute the schedule once and index
each epoch off it. Single source of truth, so seed and move can never drift.
Behavior-preserving: per-epoch amounts are BYTE-IDENTICAL to before
(partial-first = ranges[0].tokenAmount, full = committedTRAC/epochs, final =
ranges[2].tokenAmount), so moveEpochPublishingAllocation still subtracts
exactly what was seeded and K_total stays net-zero. Same ABI, same version
(10.0.3). Also commits the RFC-51 ABI regenerations that the prior pass left
stale in the working tree.
Verified: test/v10-rfc-51-publishing-allocation.test.ts passes (seed-total ==
committedTRAC, net-zero future-epoch move, realized-publish-off).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 42267c91f77cc398663789585962693012f2c763)
…ation semantics
OT-RFC-51 re-based the publishing factor P(t) from realized publishing to
committed publishing allocation and narrowed its window from 4 epochs to 1
(current epoch only). The two integration suites were renamed to compile but
still encoded the OLD behavior.
RandomSampling.test.ts:
- calculateExpectedNodeScore: collapse the 4-epoch P(t) sum
([currentEpoch-3 .. currentEpoch]) to a single currentEpoch read, mirroring
RandomSampling._calculateNodeScore (which now reads one bucket). Update the
doc comment for the 4->1 window + realized->allocation input swap.
- "Should demonstrate publishing factor impact on score": a realized publish
no longer credits K_n, so feed allocation via the renamed accumulator
(addEpochPublishingAllocation, the same write path PublishingConviction uses
to seed a PCA primaryNode) and assert the current-epoch P(t) lifts the score
— instead of creating a KA and expecting K_n > 0.
- "max publishing value is zero": fix the contradictory comment (it asserts 0
and now explains why under allocation semantics).
- The suite stays describe.skip: it is a pre-existing V8-stake-pipeline
tombstone (scoring reads CSS, not StakingStorage), unrelated to RFC-51;
un-skipping needs a fresh CSS-primitive harness per the in-tree note.
Profile.test.ts:
- ensureNodeHasChunksThisEpoch: getNodeCurrentEpochPublishingAllocation is no
longer a valid "did this node publish this epoch?" probe (realized publish
doesn't credit allocation anymore); probe the node's current-epoch challenge
state directly instead.
- Raise the per-suite Mocha timeout to 600000 on both active top-level
describes (node config has no mocha block; the full-stack deploy fixture
exceeds the 40s default under load — the baseline 4 failures were all this
timeout cascading into the deploy fixture, not RFC-51 semantics).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 0214537bbbbb36d3a313332a2e9bbeb574d6146a)
…on rename Mechanical ABI regeneration matching the already-committed EpochStorage.sol (commit 61e18bd4e): producedKnowledgeValue -> publishingAllocation across the event, mappings and getters, plus the new net-zero moveEpochPublishingAllocation mutator. The prior RFC-51 pass committed the contract but left abi/EpochStorage.json stale; this brings the generated artifact in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit db283cc72ba193a314705c84c569818459a348e3)
…e) signature OT-RFC-51 added a `primaryNode` arg to DKGPublishingConvictionNFT.createAccount, which broke the e2e flow with ethers' "no matching fragment" on the old 1-arg call. Pass the already-staked publishing node as the PCA's primaryNode (so the nodeExists gate passes and the committed TRAC is prorate-seeded as that node's publishing allocation). Also raise the per-suite Mocha timeout to 600000 (node config has no mocha block; the full-stack deploy + publish exceeds the 40s default). Verified: test/v10-e2e-conviction.test.ts passes (1/1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 91fcda063b1f02116ad75571fdc236b1250fbd74)
…Score
The one RFC-51 behaviour with no RUNNING test was the node-score impact of
allocation-based P(t): the V8-flow RandomSampling integration suite is
describe.skip-ped (CSS-incompatible tombstone). Extend the focused RFC-51
suite (its loadFixture deploys the full V10 stack) by also deploying
RandomSampling — which transitively pulls in ConvictionStakingStorage,
ProfileStorage, AskStorage and ParametersStorage — and add three running
cases against the live scoring path:
(d.1) two equal-effective-stake nodes, 3:1 seeded current-epoch
allocation => calculateNodeScore(A) > calculateNodeScore(B); each
live score is byte-identical to a JS mirror of _calculateNodeScore;
P(t)=0.75/0.25, inner = c + 0.86*P, score ratio ~2.98 (< 3:1,
proving the baseline c shapes it).
(d.2) equal-stake node with ZERO allocation but K_total>0 (real divisor)
=> P(t)=0, inner == baseline c=0.002e18, score < 1% of an allocated
peer.
(d.3) PCA createAccount->setPrimaryNode moves future-epoch allocation
A->B; after advancing one epoch the live calculateNodeScore favors
nodeB (effective stake still equal), proving the move shifts the
score, not just the accumulator.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit ba7c63ad6df371f29a8efbc6e3a1a56d19923655)
…ctor The pre-testnet review found setPrimaryNode(accountId, 0) reachable through the NFT wrapper (no zero-guard). With newNode==0 the move loop decremented neither node, yet primaryNode was set to 0 while the old node kept its seeded future buckets; a later re-designation then took the add-branch and double-credited both K_n and K_total, which has no decrement path — permanent, pumpable score-divisor inflation at no extra TRAC cost. There is no "clear designation" semantic in OT-RFC-51 (primaryNode==0 means never-seeded), so reject newNode==0: revert ZeroPrimaryNode in PublishingConviction.setPrimaryNode (and remove the now-dead clear branch + false "wrapper never forwards 0" comment), plus a defense-in-depth guard in DKGPublishingConvictionNFT.setPrimaryNode. The legitimate createAccount(0) -> setPrimaryNode(C) late-designation path (oldNode==0, newNode!=0, add-branch) is unaffected. Compiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 717b83ba0cefa076cb259a7ab6995ab67cb247c6)
OT-RFC-51 nit: addEpochPublishingAllocation still maintained the per-epoch node max (epochNodeMaxPublishingAllocation) via a compare + SSTORE, but nothing reads it for scoring — RandomSampling reads only getNodeEpochPublishingAllocation / getEpochPublishingAllocation. The PCA seed loop calls addEpochPublishingAllocation up to lockDurationEpochs (<=60) times per createAccount, so the max upkeep was pure wasted gas on the hot path. Remove the compare/SSTORE only. The epochNodeMaxPublishingAllocation mapping and its getters (getEpochNodeMax* / getCurrentEpochNodeMax* / getPreviousEpochNodeMax*) are KEPT for storage-layout + ABI stability; they now read 0 unless some other contract writes them (none currently does), so the external ABI is unchanged. Update the three EpochStorage unit-test cases that asserted max-on-add to assert the getter now reads 0 (deliberate behavior change), and bring the stale version assertion to 10.0.4 to match the contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit fd4c90d8ec83cfb027f97243c1f9e673d5a371f0)
…rface The publishingAllocation rename (producedKnowledgeValue -> publishingAllocation) removed the old EpochStorage getters, but several out-of-tree suites still referenced them via inline ABI fragments + call sites, so they would bind to a non-existent function on the live RFC-51 contract surface. Rename in the fragments and call sites: getNodeEpochProducedKnowledgeValue -> getNodeEpochPublishingAllocation getEpochProducedKnowledgeValue -> getEpochPublishingAllocation Files: devnet/v10-stress, devnet/v10-end-to-end, devnet/agent-provenance automated suites; packages/publisher agent-provenance-e2e; and the archived Staking.shared helper (tombstoned V8 flow, but it referenced the exact removed names — renamed for type-correctness, semantics unchanged). A repo-wide grep confirms no other references to the removed getter names remain. These live-devnet suites are NOT run here; references only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 74a20df6bcacf3038a5c7b1459f3d76a2f65016a)
…maryNode The blocker fix (717b83ba0) added the ZeroPrimaryNode custom error to both PublishingConviction and DKGPublishingConvictionNFT but did not regenerate the committed ABIs. Re-export via the default hardhat config's hardhat-abi-exporter (npx hardhat compile — the abiExporter is wired on hardhat.config.ts with clear+runOnCompile, so the whole dir is re-exported, NOT the node-config clearing compile). A full clean recompile + re-export of all 98 contracts diffs the committed tree in EXACTLY these two abi files — confirming the other RFC-51-touched ABIs (EpochStorage, RandomSampling, KnowledgeAssetsLifecycle, PublishingConvictionStorage, IDKGPublishingConvictionNFT) were already byte-current from earlier RFC-51 commits (EpochStorage via db283cc72; the Task-3 edit removed only an internal SSTORE, leaving the ABI unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 44439c356d3610b7f7b26f4b230c1251c71008bc)
…ess-control, per-epoch proration
Closes the should-fix test gaps from the pre-testnet review on top of the
blocker fix (717b83ba0).
Task 1 (verifies the fix):
- NFT.setPrimaryNode(_, 0) reverts ZeroPrimaryNode (wrapper guard).
- A direct PublishingConviction.setPrimaryNode(_, 0) from the NFT context
reverts ZeroPrimaryNode (logic-contract guard), reached by impersonating
the Hub-registered NFT AFTER advancing one epoch (the rate-limit check
precedes the newNode==0 check, so the advance is required to reach it).
- The legitimate createAccount(0) -> setPrimaryNode(realNode) late
designation (oldNode==0 add-branch) still seeds future epochs correctly,
asserted against the canonical stored-field schedule.
Task 2:
(a) UPDATE path realized-off: KAV10.update() with deltaTokenAmount>0 and
publisherNodeIdentityId=node1 leaves every per-node allocation
byte-identical (mirrors the publish (c) case for the update path; the
removed credit lived in _executeUpdateCore's value-delta section).
(b) Access-control reverts (revertedWithCustomError): second setPrimaryNode
same epoch -> PrimaryNodeChangeRateLimited; non-existent node on
setPrimaryNode/createAccount -> PrimaryNodeNotInShardingTable; direct
non-NFT createAccount/setPrimaryNode -> OnlyConvictionNFT; NFT
setPrimaryNode from non-owner -> NotAccountOwner; EpochStorage
moveEpochPublishingAllocation from a non-owner EOA -> UnauthorizedAccess.
(c) Per-epoch proration asserted INDIVIDUALLY per seeded epoch (partial-
first / each-full==committedTRAC/N / dust-corrected-final) for BOTH a
boundary-aligned and a mid-epoch mint, via a JS mirror of
_scheduleFor + _amountForEpoch — so a total-conserving mis-distribution
can no longer pass (previously only the lifetime sum was checked).
All 9 focused tests pass (npx hardhat test ... --config hardhat.node.config.ts).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 05b1e19201702fefb2e221cf609c934324281200)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+432
to
+478
| function setPrimaryNode(uint256 accountId, uint72 newNode) external onlyConvictionNFT { | ||
| PublishingConvictionStorage.Account memory acct = | ||
| publishingConvictionStorage.getAccount(accountId); | ||
|
|
||
| uint40 currentEpoch = uint40(chronos.getCurrentEpoch()); | ||
| if (currentEpoch <= acct.lastPrimaryNodeChangeEpoch) { | ||
| revert PrimaryNodeChangeRateLimited(accountId, acct.lastPrimaryNodeChangeEpoch); | ||
| } | ||
| // OT-RFC-51: no "clear designation" semantic. Allowing newNode == 0 | ||
| // would null the stored primaryNode while leaving the old node's | ||
| // future buckets seeded (the move loop credits/decrements neither when | ||
| // newNode == 0), and a later re-designation would take the add-branch | ||
| // and double-credit K_n / K_total with no offsetting decrement | ||
| // (K_total has no decrement path). Reject it. | ||
| if (newNode == 0) revert ZeroPrimaryNode(); | ||
| if (!shardingTableStorage.nodeExists(newNode)) { | ||
| revert PrimaryNodeNotInShardingTable(newNode); | ||
| } | ||
|
|
||
| uint72 oldNode = acct.primaryNode; | ||
|
|
||
| // Move only FUTURE epochs within the lock. The schedule's last | ||
| // credited epoch is `createdAtEpoch + lockDurationEpochs`. The | ||
| // proration schedule is computed ONCE (it is loop-invariant and | ||
| // anchored to the stored `Account`, so it byte-matches the | ||
| // create-time seed) and each epoch's amount is read off it via | ||
| // `_amountForEpoch` — making each move exactly reverse its seed. | ||
| PublishingMathLib.ActiveSinkRange[3] memory ranges = _scheduleFor(acct); | ||
| uint256 lockEpochs = uint256(acct.lockDurationEpochs); | ||
| uint256 lastEpoch = uint256(acct.createdAtEpoch) + lockEpochs; | ||
| for (uint256 e = uint256(currentEpoch) + 1; e <= lastEpoch; e++) { | ||
| uint96 amount = _amountForEpoch(ranges, acct.committedTRAC, lockEpochs, e); | ||
| if (amount == 0) continue; | ||
| // newNode is guaranteed non-zero by the guard above. | ||
| if (oldNode != 0) { | ||
| epochStorage.moveEpochPublishingAllocation(oldNode, newNode, e, amount); | ||
| } else { | ||
| // First designation of a PCA created with primaryNode == 0: | ||
| // no prior node was seeded for these epochs, so credit new. | ||
| epochStorage.addEpochPublishingAllocation(newNode, e, amount); | ||
| } | ||
| } | ||
|
|
||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); | ||
|
|
||
| emit PrimaryNodeChanged(accountId, oldNode, newNode, currentEpoch); | ||
| } |
Comment on lines
+432
to
+478
| function setPrimaryNode(uint256 accountId, uint72 newNode) external onlyConvictionNFT { | ||
| PublishingConvictionStorage.Account memory acct = | ||
| publishingConvictionStorage.getAccount(accountId); | ||
|
|
||
| uint40 currentEpoch = uint40(chronos.getCurrentEpoch()); | ||
| if (currentEpoch <= acct.lastPrimaryNodeChangeEpoch) { | ||
| revert PrimaryNodeChangeRateLimited(accountId, acct.lastPrimaryNodeChangeEpoch); | ||
| } | ||
| // OT-RFC-51: no "clear designation" semantic. Allowing newNode == 0 | ||
| // would null the stored primaryNode while leaving the old node's | ||
| // future buckets seeded (the move loop credits/decrements neither when | ||
| // newNode == 0), and a later re-designation would take the add-branch | ||
| // and double-credit K_n / K_total with no offsetting decrement | ||
| // (K_total has no decrement path). Reject it. | ||
| if (newNode == 0) revert ZeroPrimaryNode(); | ||
| if (!shardingTableStorage.nodeExists(newNode)) { | ||
| revert PrimaryNodeNotInShardingTable(newNode); | ||
| } | ||
|
|
||
| uint72 oldNode = acct.primaryNode; | ||
|
|
||
| // Move only FUTURE epochs within the lock. The schedule's last | ||
| // credited epoch is `createdAtEpoch + lockDurationEpochs`. The | ||
| // proration schedule is computed ONCE (it is loop-invariant and | ||
| // anchored to the stored `Account`, so it byte-matches the | ||
| // create-time seed) and each epoch's amount is read off it via | ||
| // `_amountForEpoch` — making each move exactly reverse its seed. | ||
| PublishingMathLib.ActiveSinkRange[3] memory ranges = _scheduleFor(acct); | ||
| uint256 lockEpochs = uint256(acct.lockDurationEpochs); | ||
| uint256 lastEpoch = uint256(acct.createdAtEpoch) + lockEpochs; | ||
| for (uint256 e = uint256(currentEpoch) + 1; e <= lastEpoch; e++) { | ||
| uint96 amount = _amountForEpoch(ranges, acct.committedTRAC, lockEpochs, e); | ||
| if (amount == 0) continue; | ||
| // newNode is guaranteed non-zero by the guard above. | ||
| if (oldNode != 0) { | ||
| epochStorage.moveEpochPublishingAllocation(oldNode, newNode, e, amount); | ||
| } else { | ||
| // First designation of a PCA created with primaryNode == 0: | ||
| // no prior node was seeded for these epochs, so credit new. | ||
| epochStorage.addEpochPublishingAllocation(newNode, e, amount); | ||
| } | ||
| } | ||
|
|
||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); | ||
|
|
||
| emit PrimaryNodeChanged(accountId, oldNode, newNode, currentEpoch); | ||
| } |
Comment on lines
+432
to
+478
| function setPrimaryNode(uint256 accountId, uint72 newNode) external onlyConvictionNFT { | ||
| PublishingConvictionStorage.Account memory acct = | ||
| publishingConvictionStorage.getAccount(accountId); | ||
|
|
||
| uint40 currentEpoch = uint40(chronos.getCurrentEpoch()); | ||
| if (currentEpoch <= acct.lastPrimaryNodeChangeEpoch) { | ||
| revert PrimaryNodeChangeRateLimited(accountId, acct.lastPrimaryNodeChangeEpoch); | ||
| } | ||
| // OT-RFC-51: no "clear designation" semantic. Allowing newNode == 0 | ||
| // would null the stored primaryNode while leaving the old node's | ||
| // future buckets seeded (the move loop credits/decrements neither when | ||
| // newNode == 0), and a later re-designation would take the add-branch | ||
| // and double-credit K_n / K_total with no offsetting decrement | ||
| // (K_total has no decrement path). Reject it. | ||
| if (newNode == 0) revert ZeroPrimaryNode(); | ||
| if (!shardingTableStorage.nodeExists(newNode)) { | ||
| revert PrimaryNodeNotInShardingTable(newNode); | ||
| } | ||
|
|
||
| uint72 oldNode = acct.primaryNode; | ||
|
|
||
| // Move only FUTURE epochs within the lock. The schedule's last | ||
| // credited epoch is `createdAtEpoch + lockDurationEpochs`. The | ||
| // proration schedule is computed ONCE (it is loop-invariant and | ||
| // anchored to the stored `Account`, so it byte-matches the | ||
| // create-time seed) and each epoch's amount is read off it via | ||
| // `_amountForEpoch` — making each move exactly reverse its seed. | ||
| PublishingMathLib.ActiveSinkRange[3] memory ranges = _scheduleFor(acct); | ||
| uint256 lockEpochs = uint256(acct.lockDurationEpochs); | ||
| uint256 lastEpoch = uint256(acct.createdAtEpoch) + lockEpochs; | ||
| for (uint256 e = uint256(currentEpoch) + 1; e <= lastEpoch; e++) { | ||
| uint96 amount = _amountForEpoch(ranges, acct.committedTRAC, lockEpochs, e); | ||
| if (amount == 0) continue; | ||
| // newNode is guaranteed non-zero by the guard above. | ||
| if (oldNode != 0) { | ||
| epochStorage.moveEpochPublishingAllocation(oldNode, newNode, e, amount); | ||
| } else { | ||
| // First designation of a PCA created with primaryNode == 0: | ||
| // no prior node was seeded for these epochs, so credit new. | ||
| epochStorage.addEpochPublishingAllocation(newNode, e, amount); | ||
| } | ||
| } | ||
|
|
||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); | ||
|
|
||
| emit PrimaryNodeChanged(accountId, oldNode, newNode, currentEpoch); | ||
| } |
Comment on lines
+432
to
+478
| function setPrimaryNode(uint256 accountId, uint72 newNode) external onlyConvictionNFT { | ||
| PublishingConvictionStorage.Account memory acct = | ||
| publishingConvictionStorage.getAccount(accountId); | ||
|
|
||
| uint40 currentEpoch = uint40(chronos.getCurrentEpoch()); | ||
| if (currentEpoch <= acct.lastPrimaryNodeChangeEpoch) { | ||
| revert PrimaryNodeChangeRateLimited(accountId, acct.lastPrimaryNodeChangeEpoch); | ||
| } | ||
| // OT-RFC-51: no "clear designation" semantic. Allowing newNode == 0 | ||
| // would null the stored primaryNode while leaving the old node's | ||
| // future buckets seeded (the move loop credits/decrements neither when | ||
| // newNode == 0), and a later re-designation would take the add-branch | ||
| // and double-credit K_n / K_total with no offsetting decrement | ||
| // (K_total has no decrement path). Reject it. | ||
| if (newNode == 0) revert ZeroPrimaryNode(); | ||
| if (!shardingTableStorage.nodeExists(newNode)) { | ||
| revert PrimaryNodeNotInShardingTable(newNode); | ||
| } | ||
|
|
||
| uint72 oldNode = acct.primaryNode; | ||
|
|
||
| // Move only FUTURE epochs within the lock. The schedule's last | ||
| // credited epoch is `createdAtEpoch + lockDurationEpochs`. The | ||
| // proration schedule is computed ONCE (it is loop-invariant and | ||
| // anchored to the stored `Account`, so it byte-matches the | ||
| // create-time seed) and each epoch's amount is read off it via | ||
| // `_amountForEpoch` — making each move exactly reverse its seed. | ||
| PublishingMathLib.ActiveSinkRange[3] memory ranges = _scheduleFor(acct); | ||
| uint256 lockEpochs = uint256(acct.lockDurationEpochs); | ||
| uint256 lastEpoch = uint256(acct.createdAtEpoch) + lockEpochs; | ||
| for (uint256 e = uint256(currentEpoch) + 1; e <= lastEpoch; e++) { | ||
| uint96 amount = _amountForEpoch(ranges, acct.committedTRAC, lockEpochs, e); | ||
| if (amount == 0) continue; | ||
| // newNode is guaranteed non-zero by the guard above. | ||
| if (oldNode != 0) { | ||
| epochStorage.moveEpochPublishingAllocation(oldNode, newNode, e, amount); | ||
| } else { | ||
| // First designation of a PCA created with primaryNode == 0: | ||
| // no prior node was seeded for these epochs, so credit new. | ||
| epochStorage.addEpochPublishingAllocation(newNode, e, amount); | ||
| } | ||
| } | ||
|
|
||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); | ||
|
|
||
| emit PrimaryNodeChanged(accountId, oldNode, newNode, currentEpoch); | ||
| } |
Comment on lines
+432
to
+478
| function setPrimaryNode(uint256 accountId, uint72 newNode) external onlyConvictionNFT { | ||
| PublishingConvictionStorage.Account memory acct = | ||
| publishingConvictionStorage.getAccount(accountId); | ||
|
|
||
| uint40 currentEpoch = uint40(chronos.getCurrentEpoch()); | ||
| if (currentEpoch <= acct.lastPrimaryNodeChangeEpoch) { | ||
| revert PrimaryNodeChangeRateLimited(accountId, acct.lastPrimaryNodeChangeEpoch); | ||
| } | ||
| // OT-RFC-51: no "clear designation" semantic. Allowing newNode == 0 | ||
| // would null the stored primaryNode while leaving the old node's | ||
| // future buckets seeded (the move loop credits/decrements neither when | ||
| // newNode == 0), and a later re-designation would take the add-branch | ||
| // and double-credit K_n / K_total with no offsetting decrement | ||
| // (K_total has no decrement path). Reject it. | ||
| if (newNode == 0) revert ZeroPrimaryNode(); | ||
| if (!shardingTableStorage.nodeExists(newNode)) { | ||
| revert PrimaryNodeNotInShardingTable(newNode); | ||
| } | ||
|
|
||
| uint72 oldNode = acct.primaryNode; | ||
|
|
||
| // Move only FUTURE epochs within the lock. The schedule's last | ||
| // credited epoch is `createdAtEpoch + lockDurationEpochs`. The | ||
| // proration schedule is computed ONCE (it is loop-invariant and | ||
| // anchored to the stored `Account`, so it byte-matches the | ||
| // create-time seed) and each epoch's amount is read off it via | ||
| // `_amountForEpoch` — making each move exactly reverse its seed. | ||
| PublishingMathLib.ActiveSinkRange[3] memory ranges = _scheduleFor(acct); | ||
| uint256 lockEpochs = uint256(acct.lockDurationEpochs); | ||
| uint256 lastEpoch = uint256(acct.createdAtEpoch) + lockEpochs; | ||
| for (uint256 e = uint256(currentEpoch) + 1; e <= lastEpoch; e++) { | ||
| uint96 amount = _amountForEpoch(ranges, acct.committedTRAC, lockEpochs, e); | ||
| if (amount == 0) continue; | ||
| // newNode is guaranteed non-zero by the guard above. | ||
| if (oldNode != 0) { | ||
| epochStorage.moveEpochPublishingAllocation(oldNode, newNode, e, amount); | ||
| } else { | ||
| // First designation of a PCA created with primaryNode == 0: | ||
| // no prior node was seeded for these epochs, so credit new. | ||
| epochStorage.addEpochPublishingAllocation(newNode, e, amount); | ||
| } | ||
| } | ||
|
|
||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); | ||
|
|
||
| emit PrimaryNodeChanged(accountId, oldNode, newNode, currentEpoch); | ||
| } |
Comment on lines
+498
to
+518
| function _scheduleFor( | ||
| PublishingConvictionStorage.Account memory acct | ||
| ) internal view returns (PublishingMathLib.ActiveSinkRange[3] memory ranges) { | ||
| uint40 anchorEpoch = acct.createdAtEpoch; | ||
| uint256 epochs = uint256(acct.lockDurationEpochs); | ||
| // Time the account had remaining in its creation epoch, derived from | ||
| // stored fields: (start of next epoch) - createdAtTimestamp. | ||
| uint256 nextEpochStart = chronos.timestampForEpoch(uint256(anchorEpoch) + 1); | ||
| uint256 timeRemainingInCurrentEpoch = | ||
| nextEpochStart > uint256(acct.createdAtTimestamp) | ||
| ? nextEpochStart - uint256(acct.createdAtTimestamp) | ||
| : 0; | ||
|
|
||
| ranges = PublishingMathLib.prorateActiveSink( | ||
| acct.committedTRAC, | ||
| anchorEpoch, | ||
| epochs, | ||
| chronos.epochLength(), | ||
| timeRemainingInCurrentEpoch | ||
| ); | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * backing it and a recorded `Account` row. | ||
| */ | ||
| function createAccount(uint96 committedTRAC) external returns (uint256 accountId) { | ||
| function createAccount(uint96 committedTRAC, uint72 primaryNode) external returns (uint256 accountId) { |
There was a problem hiding this comment.
🔴 Bug: This changes createAccount to a new 2-arg selector, but the PR does not migrate the monorepo callers and duplicated ABIs that still encode createAccount(uint96). packages/chain/src/evm-adapter-conviction.ts, packages/publisher/test/agent-provenance-e2e.test.ts, and several devnet/unit tests still call the old signature, so they will revert once this contract is deployed. Either keep a backwards-compatible overload or update/regenerate every consumer in the same PR.
| } | ||
|
|
||
| function addEpochProducedKnowledgeValue( | ||
| function addEpochPublishingAllocation( |
There was a problem hiding this comment.
🔴 Bug: Renaming the EpochStorage selectors here is a public contract/API break, but the PR does not update the duplicated packages/chain/abi/EpochStorage.json or all remaining callers that still encode *ProducedKnowledgeValue*. That leaves SDK/tests on stale selectors against the upgraded contract. Please migrate the chain ABI and call sites in the same PR, or preserve the old selectors for compatibility.
| expect(afterSpent - beforeSpent).toBeGreaterThan(0n); | ||
|
|
||
| const afterEps: bigint = await s.eps.getNodeEpochProducedKnowledgeValue(core1.identityId, epoch); | ||
| const afterEps: bigint = await s.eps.getNodeEpochPublishingAllocation(core1.identityId, epoch); |
There was a problem hiding this comment.
🔴 Bug: RFC-51 removed realized-publish writes to EpochStorage, so getNodeEpochPublishingAllocation(...) now tracks committed PCA allocation, not per-publish attribution. After a publish this value should stay unchanged, which makes this renamed assertion incorrect; the same pattern appears in devnet/v10-end-to-end/automated.test.ts and devnet/v10-stress/automated.test.ts. Update these tests to assert the allocation was seeded at account creation and remains stable through publish.
| @@ -119,12 +119,12 @@ async function calculateExpectedNodeScore( | |||
| const startEpoch = currentEpoch >= 3n ? currentEpoch - 3n : 0n; | |||
There was a problem hiding this comment.
🟡 Issue: RandomSampling._calculateNodeScore now reads only the current epoch bucket, but this helper still sums a 4-epoch window. That means any archive test using calculateExpectedNodeScore no longer mirrors production scoring after RFC-51 and can validate the wrong result. Mirror the single-epoch contract logic here instead of only renaming the getters.
…PrimaryNode Round 1 of PR #1195 feedback (Slither): - Re-add the per-epoch epochNodeMax write in addEpochPublishingAllocation. Dropping it (an earlier gas nit) left the mapping read-in-getters but written-nowhere → Slither "uninitialized state variable". The mapping cannot be removed (it precedes nodesPaidOut in this persistent storage layout; removing would shift slots), so re-adding the write is the layout-safe fix. - Reorder setPrimaryNode to checks-effects-interactions: persist setPrimaryNodeData before the external EpochStorage move loop. The loop uses only locals, so this is behaviour-preserving; clears the Slither reentrancy finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy the RFC-51 source-of-truth ABIs from packages/evm-module/abi to the duplicated packages/chain/abi: EpochStorage, PublishingConviction, DKGPublishingConvictionNFT, PublishingConvictionStorage, IDKGPublishingConvictionNFT. Brings the chain copies onto the new surface: createAccount gains primaryNode (uint72); setPrimaryNode / moveEpochPublishingAllocation / ZeroPrimaryNode added; the producedKnowledgeValue -> publishingAllocation rename (getters/event/mutators); and the widened Account tuple (primaryNode, lastPrimaryNodeChangeEpoch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…count RFC-51 added a primaryNode arg to the NFT createAccount selector. Pass [committedTRAC, primaryNode] in the EVM adapter and add an optional primaryNode parameter (default 0n = no designated node) to the adapter method, the ChainAdapter interface, the mock adapter, and the DKGAgent registry facade so the value can be threaded from the public API while existing callers (which don't designate a node) keep working unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…publish Update the hand-written createAccount ABI fragment + call to the 2-arg (committedTRAC, primaryNode=0n) form. RFC-51 removed realized-publish credits: getNodeEpochPublishingAllocation now tracks committed PCA allocation only, so a plain publish leaves it UNCHANGED. Flip the Diagram 1 and Diagram 6 (a)/(e) assertions from toBeGreaterThan to toBe(before) and rewrite the now-stale comments/titles. Diagram 6 (a)/(e) no longer discriminate via this getter — their surviving coverage is publish confirmation + on-chain acceptance of a valid override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forward-migrate the devnet PCA callers + assertions: - createAccount ABI fragments -> (uint96, uint72); calls pass primaryNode 0n between committedTRAC and the ethers overrides object; the typed cast in agent-provenance gains the primaryNode parameter. - accounts() fragment in conviction-lazy-settle widened to the 11-field tuple (primaryNode, lastPrimaryNodeChangeEpoch appended). - getNodeEpochPublishingAllocation assertions that expected a publish to CREDIT a node now expect it UNCHANGED (RFC-51 removed realized-publish credits). For mode (b) / unauthorized-fall-through, whose original primary invariant was per-publish K_n attribution, the comments are rewritten: the surviving proof is author-recorded + publish-confirmed. Do not run the live-devnet suites; references + assertion semantics only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
calculateExpectedNodeScore summed a 4-epoch publishing window; mirror RandomSampling._calculateNodeScore, which now reads K_n / K_total from currentEpoch only. Also rename the stale getNodeCurrentEpochProducedKnowledgeValue getter to getNodeCurrentEpochPublishingAllocation in both archive helpers. Archive is excluded from CI; fixing per Codex flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… max addEpochPublishingAllocation re-added the epochNodeMaxPublishingAllocation write, so the per-epoch node max getters are maintained again. Restore the three max-getter assertions that an earlier closeout had flipped to expect 0: single-add -> 500; multi-node epoch -> max(1500,3000,2800)=3000; current/previous-epoch max -> 800 / 1000. Run green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| * backing it and a recorded `Account` row. | ||
| */ | ||
| function createAccount(uint96 committedTRAC) external returns (uint256 accountId) { | ||
| function createAccount(uint96 committedTRAC, uint72 primaryNode) external returns (uint256 accountId) { |
There was a problem hiding this comment.
🔴 Bug: Changing the public wrapper ABI from createAccount(uint96) to createAccount(uint96,uint72) is a breaking change that hasn't been propagated through the repo. There are still live callers such as test/unit/DKGPublishingConvictionNFT*.ts, test/v10-pca-lifecycle.test.ts, and test/v10-pca-agent-dos.test.ts that invoke the 1-arg form, so the regenerated TypeChain surface will fail to compile/run. Either keep a 1-arg compatibility overload that forwards primaryNode = 0, or update every remaining caller in this PR.
| /// (`epochPublishingAllocation`, i.e. K_total) is intentionally | ||
| /// NOT touched, so the move is net-zero on the denominator of | ||
| /// every node's publishing factor. The per-epoch node max is | ||
| /// deliberately left stale (the RFC does not require recomputing |
There was a problem hiding this comment.
🔴 Bug: Leaving epochNodeMaxPublishingAllocation intentionally stale makes the public max getters incorrect after setPrimaryNode. A move can lower the old max node or raise the destination above the stored max, so getEpochNodeMaxPublishingAllocation/getCurrentEpochNodeMaxPublishingAllocation can return values that no longer match any node's actual allocation. Please recompute the max when moving away from or into the current max, or remove/deprecate these getters in the same API change.
| // `addEpochProducedKnowledgeValue(publisherNodeIdentityId, ...)` | ||
| // write has been removed here. `publisherNodeIdentityId` remains a | ||
| // self-claimed attribution field on the publish struct but no longer | ||
| // drives scoring, so there is no longer a `nodeExists` gate to apply. |
There was a problem hiding this comment.
🔴 Bug: Removing the nodeExists gate here changes the publish contract from "reject bogus publisherNodeIdentityId" to "persist any arbitrary non-zero id". Existing callers/tests still rely on the revert path (packages/publisher/test/agent-provenance-e2e.test.ts covers it), so this is an input-contract break as well as a data-quality regression for on-chain attribution. If RFC-51 really wants self-claimed ids, the caller-side validation/tests/docs need to be updated in the same PR; otherwise keep the non-zero nodeExists check.
…on bumps TASK 1 — forward-migrate every single-arg createAccount call/helper to the RFC-51 wrapper signature createAccount(uint96 committedTRAC, uint72 primaryNode). Pass an inert primaryNode = 0 everywhere (no designated node, no allocation seeded) since these lifecycle / DoS / conservation / context-graph / NFT-unit tests don't assert publishing-allocation or score behaviour. Updated helpers (createAccountFor, the NFT-extra and ContextGraphs createAccount wrappers) plus all direct calls, including the createAccount(0,0) InvalidAmount revert case. TASK 2 — bump the RandomSampling version assertions to match the contract: unit 10.0.4 -> 10.0.5, integration 10.0.3 -> 10.0.5. EpochStorage already asserts 10.0.4 (correct); no other bumped contract has a test version assert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TASK 3 — the CH-5 abi-pinning drift detector flagged exactly the three conviction ABIs whose surface RFC-51 intentionally changed (the chain-local abi/ snapshot was already synced in a26dd41): DKGPublishingConvictionNFT, PublishingConviction, PublishingConvictionStorage. Re-pinned to the digests the test now reports and documented the intentional drift inline: createAccount gained a uint72 primaryNode arg (new arity), setPrimaryNode + ZeroPrimaryNode guard + moveEpochPublishingAllocation, the PCA Account tuple widened to carry the designated primaryNode, and the old per-publish realized-publish credit path was removed (publishingAllocation is now seeded prorated at account creation). KAL / DKGKnowledgeAssets pins and every event-shape sanity assertion did NOT drift (verified) — RFC-51 touched only the conviction contracts. EpochStorage and IDKGPublishingConvictionNFT have no pin entry, so none was added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ved) TASK 4 — RFC-51 removed the realized-publish credit block that ran `shardingTable.nodeExists(publisherNodeIdentityId)`, so publisherNodeIdentityId is now a vestigial, unvalidated, ignored field at publish time (attribution is the PCA's primaryNode, seeded at account creation). The former "validation revert — fake non-existent identity id reverts on chain" case is renamed and rewritten to assert the publish CONFIRMS (resolves) instead of rejecting. Updated the stale mode-(e) comment (no per-publish attribution contrast survives — valid / zero / fake ids all confirm and none moves any node's K_n) and the file-header + harness-staking comments accordingly (staking is still needed for ACK quorum, not for the removed attribution check). Verified locally: agent-provenance-e2e.test.ts 17 passed / 1 skipped / 0 failed (mode (e) self-skips on the single-node harness with no receiver cores). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| uint256 epoch, | ||
| uint96 amount | ||
| ) external onlyContracts { | ||
| nodesEpochPublishingAllocation[fromId][epoch] -= amount; |
There was a problem hiding this comment.
🔴 Bug: moveEpochPublishingAllocation never updates epochNodeMaxPublishingAllocation. After a primary-node move, the public get*NodeMaxPublishingAllocation getters can become wrong in both directions: stale high if the source used to be the max holder, or too low if the destination becomes the new max. Recompute or at least refresh the max in this path, otherwise the renamed max getters stop reflecting on-chain state as soon as setPrimaryNode is used.
| // MUST surface the owner revert (→ 403), never swallow it. | ||
|
|
||
| createPublishingConvictionAccount?(committedTRAC: bigint): Promise<{ accountId: bigint } & TxResult>; | ||
| createPublishingConvictionAccount?(committedTRAC: bigint, primaryNode?: bigint): Promise<{ accountId: bigint } & TxResult>; |
There was a problem hiding this comment.
🟡 Issue: this adds a write-side primaryNode parameter, but the adapter's canonical PCA read model still exposes only the pre-RFC-51 fields. Callers can now create/designate a primary node through the SDK, but they still cannot read back primaryNode or lastPrimaryNodeChangeEpoch via getPublishingConvictionAccountInfo, which makes the feature effectively write-only for higher-level clients. Extend the DTO/read path alongside this API change.
| } | ||
|
|
||
| emit AccountCreated(accountId, publisher, committedTRAC, discountBps, currentEpoch, expiresAtEpoch); | ||
| emit PrimaryNodeChanged(accountId, 0, primaryNode, currentEpoch); |
There was a problem hiding this comment.
🟡 Issue: createAccount(..., 0) now emits PrimaryNodeChanged(accountId, 0, 0, currentEpoch), which is a no-op state transition but looks like a real designation change to off-chain consumers. Indexers using this event as the assignment history will record a spurious change for inert PCAs. Consider emitting it only when primaryNode != 0, or splitting initial designation into a separate event.
Unrelated to OT-RFC-51 — pre-existing on main: ShardingTable.sol is _VERSION 10.0.3 but the unit test still asserts 10.0.2 (both byte-identical to main; this PR does not touch ShardingTable). Included only to unblock the Tornado Solidity shard so the PR's CI reflects RFC-51 cleanly. Safe to drop and handle separately if preferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| * backing it and a recorded `Account` row. | ||
| */ | ||
| function createAccount(uint96 committedTRAC) external returns (uint256 accountId) { | ||
| function createAccount(uint96 committedTRAC, uint72 primaryNode) external returns (uint256 accountId) { |
There was a problem hiding this comment.
🔴 Bug: This mutates the existing PCA ABI in place: createAccount(uint96) becomes a different selector here, and accounts(uint256) is widened below. Any integration generated against the current contract will start reverting or fail to decode after upgrade. If RFC-51 needs to land on an existing deployment, preserve the old selector/getter and add versioned entrypoints for the new fields instead of rewriting the live surface.
|
|
||
| function getNodeEpochProducedKnowledgeValue(uint72 identityId, uint256 epoch) external view returns (uint96) { | ||
| return nodesEpochProducedKnowledgeValue[identityId][epoch]; | ||
| function getNodeEpochPublishingAllocation(uint72 identityId, uint256 epoch) external view returns (uint96) { |
There was a problem hiding this comment.
🔴 Bug: Renaming the *ProducedKnowledgeValue* views to *PublishingAllocation* is another contract ABI break. Off-chain consumers compiled against the current EpochStorage interface will hit missing-function errors after upgrade. Please keep the old selectors as forwarding shims, or roll this out behind a versioned storage contract.
| /// (`epochPublishingAllocation`, i.e. K_total) is intentionally | ||
| /// NOT touched, so the move is net-zero on the denominator of | ||
| /// every node's publishing factor. The per-epoch node max is | ||
| /// deliberately left stale (the RFC does not require recomputing |
There was a problem hiding this comment.
🔴 Bug: Leaving epochNodeMaxPublishingAllocation stale makes the public get*NodeMaxPublishingAllocation() getters incorrect after setPrimaryNode moves allocation between nodes. A move from the current max holder to another node can make the reported max both too high or too low. Recompute/update the max on moves, or drop the getter if it is no longer trustworthy.
|
|
||
| async createPublishingConvictionAccount(this: DKGAgent, | ||
| committedTRAC: bigint, | ||
| primaryNode: bigint = 0n, |
There was a problem hiding this comment.
🔴 Bug: Defaulting primaryNode to 0n keeps existing call sites compiling, but under RFC-51 it silently creates a PCA with no seeded allocation. This PR also doesn't expose a setPrimaryNode method through the TS SDK/daemon surface, so unchanged callers have no way to opt back in without raw contract calls. Make primaryNode explicit all the way through the public API, or fail fast when it is omitted.
Address Codex review on PR #1195: - moveEpochPublishingAllocation now raises epochNodeMaxPublishingAllocation on the destination, consistent with addEpochPublishingAllocation, so the public max getters are a coherent per-epoch HIGH-WATER MARK rather than going too-low after a re-designation. The decrement deliberately does not lower it (true current max would need an O(nodes) scan); documented as off-chain-observability only — nothing on-chain reads it (scoring reads only get(Node)EpochPublishingAllocation), so no consensus impact. - createAccount only emits PrimaryNodeChanged when primaryNode != 0, so an inert createAccount(_, 0) no longer emits a spurious PrimaryNodeChanged(_, 0, 0, _) that indexers would record as a real designation. Not addressed (deliberate): the ABI-break review comments (new createAccount arity, EpochStorage getter rename, accounts() widening) — RFC-51 intentionally breaks these selectors; internal callers/ABIs are forward-migrated and CI is green; preserving old selectors for backwards compat is out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…FC-49 catalog cutover) #1201 (RFC-49 catalog model + ACK domain separation) landed on main and edits the same contracts as RFC-51. Resolved 4 conflicts: - KnowledgeAssetsLifecycle.sol: keep main's catalog model + ACK_DIGEST_VERSION; RFC-51's realized-credit removal preserved (calls gone, comments remain); _VERSION 10.1.0 -> 10.1.1. - RandomSampling.sol: keep main's proof-race/sampling rewrite + RFC-51's 1-epoch publishing-factor read (main left _calculateNodeScore untouched); _VERSION 10.1.0 -> 10.1.1. - EpochStorage.test.ts / RandomSampling.test.ts: version assertions reconciled (10.0.4 / 10.1.1). Re-synced the 3 main-domain ABIs (DKGKnowledgeAssets, KnowledgeAssetsLifecycle, RandomSamplingStorage) in evm-module + chain. Verified: compile OK; focused + EpochStorage + RandomSampling unit = 68 passing; chain abi-pinning 20/20; no straggler callers of the renamed EpochStorage getters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| * backing it and a recorded `Account` row. | ||
| */ | ||
| function createAccount(uint96 committedTRAC) external returns (uint256 accountId) { | ||
| function createAccount(uint96 committedTRAC, uint72 primaryNode) external returns (uint256 accountId) { |
There was a problem hiding this comment.
🔴 Bug: This replaces the legacy createAccount(uint96) selector instead of preserving it. Any existing publisher/CLI still compiled against the one-argument ABI will start failing as soon as this wrapper is redeployed. Please keep a backward-compatible overload that forwards to createAccount(committedTRAC, 0) or roll this out as an explicit breaking contract/API version.
|
|
||
| function getNodeEpochProducedKnowledgeValue(uint72 identityId, uint256 epoch) external view returns (uint96) { | ||
| return nodesEpochProducedKnowledgeValue[identityId][epoch]; | ||
| function getNodeEpochPublishingAllocation(uint72 identityId, uint256 epoch) external view returns (uint96) { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public getter here removes the existing getNodeEpochProducedKnowledgeValue(uint72,uint256) selector. That is an ABI break for live off-chain consumers even though the underlying storage is the same. Consider keeping the old getter(s) as forwarding aliases so external callers can migrate gradually.
| // so this reordering is behaviour-preserving; it also clears Slither's | ||
| // reentrancy finding (the EpochStorage callees are Hub-registered | ||
| // onlyContracts storage, but CEI ordering is the right discipline). | ||
| publishingConvictionStorage.setPrimaryNodeData(accountId, newNode, currentEpoch); |
There was a problem hiding this comment.
🟡 Issue: There is no newNode != oldNode guard before advancing lastPrimaryNodeChangeEpoch. A no-op setPrimaryNode(accountId, acct.primaryNode) now succeeds, emits PrimaryNodeChanged, and consumes the account's only change slot for that epoch, which can block the real re-designation the operator intended. Revert or short-circuit before persisting the new cursor when the designation is unchanged.
| // MUST surface the owner revert (→ 403), never swallow it. | ||
|
|
||
| createPublishingConvictionAccount?(committedTRAC: bigint): Promise<{ accountId: bigint } & TxResult>; | ||
| createPublishingConvictionAccount?(committedTRAC: bigint, primaryNode?: bigint): Promise<{ accountId: bigint } & TxResult>; |
There was a problem hiding this comment.
🟡 Issue: The SDK surface now exposes primaryNode at creation time, but there is still no adapter method for the new on-chain setPrimaryNode flow. That leaves the late-designation path (createAccount(..., 0) and assign later) unreachable from the supported TypeScript API and forces callers down to raw contract calls. Please expose a dedicated re-designation method in the same public surface.
A `setPrimaryNode(accountId, currentNode)` call was previously accepted: it would burn the once-per-epoch change slot, emit a spurious `PrimaryNodeChanged`, and run a pointless future-epoch self-move loop (net-zero, but wasted gas and a misleading event). Reject it with a new `PrimaryNodeUnchanged(accountId, node)` error, guarding right after `oldNode` is read. Adds a focused revert test, re-pins the PublishingConviction ABI digest for the new error, and bumps the contract `_VERSION` to 10.0.4 for the surface change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // `addEpochProducedKnowledgeValue(publisherNodeIdentityId, ...)` | ||
| // write has been removed here. `publisherNodeIdentityId` remains a | ||
| // self-claimed attribution field on the publish struct but no longer | ||
| // drives scoring, so there is no longer a `nodeExists` gate to apply. |
There was a problem hiding this comment.
🔴 Bug: Removing the nodeExists gate changes publisherNodeIdentityId from "0 or a real node" into arbitrary caller-controlled data, but we still persist and expose that field as publish attribution. That is a behavioral/API regression for any consumer that assumes non-zero ids resolve to an existing node. Please keep the validation when publisherNodeIdentityId != 0, even if realized publishes no longer credit EpochStorage.
| /// (`epochPublishingAllocation`, i.e. K_total) is intentionally | ||
| /// NOT touched, so the move is net-zero on the denominator of | ||
| /// every node's publishing factor. | ||
| /// @dev `epochNodeMaxPublishingAllocation` is a per-epoch HIGH-WATER |
There was a problem hiding this comment.
🔴 Bug: moveEpochPublishingAllocation no longer keeps epochNodeMaxPublishingAllocation equal to the live per-epoch max. Once allocation moves off the previous max holder, get*NodeMaxPublishingAllocation() can return a stale value that no node actually has. That silently changes the meaning of an existing public getter for off-chain consumers. Either recompute the max on moves, or rename/deprecate this getter instead of keeping the old contract.
| // MUST surface the owner revert (→ 403), never swallow it. | ||
|
|
||
| createPublishingConvictionAccount?(committedTRAC: bigint): Promise<{ accountId: bigint } & TxResult>; | ||
| createPublishingConvictionAccount?(committedTRAC: bigint, primaryNode?: bigint): Promise<{ accountId: bigint } & TxResult>; |
There was a problem hiding this comment.
🟡 Issue: This exposes the new primaryNode only on create, but the same PR adds on-chain setPrimaryNode and new account fields. Without matching adapter/agent read-write methods, SDK consumers cannot re-designate or even verify the stored primary node without dropping to raw contract calls. Please plumb the rest of the RFC-51 surface through this API layer in the same change.
| async createPublishingConvictionAccount(committedTRAC: bigint): Promise<{ accountId: bigint } & TxResult> { | ||
| // `primaryNode` (RFC-51) is accepted for interface parity but not modeled: | ||
| // the mock tracks account budget only, not per-node publishing allocation. | ||
| async createPublishingConvictionAccount(committedTRAC: bigint, _primaryNode: bigint = 0n): Promise<{ accountId: bigint } & TxResult> { |
There was a problem hiding this comment.
🟡 Issue: The mock now accepts primaryNode but ignores it completely, including the new validation surface. That makes mock-based tests diverge from real EVM behavior and can hide regressions around out-of-range or otherwise invalid node ids. At minimum validate the uint72 range here, and ideally model or explicitly reject unsupported non-zero primaryNode values.
…SDK/CLI) Before this, `POST /api/pca` and the SDK `createPublishingConvictionAccount` defaulted `primaryNode` to `0n`, so a PCA created via the standard daemon endpoint seeded its committed TRAC to NO node — contributing nothing to any node's publishing factor P(t). Since the SDK exposes no `setPrimaryNode`, such a PCA was silently and permanently inert. RFC-51 was therefore non-functional end-to-end through the normal flow. (Not a consensus bug: a zero-seeded PCA just yields P(t)=0 identically on every node, since RandomSampling guards the K_total==0 denominator.) Make the node an explicit, required decision at creation: - daemon `POST /api/pca`: require a non-zero `primaryNode` in the body (parsePrimaryNode validates a uint72 > 0; node 0 is the contract's "no node" sentinel and is rejected here because the daemon can't re-point it). - agent registry `createPublishingConvictionAccount`: drop the `0n` default. - api-client `createPca` + CLI `pca create --primary-node`: thread it through. - tests: assert the node reaches the facade; add missing/zero/non-integer rejection cases. Adapter-level defaults are left intact (direct-adapter callers may still create-then-designate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
branarakic
commented
Jun 17, 2026
branarakic
left a comment
branarakic
left a comment
Contributor
Author
There was a problem hiding this comment.
Codex review finding.
| if (typeof node !== 'bigint') return jsonResponse(res, 400, node); | ||
| try { | ||
| const result = await agent.createPublishingConvictionAccount(amount); | ||
| const result = await agent.createPublishingConvictionAccount(amount, node); |
Contributor
Author
There was a problem hiding this comment.
🟡 Issue: This new primaryNode path can now hit deterministic PCA reverts such as PrimaryNodeNotInShardingTable, ZeroPrimaryNode, PrimaryNodeChangeRateLimited, and PrimaryNodeUnchanged, but classifyPcaRevert only knows the pre-RFC-51 errors. A syntactically valid but nonexistent node id will therefore come back from POST /api/pca as HTTP 500 instead of a caller-actionable 400/409. Please add the new primary-node custom errors to the classifier and cover at least the bad-node create case in the route tests.
…ot 500 The new POST /api/pca primaryNode path can revert with RFC-51 custom errors that classifyPcaRevert didn't know, so a syntactically valid but nonexistent node id came back as HTTP 500 instead of a caller-actionable 4xx. Add the primary-node errors to the classifier: PrimaryNodeNotInShardingTable (400, reachable from createAccount) plus ZeroPrimaryNode (400), PrimaryNodeUnchanged (409) and PrimaryNodeChangeRateLimited (409) — the latter three are setPrimaryNode-only (re-designation, not yet wired) but classified now so they map cleanly when that route lands. Adds the bad-node create route test. Addresses branarakic's PR #1195 review comment on pca.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n path Adds devnet/rfc51-publishing-allocation — an operator-run devnet suite that covers what the Hardhat unit suite and the mock-based daemon route tests cannot: the full RFC-51 creation path against a live multi-node devnet (daemon POST /api/pca → agent registry → chain adapter → real DKGPublishingConvictionNFT.createAccount → EpochStorage seeding). Two assertions, both validated green on a 2-core devnet: - Attribution-follows-param: a PCA created THROUGH node1's daemon but designating node2 seeds node2's committed allocation by exactly committedTRAC (summed over the lock) and leaves node1's untouched — the literal heart of RFC-51 (a staker allocates to a node of their choice). - Bad primaryNode → HTTP 400: a non-existent node produces a real, ethers-wrapped PrimaryNodeNotInShardingTable revert that the daemon's classifyPcaRevert maps to 400 (not 500) — proving the classifier fix works on a live revert, which the synthetic-string unit test can't show. Like every devnet suite it's operator-run (./scripts/devnet.sh start 2; pnpm test:devnet:rfc51-publishing-allocation), NOT part of CI's default fan-out. Registers the workspace package + the test:devnet:* script. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
branarakic
commented
Jun 17, 2026
| const MAX_UINT72 = (1n << 72n) - 1n; | ||
|
|
||
| function parsePrimaryNode(raw: unknown): bigint | { error: string } { | ||
| if (typeof raw !== 'string' && typeof raw !== 'number') { |
Contributor
Author
There was a problem hiding this comment.
🔴 Bug: Accepting JSON numbers here can allocate to the wrong node for valid uint72 ids above Number.MAX_SAFE_INTEGER: JSON.parse has already rounded the value before String(raw)/BigInt(s) runs. A direct HTTP caller sending a large identity id as a number can therefore create a PCA for a different rounded node (or get a misleading bad-node revert). Make primaryNode string-only, or reject number inputs unless Number.isSafeInteger(raw), and add a regression for an unsafe integer.
A uint72 node identityId can exceed Number.MAX_SAFE_INTEGER (2^53-1). When primaryNode arrives as a JSON *number*, JSON.parse has already rounded it to the nearest double before parsePrimaryNode runs, so String(raw)/BigInt() would silently read a DIFFERENT node — creating a PCA for the wrong (rounded) node or producing a misleading bad-node revert. Reject non-safe-integer numbers and direct large ids to the string path (which BigInt-parses losslessly). Adds regressions: 2**53 number → 400, and a max-uint72 string accepted exactly. Addresses branarakic's PR #1195 review comment on pca.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OT-RFC-51: Publishing Allocation
Feeds the node-score publishing factor
P(t)with committed PCA publishing allocation instead of realized publishing volume. Reward-pool economics are unchanged — only what feedsK_nchanges — but a node's expected yield becomes forward-knowable, the publish-to-earn spam gradient is removed, and the "expected publishing budget" signal and the actual yield driver become the same on-chain number (which the staking UI can read directly).Spec: OT-RFC-51 in
dkgv10-spec(branchrfc/51-publishing-allocation).What changes
PublishingConvictionStorage.Account) gains a persistentprimaryNodedesignation (+lastPrimaryNodeChangeEpoch).createAccount(committedTRAC, primaryNode)prorate-seeds the committed TRAC as per-epoch publishing allocation onto that node viaPublishingMathLib.prorateActiveSink, so the seeded total== committedTRACexactly for any mint time (no mint-alignment over-credit, no dust).setPrimaryNodere-designates the node, moving future-epoch allocation net-zero onK_total— rate-limited once per epoch; new node validated vianodeExists; old node intentionally not re-validated (so an owner can always move off a departed node).EpochStorage:producedKnowledgeValue→publishingAllocationrename + a new net-zeromoveEpochPublishingAllocationmutator.RandomSampling._calculateNodeScore: the publishing factor now reads a single current epoch (P = K_n / K_total); window narrowed 4→1 (allocation is a flat annuity, so the smoothing window is vestigial and 1 epoch matches the current-stateS(t)/A(t)terms). Coefficients /S(t)/A(t)unchanged.KnowledgeAssetsLifecycle: the realized-publishK_ncredits (publish and update paths) are removed — no double-feed.P(t)is linear and uncapped, with no node opt-in — by design. Allocation is capital that funds the staker pool, so proportional return is fair, preserves parity with today's linear realized factor, and maximizes the buy-and-commit-TRAC incentive. (Security rationale in the RFC: wash-allocation is self-limiting;K_total"suppression" is non-economic.)Contracts touched (version bumps)
PublishingConvictionStorage10.0.2 → 10.0.3PublishingConviction10.0.2 → 10.0.3DKGPublishingConvictionNFT10.0.2 → 10.0.3EpochStorage10.0.3 → 10.0.4RandomSampling10.0.4 → 10.0.5KnowledgeAssetsLifecycle10.0.5 → 10.0.6Tests
packages/evm-module/test/v10-rfc-51-publishing-allocation.test.ts— 9 passing:== committedTRACfor both boundary-aligned and mid-epoch mints, asserted per-epoch (partial-first / full =committedTRAC/N/ dust-corrected final);K_totalper-epoch net-zero;K_n;calculateNodeScoreimpact — 3:1 allocation → byte-exact score ratio, zero-allocation → baselinec,setPrimaryNodeshifts the future-epoch score;OnlyConvictionNFT,NotAccountOwner,onlyContracts) and theZeroPrimaryNodeguard.Conviction e2e + Profile suites green. (The
RandomSampling.test.tsintegration suite remainsdescribe.skip— a pre-existing V8-stake tombstone, unrelated to this change.)Review
A pre-testnet adversarial review found one blocker —
setPrimaryNode(_, 0)was a reachableK_total-inflation / score-divisor manipulation (it nulledprimaryNodewhile leaving the old node's buckets seeded, enabling a later double-credit;K_totalhas no decrement path). Fixed: rejected withZeroPrimaryNodein both the logic contract and the NFT wrapper. All should-fix items closed.Status / notes
feat/rfc49-public-projection, but the change applies cleanly ontomainand compiles + tests green standalone — so this PR targetsmain.🤖 Generated with Claude Code