Add support for storing public keys on-chain that will be used to validate a network peer's identity. #185
Conversation
|
@arhag @ericpassmore It would be very nice if this could make it into release 3.8 of It is needed for AntelopeIO/spring#1169. Resolved: this will be in the next version (April or later) |
…tracts into gh_184
…tracts into gh_184
…ontracts into gh_184
| struct [[eosio::table("peerkeysver"), eosio::contract("eosio.system")]] peer_keys_version { | ||
| uint64_t version; // version incremented every time peer_keys_table is modified | ||
|
|
||
| uint64_t primary_key() const { return 0; } // table has just one row |
There was a problem hiding this comment.
Usually for a single row table the pattern is to use a singleton. For example, the globals,
https://github.com/eosnetworkfoundation/eos-system-contracts/blob/c66e447b7883d457557398907be5ad72f808fe86/contracts/eosio.system/include/eosio.system/eosio.system.hpp#L454-L460
There was a problem hiding this comment.
I was a little worried about singleton since you wrote that:
find()s are one of the most expensive host functions a contract can call, and cdt's singleton impl is woof in this regard. eosio system contract: 12 find()s on every single action to deal with singletons
But if you say singleton is better I'm happy to change.
There was a problem hiding this comment.
singleton really seems like the right pattern here
The performance woof can be resolved in cdt fairly easily some day. Actually, we can probably resolve the problem in nodeos too by short circuiting redundant lookups in the iterator cache.
| }; | ||
|
|
||
| struct [[eosio::table("peerkeysver"), eosio::contract("eosio.system")]] peer_keys_version { | ||
| uint64_t version; // version incremented every time peer_keys_table is modified |
There was a problem hiding this comment.
I think you can get around the need for tracking this (and the need for the table as a whole) by instead storing the block num a row is modified on. It's not a change I really feel strongly about, but it would be more efficient.
There was a problem hiding this comment.
Good idea, but I thought that, in addition to the version, I would also track the number of deleted keys.
There was a problem hiding this comment.
ah yes that is true, deletion is more tricky that way
There was a problem hiding this comment.
by instead storing the block num a row is modified on
Is there actually a way to get the current block number in a contract? It would be nice to have it to resolve the fork switch issue, but from this discussion I wonder if it is available?
There was a problem hiding this comment.
yep it was added in the GET_BLOCK_NUM protocol feature
Also reduct `num_accounts` in test to `100`
| struct [[eosio::table("peerkeys"), eosio::contract("eosio.system")]] peer_key { | ||
| name proposer_finalizer_name; | ||
| uint32_t block_num; // block number where this row was emplaced or modified | ||
| uint8_t version; // version 0 and above must have the `key` optional |
There was a problem hiding this comment.
Comment doesn't match my understanding of the prior discussion. The reason we have a version is to do the opposite of the comment: it's here to give us the option to remove the optional<key> (and any other future binary extensions). Bumping the version allows a complete 'reset' of the serialization contents.
Also from an ABI standpoint, it's probably best practice to go ahead and make it a variant even though there is just a 'v0' now. The reason is once the version needs to be bumped in the future the only way the ABI can represent both simultaneously is via a variant. Transitioning to a variant in the future would break consumers who can only handle v0 since the version field would disappear from the ABI. This situation is likely academic considering the use case for this table; I suggest it mainly because I think system contracts should follow best practices.
There was a problem hiding this comment.
Hopefully this is what you had in mind.
|
Note:start |
Resolves #184.
Two new user-facing eosio actions are added to the system contract:
regpeerkeyanddelpeerkey.The intent of these is to allows block producers to register a public key (associated to the producer's
account_name), which will be used to authenticate gossip messages sent by nodes managed by this producer. Once this is done, these nodes, provided they belong to one of the top 50 active producers, will be allowed into a gossip network managed bynodeoswhich disseminates the network addresses of these active producer nodes.This allows block producers to maintain a connections to all other active block producers, which is desirable since the Savannah consensus mechanism requires signatures from a quorum of block producers for each block.
regpeerkey: allows a block producer to register a public key for a producer account name. A block producer can only have have one public key registered at a time. If a key is already registered for anaccount_name, andregpeerkeyis called with a different key, the new key replaces the previous one in the system contract table.delpeerkey: allows to remove a public key from the system contract table. The main purpose is to reclaim the ram used, as the key may persist innodeosmemory. An existing public key for a given account can be changed by callingregpeerkeyagain.One new action (to be used internally by Spring as a readonly action) is added to the system contract:
getpeerkeysgetpeerkeys: Returns a list of top-50 producers (in rank order), along with their peer public key if it was added via the regpeerkey action.