Extensible Blockchain Parameters #9402

dimas1185 · 2020-08-14T00:12:17Z

Change Description

Extensible Blockchain Parameters

Background

The basic means of manipulating consensus parameters for a blockchain has been a pair of intrinsic functions:

get_blockchain_parameters_packed
set_blockchain_parameters_packed

These intrinsics are tied to a specific and inflexible definition of what blockchain parameters are and included no convenient means to "version" that set of parameters so that parameters could be added/removed/modified in future consensus upgrades.

Task Description

This PR is to implement a new pair of intrinsics that is intended to eventually supplant the existing intrinsics and provide greater flexibility for future consensus upgrades.

As this task will add intrinsics it MUST be a consensus upgrade itself which, when activated, will allow contracts to link to the new intrinsics. There is a system present in the chain library under whitelisted_intrinsics.cpp|hpp to facilitate additions like these.

This PR support only the blockchain parameters that are already accessible via get|set_blockchain_parameters_packed. Future tasks will add their associated parameters in conjunction with those consensus upgrades.

set_parameters_packed( span<const char> packed_parameters )

This is a new privileged intrinsic that allows a system contract the ability to set parameters in a flexible manner.

The input buffer is a packed data stream which represents an encoded sequence of parameter_id:paramter_value pairs with the following format:

|varuint32:sequence_length | varuint32:parameter_id | <various>:parameter_value | ...

The encoding of parameter_values should be specific to the parameter being set

Notes

It is illegal to have duplicate parameter_ids encoded in the sequence and should result in aborting the transaction context.
Presence of a parameter_id which is unknown OR which is known but tied to an unactivated consensus protocol is illegal and should result in aborting the transaction context.
- future consensus upgrades will need the ability to "add" valid protocol_ids
There is to be no requirement for ordering of items in the sequence.

uint32_t get_parameters_packed( span<const char> packed_parameter_ids, span<char> packed_parameters)

This is a new privileged intrinsic that allows a system contract the ability to get the current values of parameters in a flexible manner.

The input buffer is a packed data stream which represents an encoded sequence of parameter_id pairs with the following format:

|varuint32:sequence_length | varuint32:parameter_id | ...

The output buffer is a packed data stream which represents an encoded sequence of parameter_id:paramter_value pairs with the following format:

|varuint32:sequence_length | varuint32:parameter_id | <various>:parameter_value | ...

The encoding of parameter_values should be specific to the parameter being set

note the output buffer format should be valid input for set_parameters_packed

For each known parameter_id in the input sequence there should be an associated entry in the output sequence with the current encoded parameter_value

Notes

It is illegal to have duplicate parameter_id s encoded in the input sequence and should result in aborting the transaction context.
Presence of a parameter_id which is unknown OR which is known but tied to an unactivated consensus protocol is legal but should not result in additional entries in the output sequence.
- future consensus upgrades will need the ability to "add" valid protocol_ids
- This means that the output sequence length can be less than the input sequence length
There is to be no requirement or guarantee for ordering of items in the input OR output sequence.
It is illegal to pass in a non-zero sized output buffer that cannot completely contain the output and should result in aborting the transaction context
- If callers want to determine the required output size at runtime, they should pass in a zero-sized output buffer and the return value of the call should represent the smallest valid size for a buffer.

New protocol feature
This PR adds new protocol feature called BLOCKCHAIN_PARAMETERS. Protocol feature enables above mentioned intrinsics for privileged contracts.

Acceptance Criteria

In addition to presence of the above intrinsics the following expectations should be met:

unittests guaranteeing that existing system contracts are valid and continue to work before and after the activation of this protocol feature
unittests that demonstrate that calling setcode with WASM that imports these intrinsics fails prior to the activation of this consensus upgrade and succeeds immediately after activation.
uinttests that demonstrate that parameters written with the new intrinsics can be read with the old intrinsics and vice versa

Change Type

Select ONE

Documentation

Stability bug fix

Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

heifner · 2020-08-14T00:57:17Z

libraries/chain/include/eosio/chain/chain_config_helper.hpp

+#include <boost/preprocessor/seq/for_each_i.hpp>      /* BOOST_PP_SEQ_FOR_EACH_I */
+#include <boost/preprocessor/seq/size.hpp>            /* BOOST_PP_SEQ_SIZE */
+#include <boost/preprocessor/seq/elem.hpp>            /* BOOST_PP_SEQ_ELEM */
+#include <boost/preprocessor/repetition/repeat.hpp>   /* BOOST_PP_REPEAT */


Do we really need to add all these macros? Can't we just pack/unpack manually. Seems like a maintenance mess.
Note: not talking about the include files so much as the added macros below.

@heifner tbh, we have a lot of metaprogramming & macro boiler plates in our code (e.g. fc reflect) and I was not so experienced with this stuff so I decided to train a bit to get more familiarity with this in future. I had plans to make some kind of FC_REFLECT_RANGE to make it generic but later I put this in config helper.
So since this code is not going to be reusable like FC_REFLECT we can put there just resolved macros.
I'm really 50/50 on this. Either works for me.

The contract also has to do something similar, right? I assume we are not going to have all of this for the contract code.

@heifner CDT&contracts part is not implemented yet but you can check the unit tests for example of possible implemntation:
unittests/test-contracts/params_test/params_test.cpp

I think I'm in agreement with Kevin here. While a bit tedious, maintaining manual pack/unpack would likely be easier than trying to maintain moderately complex macros.

If you want to make it easy to add the manual code, you could probably run just the preprocessor step and then copy/paste that to help you avoid some typing.

@heifner @jeffreyssmith2nd please check my recent commit

jeffreyssmith2nd

Please change the title to something clearer and provide better details in the description. These are used to generate the Release Notes, it's helpful if they are descriptive.

heifner · 2020-08-14T12:59:54Z

Please change the title to something clearer and provide better details in the description. These are used to generate the Release Notes, it's helpful if they are descriptive.

I would like to add the details here are likely to long outlive whatever is in Jira when we dump Jira and move to something else.

dimas1185 · 2020-08-14T14:51:22Z

Please change the title to something clearer and provide better details in the description. These are used to generate the Release Notes, it's helpful if they are descriptive.

@heifner @jeffreyssmith2nd description was updated.
when you say title, do you mean inline description title or commit message?

heifner · 2020-08-14T14:53:21Z

@heifner @jeffreyssmith2nd description was updated.
when you say title, do you mean inline description title or commit message?

The PR title: "new v1 config, intrinsics, UT"

libraries/chain/protocol_feature_manager.cpp

jeffreyssmith2nd · 2020-08-14T18:28:19Z

libraries/chain/include/eosio/chain/chain_config.hpp

+
+RANGE_PACK(eosio::chain::chain_config_v0, CHAIN_CONFIG_V0_MEMBERS())
+RANGE_UNPACK(eosio::chain::chain_config_v0, CHAIN_CONFIG_V0_MEMBERS())
+//uncomment after adding 1st member to v1 config


Do we need all of this skeleton v1 code?

jeffreyssmith2nd · 2020-08-14T20:06:47Z

libraries/chain/include/eosio/chain/chain_config_helper.hpp

+ */
+template<auto T>
+constexpr size_t field_id(){
+   FC_THROW_EXCEPTION(config_parse_error, "only specializations of field_id can be used");


Is there a way these could be static_asserts instead, making it a compile time failure?

jeffreyssmith2nd · 2020-08-14T20:09:39Z

libraries/chain/include/eosio/chain/chain_config_helper.hpp

+#include <boost/preprocessor/seq/for_each_i.hpp>      /* BOOST_PP_SEQ_FOR_EACH_I */
+#include <boost/preprocessor/seq/size.hpp>            /* BOOST_PP_SEQ_SIZE */
+#include <boost/preprocessor/seq/elem.hpp>            /* BOOST_PP_SEQ_ELEM */
+#include <boost/preprocessor/repetition/repeat.hpp>   /* BOOST_PP_REPEAT */


I think I'm in agreement with Kevin here. While a bit tedious, maintaining manual pack/unpack would likely be easier than trying to maintain moderately complex macros.

If you want to make it easy to add the manual code, you could probably run just the preprocessor step and then copy/paste that to help you avoid some typing.

heifner · 2020-08-17T15:18:53Z

libraries/chain/webassembly/privileged.cpp

+         [&]( auto& gprops ) {
+              gprops.configuration = config_range.config;
+      });
+   }


See comments in slack.

jeffreyssmith2nd · 2020-08-19T19:35:43Z

libraries/chain/include/eosio/chain/chain_config.hpp

+inline DataStream &operator<<(DataStream &s, const eosio::chain::data_entry<eosio::chain::chain_config_v0, eosio::chain::config_entry_validator> &entry){
+   using namespace eosio::chain;
+
+   if (!entry.is_allowed())


Should we consider some sort of logging here to let users know they tried to read something that doesn't exist?

this part will be tested after adding first parameter into v1 config. I'll add logs, good idea.

jeffreyssmith2nd · 2020-08-19T19:36:48Z

unittests/test-contracts/params_test/params_test.cpp

+                           (4_ui)(700_32)
+                           (5_ui)(50_32)
+                           (6_ui)(120_32)
+                           (7_ui)(250'000_32)


Did you determine if these are encoded correctly and just shown weird by Github?

yes, it just weird Github highlight.
I checked, this is regular single quote:

Char Dec Hex Oct

' 39 27 47

new v1 config, intrinsics, UT

7e887fc

dimas1185 requested a review from arhag August 14, 2020 00:17

heifner reviewed Aug 14, 2020

View reviewed changes

jeffreyssmith2nd suggested changes Aug 14, 2020

View reviewed changes

wasm/abi for params_test added to test-contracts

da0f5d6

heifner reviewed Aug 14, 2020

View reviewed changes

libraries/chain/protocol_feature_manager.cpp Show resolved Hide resolved

dimas1185 changed the title ~~new v1 config, intrinsics, UT~~ Extensible Blockchain Parameters Aug 14, 2020

jeffreyssmith2nd reviewed Aug 14, 2020

View reviewed changes

heifner reviewed Aug 17, 2020

View reviewed changes

refactoring: macro magic removed

6aac5af

jeffreyssmith2nd reviewed Aug 19, 2020

View reviewed changes

jeffreyssmith2nd approved these changes Aug 20, 2020

View reviewed changes

dimas1185 merged commit 41eea2b into develop Aug 20, 2020

dimas1185 deleted the config-v1-rc branch August 20, 2020 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible Blockchain Parameters #9402

Extensible Blockchain Parameters #9402

dimas1185 commented Aug 14, 2020 •

edited

Loading

heifner Aug 14, 2020 •

edited

Loading

dimas1185 Aug 14, 2020

heifner Aug 14, 2020

dimas1185 Aug 14, 2020 •

edited

Loading

jeffreyssmith2nd Aug 14, 2020 •

edited

Loading

dimas1185 Aug 18, 2020

jeffreyssmith2nd left a comment

heifner commented Aug 14, 2020

dimas1185 commented Aug 14, 2020 •

edited

Loading

heifner commented Aug 14, 2020

jeffreyssmith2nd Aug 14, 2020

jeffreyssmith2nd Aug 14, 2020

jeffreyssmith2nd Aug 14, 2020 •

edited

Loading

heifner Aug 17, 2020

jeffreyssmith2nd Aug 19, 2020

dimas1185 Aug 20, 2020

jeffreyssmith2nd Aug 19, 2020

dimas1185 Aug 19, 2020

Extensible Blockchain Parameters #9402

Extensible Blockchain Parameters #9402

Conversation

dimas1185 commented Aug 14, 2020 • edited Loading

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

heifner Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

jeffreyssmith2nd Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreyssmith2nd left a comment

Choose a reason for hiding this comment

heifner commented Aug 14, 2020

dimas1185 commented Aug 14, 2020 • edited Loading

heifner commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreyssmith2nd Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 commented Aug 14, 2020 •

edited

Loading

heifner Aug 14, 2020 •

edited

Loading

dimas1185 Aug 14, 2020 •

edited

Loading

jeffreyssmith2nd Aug 14, 2020 •

edited

Loading

dimas1185 commented Aug 14, 2020 •

edited

Loading

jeffreyssmith2nd Aug 14, 2020 •

edited

Loading