Stop unusable keys from freezing the onblock schedule rebuild

[heifner]

update_ranked_producers writes last_producer_schedule_update and then proposes both a producer schedule and a finalizer policy, all inside onblock. Anything that throws in there rolls that write back with the rest of the transaction, so the 120-slot gate re-fires on the next block and fails again — permanently, on every block after. Two different keys could trigger it, and they need opposite fixes. Relaxing the first also let a third key type reach the path, where it turned out to dereference null.

Producer keys: the checks go

set_proposed_producers_common validated every key in every authority. Neither check decides anything there:

• block_state::verify_signee already asserts contains_type(k1, r1) on every key recovered from a block signature, so a producer holding another type can never sign with it.
• A key that decodes to no curve point is recovered from no signature, so it never satisfies the authority in keys_satisfy_and_relevant.

A producer holding an unusable block-signing key burns its rounds, which is already ranking's problem via consecutive_missed_rounds and demotion. Both checks are dropped, on both packing formats.

Upstream removed the same checks from the authority format in EOSIO/eos#8021 — "to allow contracts to call set_proposed_producers_ex in a manner that gives the contract confidence that the intrinsic will not abort the transaction" — and kept them on the legacy producer_key format only "to avoid introducing an unintended consensus change" on an intrinsic already live on mainnet. That constraint does not apply pre-launch, so both formats are lenient and there is one rule rather than two that differ by packing format. The same PR notes the check never established what it appears to: a valid curve point whose private key nobody holds passes it just the same.

r1::public_key_shim::valid() threw rather than answering — the r1::public_key constructor raises when o2i_ECPublicKey rejects the point, so a bare fc::exception escaped past the assert meant to catch it. It returns false now, independent of the schedule path.

Finalizer keys: the check stays, and moves earlier

regfinkey admitted the BLS identity point. A proof of possession cannot screen it out — bls_pop_verify computes e(-g₁, sig)·e(pk, H(pk)), and both terms are 1 when the points are the identity, so an all-zero key with an all-zero proof verifies. Registered as a producer's first key it auto-activates without proposing a policy, and nothing looks at it again until the rebuild does, at which point set_finalizers rejects it and the freeze above begins.

That rejection has to stand. The identity point is not a key that cannot vote, it is a key whose votes anyone can cast: aggregate_public_keys treats it as a no-op on the aggregate while verify_weights still counts its weight from the bitset, so a quorum certificate can claim it voted and verify against the honest signatures alone. Free weight toward the finality threshold, for anyone who can see the policy.

So it is refused at regfinkey instead, which is the only action that writes new key material into _finalizer_keys — actfinkey and delfinkey can only move or remove a key already registered there. Alongside the identity, two further conditions are rejected: bytes that are not a canonical point on the curve, which make set_finalizers raise while deserializing them before the policy is validated at all, and points outside the r-order subgroup.

Subgroup membership is a separate test from being on the curve, and the distinction matters. A small-order point such as affine (0, 2) is canonical, on the curve, and not the identity, yet its order is coprime to r, so it pairs to one against every G2 point — bls_pop_verify accepts it with an identity proof exactly as it accepts the identity key. IETF's BLS KeyValidate specifies deserialize, reject the identity, and check the subgroup; only the first two were enforced.

The subgroup check is also applied centrally, in fc::crypto::bls::public_key, so every path that deserializes BLS material gets it rather than the contract alone. Key generation is unaffected either way: a public key is [sk]G, which is always in the subgroup, so no key our tooling produces can fail these checks — reaching them requires hand-crafted bytes.

delfinkey stays lenient so a key registered before these checks can still be removed.

set_finalizers reported an off-curve key as a bare fc::exception, because the bls_public_key constructor raises outside the catch that converts policy errors. It now reports like every other input error on that path.

An absent BLS payload

Dropping the key-type check lets a BLS key reach the schedule path, and public_key_shim keeps its payload behind a shared_ptr that fc reflects directly — so a false presence flag unpacks to null, and valid(), to_string(), serialize() and unwrapped() all dereference it. A one-key authority slips past proposer_policy::validate untouched, because the first insertion into the uniqueness set compares nothing. The result is process termination rather than a controlled exception.

Fixed at the representation rather than the call site: reflector_init() rejects an absent payload where the bytes are read. That covers every path that deserializes BLS material, not just this one — signature_shim has the same shape and sits in the fc::crypto::signature variant used for transaction signatures, so it was reachable from RPC and peer input independently of producer schedules. The wire format is unchanged and no key-type restriction is restored.

Guards on the shims' constructors are not needed: both allocate, so with deserialization guarded there is no null source for a copy to propagate.

Notes

• The producer-key half is a consensus-behaviour change with no protocol-feature mechanism to gate it behind (is_feature_active is hard-false), so it lands pre-launch.
• protocol_feature_tests/producer_keys asserted that a proposed schedule rejects WA/EM/ED/BLS keys; it now asserts they are accepted. The K1/R1 rule had no coverage on the signing side, where it now lives exclusively — a block re-signed with an EM key covers it.
• Check the BLS pairing result before reading it wire-cdt#121, now merged, makes bls_pop_verify check the pairing return code rather than comparing an uninitialized buffer. It is not required by this PR, which validates the point explicitly before calling it, but it closes the same gap for every other caller. The sysio.system.wasm committed here is unaffected: CI resolves CDT from a published release asset rather than from master, and builds the system contracts only on tag builds, so a rebuild is due once a wire-cdt release carrying Specify a specific version for all dependencies #121 is published.

[huangminghuang]

[Medium] Reject an absent BLS payload while retaining schedule-key leniency

wire-sysio/libraries/chain/webassembly/privileged.cpp

Lines 65 to 72 in 0e965e2

	// Key type and key validity are deliberately NOT checked, on either packing format. Both are
	// per-producer capability facts already enforced where they decide something:
	// block_state::verify_signee rejects any key recovered from a block signature that is not K1
	// or R1, and a key that decodes to no curve point is recovered from no signature at all, so
	// it never matches in keys_satisfy_and_relevant. Checking them here adds no safety and one
	// failure mode: the system contract assembles a schedule from its own tables inside onblock,
	// where a throw rolls the rebuild timestamp back with it and re-fires on every block after.
	for (const auto& p : candidate.proposer_schedule.producers) {

The relaxed common path now accepts every public-key variant without checking its representation. BLS public_key_shim stores its bytes behind a shared_ptr, and raw deserialization permits that pointer to be absent. A one-key authority with this representation passes proposer_policy::validate because the first uniqueness-set insertion performs no comparison. Both legacy and authority packing forms reach this common path.

On the default node configuration, schedule logging calls the BLS shim to_string and dereferences the absent pointer, normally terminating the process rather than raising a controlled fc exception. With info logging disabled, a later equality comparison against the same schedule dereferences it as well.

Please reject an absent BLS payload, or enforce non-nullness during deserialization, without restoring K1/R1-only or curve-validity restrictions. A regression test should cover both packing formats. Reaching this path requires privileged intrinsic input, which is why I consider it medium severity.

[huangminghuang]

Requesting changes based on the medium-severity issue described here: #632 (comment)

[heifner]

Confirmed and fixed in a446bad. FC_REFLECT(public_key_shim, (shim_ptr)) reflects the pointer itself, and fc's unpack resets it on a false presence flag, so all four accessors dereference null.

Fixed at the representation rather than the schedule call site: reflector_init() on both public_key_shim and signature_shim rejects an absent payload where the bytes are read. That covers every path that deserializes BLS material, not just this one — signature_shim has the same shape and sits in the fc::crypto::signature variant used for transaction signatures, so it was reachable from RPC and peer input independently of producer schedules. No key-type or curve-validity restriction is restored, and the wire format is unchanged.

Guards on the constructors turned out to be unnecessary: both allocate, so with deserialization guarded there is no null source to copy from.

Tests: crafted bytes for a BLS public key and a BLS signature must throw rather than crash, with a real key still round-tripping; and both packing formats at the schedule layer — vector<producer_authority> and vector<legacy::producer_key> — built by packing a real key and stripping the payload so they survive encoding changes. Verified load-bearing: with reflector_init stubbed out they fail with "exception fc::exception expected but not raised".

[huangminghuang]

Blocking findings remain in the finalizer-key fix: the new check does not enforce G1 subgroup membership, and retained pre-upgrade keys can still become active without validation. I also noted that the schedule liveness regression stops before block publication. The branch currently conflicts with master; conflict resolution must preserve the current master constraints as well.

[heifner]

Merged master and rebuilt the contracts for the wasm conflict, which was the only conflicting file.

Subgroup [P1] — confirmed and fixed in 08b44d2. Reproduced it first: with the check disabled, regfinkey accepted (0,2). The bls_g1_add probe cannot catch it, since check_valid and inCorrectSubgroup() are separate calls in the vendored library. Enforced in fc::crypto::bls::public_key, which covers every path that deserializes BLS material, and again at regfinkey as [r]P == identity so such a key never reaches onblock. Regressions at both layers using your vector.

Worth recording that key generation cannot produce one: a public key is [sk]G, always in the subgroup, so this requires hand-crafted bytes rather than any tooling output.

Finish the block [P2] — you were right, and more than stated. Block assembly is where the logging from your earlier finding happens, so the tests were missing exactly the path that mattered. Both now produce the block and assert new_proposer_policy_diff.

Added the rank-walk regression in d05e18d. A scheduled producer re-registers with an undecodable R1 key, which registration admits because master's check is a non-zero-bytes test and the key is a non-zero R1 point, and the walk must still publish. Verified load-bearing: with the removed validity check restored, no schedule publishes across 200 blocks; with the fix, one lands after 58. It stops at the proposal deliberately — past that the policy activates, and a harness that signs every block itself cannot sign a slot belonging to a producer whose key nothing can sign with. That is the intended consequence of admitting the key, not a chain failure.

Retained keys [P1] — I do not think this one applies here. There is no chain and no existing data, so no pre-upgrade rows can exist, and regfinkey is the only writer of new key material, which means every row present is validated. The path you describe is real as code, but reaching it needs retained invalid state.

Where I agree: it stops being moot the moment validation rules change again on a running chain, since keys registered between two rule changes are exactly the retained-unvalidated case. That seems worth building before launch, as upgrade-safety machinery rather than as part of this PR. Happy to be persuaded otherwise if you see a path to it that does not depend on pre-existing rows.

[huangminghuang]

Re-reviewed current head d05e18d22969cae97e6a2349e43c73e7de63b0f9. The subgroup-membership fix and the schedule-publication regressions now look correct, and I resolved those two threads. One blocking liveness/upgrade issue remains: retained pre-upgrade finalizer keys can still become active without validation, as detailed in #632 (comment). The branch also now conflicts with current master in the generated sysio.system.wasm; resolving it must rebuild the contract with both source changes. Please also refresh the PR description, which still describes curve-only validation and registration as the sole admission path.

[heifner]

Merged latest master and rebuilt the contract; the wasm was again the only conflict. Description refreshed — it described curve-only validation and was loose about admission paths, both now corrected.

Retained keys — still declining, and here is the concrete reason rather than just "pre-launch".

You are right about the mechanism: the central constructor does make a later set_finalizers throw deterministic rather than closing the gap. That part I accept. But reaching it needs a bad row in _finalizer_keys, and there is no path to one.

regfinkey is the only action that writes new key material; actfinkey and delfinkey only move or remove a row already there. Before the system contract is deployed, sysio.bios has no regfinkey or actfinkey at all — its only finalizer action is setfinalizer, which is require_auth(get_self()). So during bios there is no permissionless registration path to abuse, and once the system contract is deployed it is this one, with the guard on the only writer.

The window you are pointing at — an old contract accepting a key a new node would reject — requires a chain already running a prior system contract version. There is no such chain.

I also cannot write the retained-state regression you asked for. With regfinkey guarded there is no API path to plant a bad row, so an activation-time check would be unreachable code with no possible test, which is a poor trade for the liveness path it sits on.

[huangminghuang]

Re-reviewed exact head 829d76dda9027a89c16176cb5262662ae77abb34, excluding the retained pre-upgrade-key concern from the approval decision as requested. No other issues found. The merged sources and rebuilt sysio.system.wasm preserve both the WIRE-360 onlinkauth removal and this PR's identity/on-curve/subgroup validation, the producer-schedule paths and publication regressions look correct, the branch is mergeable with current master, and all required checks passed.