eosio.system Savanna: vote-accounting, EVM-vote auth, and onblock/kick robustness (builds on #62)#64
Open
TheJudii wants to merge 20 commits into
Open
eosio.system Savanna: vote-accounting, EVM-vote auth, and onblock/kick robustness (builds on #62)#64TheJudii wants to merge 20 commits into
TheJudii wants to merge 20 commits into
Conversation
Positional comparison could spuriously rebuild schedule metrics (wiping missed-block counters) when producers sharing a location value swapped sort order, or when a producer changed location without membership changing. All consumers of producers_metric are name-keyed, so compare sorted name sets instead. Also document why set_proposed_finalizers is not gated on set_proposed_producers success.
Deferred transactions are disabled under Spring/Savanna: the deferred refund in changebw and the deferred bidrefund in bidname would silently never execute, and may hard-fail once DISABLE_DEFERRED_TRXS protocol features are active. Refunds remain in their tables and must be claimed explicitly via the existing refund/bidrefund actions, matching upstream reference-contracts. The PR's test changes already assumed this behavior.
Assert expected eosio.system artifact hashes only on workflow_dispatch and release branches so regular PRs do not fail CI on every legitimate contract change; hashes are still printed on every run. Pin TELOSZERO_REF to the commit SHA of teloszero-v1.2.2 since tags are mutable.
Covers regfinkey validation (registration, PoP, duplicates), the actfinkey/delfinkey lifecycle, switchtosvnn authorization and guard checks plus the one-way transition, and a regression test asserting schedule metrics survive no-op schedule proposals under Savanna (the autokick fix). Authored without a local Spring build; needs a CI run to verify.
Rebuilt with CDT 4.1.1 after the schedule-metrics and deferred-refund fixes. ABI hash is unchanged (5545f53b...) confirming the fixes are ABI-neutral; the WASM hash necessarily changed with the code. Verified reproducible across two independent builds.
missed_block_autokick_threshold_and_lifetime: simulates a scheduled producer missing every production window until the missed-block threshold (648 for a 21-producer schedule) is crossed, then asserts the producer is kicked exactly once with REACHED_TRESHOLD and that lifetime_missed_blocks is counted a single time - regression for the double-count fixed in this branch. schedule_metrics_survive_location_swap: swaps two producers' locations so the proposed schedule reorders without membership changing, then asserts the schedule metrics vector retains its original order (i.e. was not rebuilt) - regression for the membership-based comparison.
Ports AntelopeIO/reference-contracts tests/eosio.finalizer_key_tests.cpp (17 cases, 1194 assertions) - the suite EOS mainnet's Savanna transition was validated with - adapted for Telos: - get_row_by_id replaced with get_row_by_account-based lookup - switchtosvnn shortfall error: Telos's per-producer guard fires before the aggregate count check - finalizers-replaced test rewritten for Telos semantics (BP rotation above 21 candidates; schedule freezes when keyed candidates drop below the last schedule size instead of refilling from standbys) All 17 cases pass against the Telos contract.
…erflow, onblock get_balance abort, Savanna key-deletion freeze Resolves the contract-side findings from the security audit of this branch (see AUDIT_telos_system_contract.md). Node-side findings (BLS length overflow, QC bitset OOB) are addressed in a separate teloszero-core PR. voting.cpp (findings #1/#2/#3 - latent CRITICAL chain halt, pre-existing): - Clamp _gstate.total_producer_vote_weight to >= 0 at both mutation sites (update_votes and propagate_weight_change), mirroring the per-producer clamp and the EVM-vote path. Without this the discarded negative mass leaked into the unclamped global and could drift it below the recalculate_votes() `<= -0.1` trigger. - Make the recalculate_votes() replay non-aborting (new `recalculating` flag): it runs inside onblock, so a check(false) on a stale inactive producer or a deregistered proxy left in a voter row would permanently halt block production. Skip such entries instead of asserting. eosio.system.cpp (findings #4/#5 - EVM-vote bridge, dormant on mainnet): - require_auth(get_self()) on getevmvote and setbpevmstat (were permissionless; they mutate consensus-relevant vote weight / send system EVM txs). - Fix the uint64 underflow in the vote delta: subtract smaller-from-larger and carry the sign explicitly, instead of double(current - previous) which wrapped to ~1.84e19 on any vote decrease and inflated total_votes. - Documented the remaining partial-sync caveat (full per-producer EVM-component redesign deferred; auth gate removes attacker reachability meanwhile). finalizer_key.cpp (finding #9 - Savanna schedule/finalizer freeze, NEW): - Reject delfinkey of an active producer's last finalizer key under Savanna; require unregprod first, so the keyed-producer set cannot silently fall below last_producer_schedule_size via key deletion. producer_pay.cpp (finding #10): - Non-aborting tedp core-balance lookup in claimrewards_snapshot()/pay() so a missing balance row cannot abort onblock. system_kick.cpp (finding #11): - Clamp the block_counter_correction subtraction instead of letting the uint32 underflow wrongly autokick a producer. tests: - evm_vote_sync_actions_require_system_auth (getevmvote/setbpevmstat auth). - delfinkey_blocked_for_active_producer_under_savanna. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TheJudii
changed the title
… checks, whitespace finalizer_key.cpp: the delfinkey guard added earlier blocked ANY active producer from deleting its last finalizer key, which also blocked the normal finalizer- replacement flow (and broke update_elected_producers_finalizers_replaced_test). Scope it to the actual freeze condition: block only when deleting would drop the number of keyed, active, voted producers below last_producer_schedule_size (i.e. no replacement to keep the schedule full). When enough other keyed producers remain, the deletion is allowed and the schedule replaces this one. Update the Telos delfinkey regression test message to match. eosio.system.cpp: in setvotedecay and setbpevmstat, check that the eosio EVM account row exists before reading ->address (previously dereferenced the iterator before the existence check). tests: strip trailing whitespace flagged by `git diff --check`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
voting.cpp: move the `total_producer_vote_weight >= 0` floor to a single check after the producer_deltas loop instead of clamping per iteration. The map is iterated in name order, so on a set-changing re-vote a negative (old-producer) delta can be applied before its offsetting positive (new-producer) delta; a per-iteration clamp would discard that negative mass on a transient zero-crossing and leave the global biased upward. Flooring once after the loop gives the exact net while still keeping the global from drifting below the recalculate_votes() trigger. Behaviour is unchanged whenever the running total never crosses zero mid-loop (e.g. mainnet, where the aggregate is far from zero). producer_pay.cpp: correct a comment -- the TEDP account constant is exrsrv.tf, not "eosio.tedp". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TheJudii
force-pushed
the
codex/savanna-audit-remediation
branch
from
88d392a to
e0aff4e
Compare
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.
Builds on #62 and tightens up a handful of correctness and robustness issues in
eosio.systemthat came out of going back over the Savanna branch and the code around it. Targetsmainwith the same Savanna feature set as #62, just with these fixes on top. The matching node-side changes are in telosnetwork/teloszero-core#7.Nothing here touches the ABI — no new or changed actions or tables — so the ABI hash is unchanged from #59/#62 and only the wasm differs.
Vote-weight accounting (
voting.cpp)Each producer's
total_votesalready gets clamped at zero, but the same delta was being added to the globaltotal_producer_vote_weightwithout that clamp, so the aggregate could slowly drift negative over time. That matters becauserecalculate_votes()runs fromonblockand keys off the global — and on its replay it couldcheck()-fail on a voter that still points at a producer which has since gone inactive, or a proxy that's since been removed. You really don't want acheck()failure on theonblockpath.So two things: clamp the global in both spots it's updated (
update_votesandpropagate_weight_change, the same way the per-producer total and the EVM path already do), and make the replay skip stale producers/proxies instead of asserting on them.EVM vote sync (
eosio.system.cpp)getevmvoteandsetbpevmstatwere callable by anyone, even though they move producer vote weight around (and, under Savanna, feed finalizer selection) and send system-signed EVM transactions — so they now require the system account. The per-BP delta was also being computed ascurrent - previouson unsigned values, which gives the wrong answer when a BP's EVM total goes down; that's now a signed difference.This whole path is dormant on mainnet today (
setvotecontris unset), and it should stay disabled until the EVM-derived component is tracked separately from native vote weight — there's a note inline to that effect, and a couple of brittle iterator checks in the EVM helpers were tidied up while I was in there.Finalizer keys (
finalizer_key.cpp)Under Savanna, if too few producers hold finalizer keys the schedule and finalizer-policy updates stop progressing. A producer deleting its last key can cause that — but only when there's nobody to take its place. So
delfinkeynow refuses to delete an active producer's last key only when doing so would leave fewer keyed producers than the schedule needs; otherwise the normal replacement flow goes through exactly as before.onblock robustness
claimrewards_snapshot()/pay()no longer abort ifeosio.tedphappens to have no core-token balance row — a missing row is just treated as a zero balance, which is what the surrounding code already assumes when the balance is zero. And the missed-block correction subtraction is clamped so it can't wrap around and miscount a producer's missed blocks.Tests
Added coverage for the EVM-sync authorization and the
delfinkeyrule, and the existing finalizer-key / autokick / rotation suites still apply.Deliberately left for later
Separating the EVM vote component from native weight (and keeping EVM voting off until that lands), any change to the
update_elected_producersearly-return, and theregproxyrow-creation cleanup. None of these block this PR.