Stop unusable keys from freezing the onblock schedule rebuild

contracts/sysio.system/src/finalizer_key.cpp

@@ -17,12 +17,61 @@ namespace sysiosystem {
       return !get_last_proposed_finalizers().empty();
    }
 
-   // Validates finalizer_key in text form and returns a binary form
+   // Validates finalizer_key in text form and returns a binary form.
+   // This checks the prefix and the base64url encoding only -- it says nothing about the point.
    sysio::bls_g1 to_binary(const std::string& finalizer_key) {
       check(finalizer_key.compare(0, 7, "PUB_BLS") == 0, "finalizer key does not start with PUB_BLS: " + finalizer_key);
       return sysio::decode_bls_public_key_to_g1(finalizer_key);
    }
 
+   namespace {
+
+      constexpr auto identity_key_error = "finalizer key must not be the identity point";
+      constexpr auto invalid_key_error  = "finalizer key is not a valid G1 point";
+
+      // The all-zero G1 encoding is the point at infinity, and a proof of possession cannot screen
+      // it out: the pairing bls_pop_verify computes is satisfied by it for free. It must never
+      // become a finalizer key -- aggregating it into a quorum certificate is a no-op on the
+      // aggregate public key while its weight still counts toward the threshold, so its votes
+      // could be cast by anyone.
+      bool is_identity_g1( const sysio::bls_g1& key ) {
+         return key == sysio::bls_g1{};
+      }
+
+      // Whether the bytes are a point on the curve. The host G1 primitives deserialize with
+      // validity checking and report failure, so adding the identity to the key answers this in
+      // one host call. A key that fails here makes set_finalizers throw while deserializing it.
+      bool is_valid_g1( const sysio::bls_g1& key ) {
+         sysio::bls_g1 sum{};
+         return sysio::bls_g1_add( key, sysio::bls_g1{}, sum ) == 0;
+      }
+
+      constexpr auto subgroup_key_error = "finalizer key is not in the r-order subgroup";
+
+      /// Order of the G1 subgroup, little-endian, as bls_g1_weighted_sum reads its scalars.
+      constexpr sysio::bls_scalar g1_subgroup_order = {
+         '\x01', '\x00', '\x00', '\x00', '\xff', '\xff', '\xff', '\xff',
+         '\xfe', '\x5b', '\xfe', '\xff', '\x02', '\xa4', '\xbd', '\x53',
+         '\x05', '\xd8', '\xa1', '\x09', '\x08', '\xd8', '\x39', '\x33',
+         '\x48', '\x7d', '\x9d', '\x29', '\x53', '\xa7', '\xed', '\x73'
+      };
+
+      // Whether `key` lies in the r-order subgroup, tested as [r]P == identity.
+      //
+      // Being on the curve is not enough. A small-order point such as affine (0, 2) -- on the
+      // curve, canonical, and not the identity -- pairs to one against any G2 point, because its
+      // order is coprime to r. bls_pop_verify therefore accepts it with an identity proof, and it
+      // would carry finality weight that anyone could cast, exactly as the identity key would.
+      bool is_in_g1_subgroup( const sysio::bls_g1& key ) {
+         const sysio::bls_g1     points[1]  = { key };
+         const sysio::bls_scalar scalars[1] = { g1_subgroup_order };
+         sysio::bls_g1 product{};
+         if( sysio::bls_g1_weighted_sum( points, scalars, 1, product ) != 0 ) return false;
+         return product == sysio::bls_g1{};
+      }
+
+   } // namespace
+
    // Returns hash of finalizer_key in binary format
    static sysio::checksum256 get_finalizer_key_hash(const sysio::bls_g1& finalizer_key_binary) {
       return sysio::sha256(finalizer_key_binary.data(), finalizer_key_binary.size());
@@ -143,10 +192,20 @@ namespace sysiosystem {
       // Basic signature format check
       check(proof_of_possession.compare(0, 7, "SIG_BLS") == 0, "proof of possession signature does not start with SIG_BLS: " + proof_of_possession);
 
-      // Convert to binary form. The validity will be checked during conversion.
+      // Convert to binary form.
       const auto fin_key_g1 = to_binary(finalizer_key);
       const auto pop_g2 = sysio::decode_bls_signature_to_g2(proof_of_possession);
 
+      // A key set_finalizers would reject must not enter the table. The schedule rebuild proposes
+      // every active finalizer key from inside onblock, and a throw there rolls back the rebuild
+      // timestamp with it, so the gate re-fires and fails again on every block that follows.
+      // Registration is the only action that admits new key material, which makes it the one place
+      // this can be caught; delfinkey is deliberately left lenient so a key that predates this
+      // check can still be removed.
+      check( !is_identity_g1(fin_key_g1), identity_key_error );
+      check( is_valid_g1(fin_key_g1), invalid_key_error );
+      check( is_in_g1_subgroup(fin_key_g1), subgroup_key_error );
+
       // Duplication check across all registered keys
       const auto idx = _finalizer_keys.get_index<"byfinkey"_n>();
       const auto hash = get_finalizer_key_hash(fin_key_g1);

contracts/tests/emissions_tests.cpp

@@ -5764,6 +5764,53 @@ BOOST_FIXTURE_TEST_CASE( active_producers_scheduled, producer_eligibility_tester
 
 // A slashed producer (status SLASHED) is dropped from the schedule; the others
 // remain (4 eligible >= min_schedule_size).
+// The rank walk runs inside onblock, so a schedule it cannot publish stops every future schedule
+// update rather than just this one: update_ranked_producers writes last_producer_schedule_update
+// before proposing, and a throw rolls that write back with the rest of the transaction. A producer
+// holding a key nothing can sign with is the case that used to do it -- regproducer accepts the
+// key, the walk proposes it, and set_proposed_producers rejected the whole schedule.
+BOOST_FIXTURE_TEST_CASE( unsignable_producer_key_does_not_stall_rebuild, producer_eligibility_tester ) try {
+   auto names = setup_ranked_producers(5);
+   trigger_reschedule();
+   for (const auto& p : names) {
+      BOOST_REQUIRE_MESSAGE(is_scheduled(p), "expected " << p.to_string() << " scheduled");
+   }
+
+   // One producer re-registers with a key no signature can ever yield: a well-formed compressed
+   // R1 point prefix over an x coordinate above the field prime. Registration admits it -- it is
+   // an R1 key and it is not all zero, which is as far as the contract can check without curve
+   // arithmetic -- so this is the case the contract guard cannot close.
+   fc::crypto::r1::public_key_data undecodable{};
+   undecodable[0] = 0x02;
+   std::fill(undecodable.begin() + 1, undecodable.end(), '\xff');
+   const fc::crypto::public_key unsignable_key{
+      fc::crypto::public_key::storage_type{std::in_place_index<1>,
+                                           fc::crypto::r1::public_key_shim{undecodable}}};
+
+   const auto bad = names.back();
+   BOOST_REQUIRE_EQUAL(success(), push_system_action(bad, "regproducer"_n, mvo()
+      ("producer", bad)("producer_key", unsignable_key)("url", "")("location", 0)));
+
+   // A block carries a proposer policy diff only when set_proposed_producers accepted the
+   // schedule, so that diff is the rebuild succeeding. Advance one block at a time and stop as
+   // soon as it appears: continuing past it would activate a policy this producer cannot sign
+   // for, and the harness signs every block itself.
+   bool proposed = false;
+   size_t blocks = 0;
+   for (; blocks < 200 && !proposed; ++blocks) {
+      proposed = produce_block()->new_proposer_policy_diff.has_value();
+   }
+   BOOST_REQUIRE_MESSAGE(proposed,
+                         "the rank walk never published a schedule with an unsignable key registered");
+   BOOST_TEST_MESSAGE("proposal landed after " << blocks << " blocks");
+
+   // One landing is proof enough: the stall was per block and permanent, so the row could not have
+   // moved at all unless onblock completed. Recovery beyond this point is not observable here --
+   // the harness holds no private half for an unsignable key and cannot sign that producer's slot
+   // once the policy activates, which is the intended consequence of admitting the key rather than
+   // a chain failure.
+} FC_LOG_AND_RETHROW()
+
 BOOST_FIXTURE_TEST_CASE( slashed_producer_removed, producer_eligibility_tester ) try {
    auto names = setup_ranked_producers(5);
    trigger_reschedule();

contracts/tests/sysio.finalizer_key_tests.cpp

@@ -2,6 +2,8 @@
 #include "finalizer_test_keys.hpp"
 
 #include <sysio/chain/kv_table_objects.hpp>
+#include <fc/crypto/bls_public_key.hpp>
+#include <fc/crypto/bls_signature.hpp>
 #include <sysio/opp/opp.hpp>
 #include <boost/test/unit_test.hpp>
 
@@ -719,4 +721,37 @@ BOOST_FIXTURE_TEST_CASE(verify_controller_schedule_and_policy_test, finalizer_ke
 }
 FC_LOG_AND_RETHROW()
 
+
+// A finalizer key that no proof of possession can screen out: the pairing bls_pop_verify computes
+// is e(-g1, sig) * e(pk, H(pk)), and both terms are 1 when the points are the identity.
+BOOST_FIXTURE_TEST_CASE(reject_identity_finalizer_key, finalizer_key_tester) try {
+   add_roa_policy(NODE_DADDY, alice, "32.0000 SYS", "32.0000 SYS", "32.0000 SYS", 0, 0);
+   BOOST_REQUIRE_EQUAL( success(), regproducer(alice) );
+
+   const std::string identity_key = fc::crypto::bls::public_key::to_string(fc::crypto::bls::public_key_data{});
+   const std::string identity_pop = fc::crypto::bls::signature::to_string(fc::crypto::bls::signature_data{});
+
+   BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key must not be the identity point"),
+                        register_finalizer_key(alice, identity_key, identity_pop) );
+
+   // Bytes with a valid encoding that are not a point on the curve. set_finalizers raises while
+   // deserializing one, before the policy is even validated.
+   fc::crypto::bls::public_key_data off_curve;
+   off_curve.fill(0xff);
+   BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key is not a valid G1 point"),
+                        register_finalizer_key(alice, fc::crypto::bls::public_key::to_string(off_curve), identity_pop) );
+
+   // Affine (0, 2) is canonical, on the curve, and not the identity, but its order is 3. That is
+   // coprime to r, so it pairs to one against any G2 point and the proof of possession above
+   // accepts it -- only a subgroup test rejects it.
+   fc::crypto::bls::public_key_data small_order{};
+   small_order[48] = 2;
+   BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key is not in the r-order subgroup"),
+                        register_finalizer_key(alice, fc::crypto::bls::public_key::to_string(small_order),
+                                               identity_pop) );
+
+   // An honest key is unaffected.
+   BOOST_REQUIRE_EQUAL( success(), register_finalizer_key(alice, key_pairs[0].pub_key, key_pairs[0].pop) );
+} FC_LOG_AND_RETHROW()
+
 BOOST_AUTO_TEST_SUITE_END()

libraries/chain/include/sysio/chain/proposer_policy.hpp

@@ -49,11 +49,9 @@ struct proposer_policy {
 
    // Validates structural well-formedness of the policy. Single source of truth
    // reused by the set_proposed_producers host function and snapshot loading.
-   // Two things are intentionally NOT checked here and stay at the intrinsic
-   // call site instead:
-   //  - account existence (requires apply_context)
-   //  - K1/R1 key type enforcement (uses unactivated_key_type to signal that
-   //    non-K1/R1 keys need a protocol feature; distinct from structural errors)
+   // Account existence is NOT checked here and stays at the intrinsic call site,
+   // which has the apply_context needed for it. Key type and key validity are not
+   // checked anywhere on this path by design -- see set_proposed_producers_common.
    // Throws producer_schedule_exception on violation.
    void validate() const {
       const auto& producers = proposer_schedule.producers;

libraries/chain/webassembly/privileged.cpp

@@ -58,23 +58,20 @@ namespace sysio { namespace chain { namespace webassembly {
          SYS_THROW(wasm_execution_error, "set_proposed_producers: {}", e.top_message());
       }
 
-      // Remaining checks that don't belong in proposer_policy::validate():
-      //   - account existence (requires apply_context)
-      //   - K1/R1 key type enforcement (uses unactivated_key_type to convey
-      //     that non-K1/R1 keys need a protocol feature to be activated — a
-      //     distinct category from structural validation errors)
-      //   - key.valid() semantics
-      using key_type = fc::crypto::public_key::key_type;
+      // Account existence is the only remaining per-producer check; it needs the apply_context, so
+      // it cannot live on proposer_policy::validate(). A schedule naming an account that does not
+      // exist is malformed, not merely unusable.
+      //
+      // 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) {
          SYS_ASSERT(context.is_account(p.producer_name), wasm_execution_error,
                     "producer schedule includes a nonexisting account");
-         std::visit([](const auto& a) {
-            for (const auto& kw : a.keys) {
-               SYS_ASSERT(kw.key.contains_type(key_type::k1, key_type::r1), unactivated_key_type,
-                          "Unactivated key type used in proposed producer schedule");
-               SYS_ASSERT(kw.key.valid(), wasm_execution_error, "producer schedule includes an invalid key");
-            }
-         }, p.authority);
       }
 
       return context.control.set_proposed_producers( context.trx_context,
@@ -175,10 +172,17 @@ namespace sysio { namespace chain { namespace webassembly {
       finpol.finalizers.reserve(abi_finpol.finalizers.size());
       for (auto& f: abi_finpol.finalizers) {
          SYS_ASSERT(f.public_key.size() == 96, wasm_execution_error, "Invalid bls public key length");
-         fc::crypto::bls::public_key pk(std::span<const uint8_t,96>(f.public_key.data(), 96));
-         finpol.finalizers.push_back(chain::finalizer_authority{.description = std::move(f.description),
-                                                                .weight = f.weight,
-                                                                .public_key{pk}});
+         // bls::public_key validates the point in its constructor and raises a bare fc::exception
+         // when the bytes are not on the curve -- which lands outside the catch below. Report it
+         // the way every other input error on this path is reported.
+         try {
+            finpol.finalizers.push_back(chain::finalizer_authority{.description = std::move(f.description),
+                                                                   .weight = f.weight,
+                                                                   .public_key{fc::crypto::bls::public_key(
+                                                                      std::span<const uint8_t,96>(f.public_key.data(), 96))}});
+         } catch (const fc::exception& e) {
+            SYS_THROW(wasm_execution_error, "set_finalizers: invalid bls public key: {}", e.top_message());
+         }
       }
 
       // Structural validation is factored into finalizer_policy::validate() so the

libraries/libfc/include/fc/crypto/bls_private_key.hpp

@@ -67,7 +67,7 @@ class private_key {
 /**
  * @brief Shim class for BLS public key operations
  */
-struct public_key_shim {
+struct public_key_shim : fc::reflect_init {
    using data_type = public_key_data;
 
    /** @brief Checks if the public key is valid */
@@ -97,13 +97,24 @@ struct public_key_shim {
       return *this;
    }
 
+   /**
+    * Reject a deserialized shim whose payload is absent.
+    *
+    * The reflected member is the shared_ptr itself and fc packs a presence flag ahead of it, so a
+    * false flag unpacks to a null pointer that valid(), to_string(), serialize() and unwrapped()
+    * would all dereference. Both constructors allocate, so deserialization is the only way to
+    * reach that state; fc calls this after unpacking a reflected type, which turns it into a
+    * controlled exception where the bytes are read.
+    */
+   void reflector_init() { FC_ASSERT(shim_ptr, "BLS public key has no payload"); }
+
    std::shared_ptr<shim<data_type>> shim_ptr;
 };
 
 /**
  * @brief Shim class for BLS signature operations
  */
-struct signature_shim {
+struct signature_shim : fc::reflect_init {
    using data_type = compact_signature;
 
    /** @brief Indicates if signature is recoverable */
@@ -126,6 +137,9 @@ struct signature_shim {
       return *this;
    }
 
+   /** Reject a deserialized shim whose payload is absent -- see public_key_shim::reflector_init. */
+   void reflector_init() { FC_ASSERT(shim_ptr, "BLS signature has no payload"); }
+
    /**
     * Not supported, throws.
     */

libraries/libfc/include/fc/crypto/elliptic.hpp

@@ -116,6 +116,11 @@ namespace fc {
       struct public_key_shim : public crypto::shim<public_key_data> {
          using crypto::shim<public_key_data>::shim;
 
+         /// Whether the stored bytes are anything other than all zero.
+         ///
+         /// The ecc::public_key constructor taking public_key_data copies without decoding, so this
+         /// does NOT establish that the bytes name a point on the curve -- unlike the R1 shim's
+         /// valid(), which does decode.
          bool valid()const {
             return public_key(_data).valid();
          }

libraries/libfc/include/fc/crypto/elliptic_r1.hpp

@@ -4,6 +4,7 @@
 #include <fc/crypto/sha256.hpp>
 #include <fc/crypto/sha512.hpp>
 #include <fc/crypto/openssl.hpp>
+#include <fc/exception/exception.hpp>
 #include <fc/fwd.hpp>
 #include <fc/io/raw_fwd.hpp>
 
@@ -106,8 +107,18 @@ namespace fc {
      struct public_key_shim : public crypto::shim<public_key_data> {
         using crypto::shim<public_key_data>::shim;
 
+        /// Whether the stored bytes decode to a point on the curve.
+        ///
+        /// Never throws: the r1::public_key constructor raises when o2i_ECPublicKey rejects the
+        /// point, and every caller of this uses it as a predicate. Note the asymmetry with the K1
+        /// shim of the same name, whose underlying constructor only copies -- there valid() is an
+        /// all-zero test and says nothing about the point.
         bool valid()const {
-           return public_key(_data).valid();
+           try {
+              return public_key(_data).valid();
+           } catch (const fc::exception&) {
+              return false;
+           }
         }
      };
 

libraries/libfc/src/crypto/bls_public_key.cpp

@@ -19,7 +19,12 @@ namespace fc::crypto::bls {
       std::span<const uint8_t, public_key_data_size> affine_non_montgomery_le_span = affine_non_montgomery_le;
       std::optional<bls12_381::g1> g1 =
          bls12_381::g1::fromAffineBytesLE(affine_non_montgomery_le_span, {.check_valid = true, .to_mont = true});
-      FC_ASSERT(g1);
+      FC_ASSERT(g1, "BLS public key is not a canonical point on the curve");
+      // check_valid establishes only that the point is canonical and on the curve. A small-order
+      // point such as affine (0, 2) satisfies both and still pairs to one against any G2 subgroup
+      // point, so a proof of possession cannot tell it from a real key -- it would carry finality
+      // weight that anyone could cast. Subgroup membership is a separate test.
+      FC_ASSERT(g1->inCorrectSubgroup(), "BLS public key is not in the r-order subgroup");
       return *g1;
    }
    public_key::public_key(const public_key_data& affine_non_montgomery_le)