Make updating fees and chain_parameters take a single fee / parameter at a time via static_variant

[vikramrajkumar]

From @theoreticalbts on February 4, 2016 22:25

Currently the committee_member_update_global_parameters_operation takes a monolithic structure containing all chain parameters and fees. This is incredibly brittle:

• New fees / parameters have to be implemented as extensions. While the extension<T> template introduced by cryptonomex/graphene#516 implementation makes this fairly straightforward, it still inevitably involves some amount of awkward boilerplate.
• Unnatural semantics. Usually people want to make a committee proposal of the form "change parameter/fee X and leave all other parameters the same." However this is not possible, you can only have a proposal of the form "set all parameters/fees to these values". Consequences:
• UI's are harder to code (since now you have to essentially implement diffing of these structs in the UI code)
• If a proposal to change fee A to X is created, then another proposal to change fee B to Y is created, then the committee cannot approve both proposals; whichever proposal goes through last will undo the other proposal! Worse, a UI which only shows diffs actually implies diff semantics, so if the UI shows that the first proposal will change A to X and the second proposal will change B to Y, it is very counter-intuitive that A will be changed back to its previous value by the second proposal!
• FBA requires some fees to be settable by someone other than the committee. It is hard to write code to inspect the current data structures to determine who has permission to do a particular fee update.

Here is a plan for a migration which can address these shortcomings:

• Rename fee_parameters to fee_parameter.
• blockchain_fee_update_operation takes a fee_parameter object which is a static_variant of all possible fee types.
• Replace each chain parameter with a single-member struct.
• blockchain_parameter_update_operation takes a chain_parameter object which is a static_variant of all such struct
• The current serialized versions of fee_schedule and chain_parameters are put into a new include/graphene/chain/protocol/legacy directory and graphene::chain::protocol::legacy namespace
• New versions of fee_schedule and chain_parameters go into include/graphene/chain which can be updated with new fields without changing serialization; new code containing new fees / parameters will consider them to already exist and have their default values, they will merely be immutable until the hardfork date passes (i.e. blockchain_fee_update_operation / blockchain_parameter_update_operation targeting new fee/parameter values should fail evaluation until the hardfork date arrives; by the hardfork date, most witnesses will have upgraded and will understand the new static_variant members when they appear on the wire)
• The GPO will still be a monolithic object that includes all fees and parameters, wallets shouldn't have to change their fee determination code as long as they ignore unknown keys in the JSON

Copied from original issue: cryptonomex/graphene#554

[vikramrajkumar]

From @theoreticalbts on February 4, 2016 22:27

Also add to this: In a future patch, stop accepting the old operations some time after the new operations have been enabled and we've verified clients can use them. A separate issue to remind us to do this should be filed when this ticket is closed.

[vikramrajkumar]

From @theoreticalbts on February 5, 2016 17:55

All existing fee parameters and chain parameters happen to be integers -- uint, bool or share_type -- and that seems likely for future parameters as well.

Essentially the "set fee operation" or "set parameter operation" is a committee action which (when consensus is achieved) writes an integer to a particular location, which is then read by chain logic.

This is similar to software driver which configures hardware by writing to memory-mapped registers -- the software (set parameter operation) writes a value which is read by chain logic (hardware) to make the chain (hardware) act in a certain way.

So we conceptually the new unified fee / parameter change operation can be:

write( parameter_space_id, pid1, pid2, parameter_value )

where parameter_space_id = 0 for global chain parameters and parameter_space_id = 1 for fees. The pid1 and pid2 specify the place to write:

• In the case of parameters, pid1 specifies the parameter and pid2 is 0.
• In the case of fees, pid1 specifies the operation and pid2 specifies which of the fee parameters for the operation to set.

We could conceivably combine pid1 and pid2 into a single integer field, but keeping them separate allows an easily explainable numbering convention.

Setting of GPO / fee structure fields by pid1 and pid2 can mostly be automated with existing fc reflection and some templates. The existing fee structures should be moved to legacy namespace.

[vikramrajkumar]

From @theoreticalbts on February 5, 2016 18:5

The reason to do things as described in the above post is to have an unchanging serialization which doesn't require separate support for old and new structures. When replacing an old parameter / fee structure with a new structure, the objects get new fields and the JSON dictionaries seen by clients get new keys. The new code will still understands previous operations.

[vikramrajkumar]

From @abitmore on February 6, 2016 5:27

Good idea. Not only this operation, but also the blacklist/whitelist features of account_update and asset_update should be considered this way.

[vikramrajkumar]

From @theoreticalbts on March 14, 2016 17:35

Here's the beginnings of progress. I'm going to shelve this for now as I've spent way, way too much time on this issue and haven't even gotten something close to compiling yet.

[vikramrajkumar]

wontfix

[jmjatlanta]

Here are my design thoughts, a summary of the above, to be sure I understand the change. In other words, these are breadcrumbs that I will refer to and refine as I work through this issue. There are probably errors and misleading statements here. Do not trust.

The existing monolithic structure is inflexible. Serialization issues prevent its expansion. So an intermediate solution has been proposed that would allow for migration without leaving too many artifacts to clean up later.

The basics: The big struct is still a big struct. Clients will see the same struct as they are used to, it will just have new fields that they will not be familiar with. They will not notice unless their json parser does not ignore unknown fields ( I assume most do ).

Updating that big struct via proposals will now become much easier and flexible. Adding parameters (while probably still requiring a hardfork) will be less cumbersome.

Outstanding question: During the implementation of htlc, it was required to put the additional configuration parameters within the extension. I have yet to completely understand why. I am fairly certain it has to do with serialization, but that does not reconcile in my head (yet) as to why that is not okay, but this issue mentions that older clients will be okay.

[pmconrad]

Keep in mind that serialization is not only about JSON but also about "raw" format as used on the wire and on disk. You can't simply add fields because that will break raw serialization.

[abitmore]

So perhaps better to use a struct for objects to be stored in object database, and use another extendable struct for operations, and map the fields. (YOYOW's approach)

[abitmore]

My understanding on this issue:

• we'll deprecate the old committee_member_update_chain_parameter_operation and create a new operation which is more flexible to do the same job;
• we'll add new fields to the chain_parameters struct directly, so calls like get_global_properties don't need to change, and the new operation will be able to handle the new fields (and also even more new fields added in the future) gracefully;
• to keep compatible with old on-chain data (existing blocks), we'll save the old struct somewhere and update the old operation to use the old struct for block serialization only (which means we may need to update evaluator of old operation to update the extended chain_parameters struct with old struct as input).

[pmconrad]

Simpler (?) alternative:

• keep ...update_chain_parameter_operation, but add an extension flag delta to chain_parameters
• use separate db_chain_parameters structure for internal database
• add logic to update db_chain_parameters from chain_parameters in update operation: if delta flag is present, only add/modify fields from operation that are present (fees) and non-zero (normal parameters)
• add new parameters as extensions to chain_parameters and as regular fields to db_chain_parameters

[abitmore]

@pmconrad your suggestion makes sense as well. The pro is it saved a new operation. The con is we still need a big operation even when only need to update one field (smaller than we have now though).

[pmconrad]

IMO having a couple of additional bytes on chain in a rarely used operation is hardly worth another operation.

[jmjatlanta]

TL;DR version-> I am still looking into this, and am hoping more experience will lead me to good answers.

I now see why this ticket has been open for so long. It seems to be straightforward, but there are a number of details that add complexity.

Peter's suggestion solves the problem with minimal fuss. It feels hackish, in my opinion, and it does not get rid of the extra effort of adding extensions. But such code (thus far) is not touched often. And parameter adjustments are also not a common occurrence. In fact, to my knowledge, one of the original problems (dirty write by 2 unintentionally competing proposals) has never happened.

Abit's suggestion requires more up-front effort, and in some respects, makes ongoing maintenance easier. It avoids the use of the extension collection, which is a big plus in my opinion. His point 3 above looks scary, and I admit I do not understand all of its implications. So this throws into question my "easier ongoing maintenance" comment.

The idea of creating a proposal and passing a collection of parameters to modify is appealing. Adding a parameter to the existing struct of parameters without worry of legacy code or objects is also appealing. But such things may be only a panacea and may not be practical.