PR for BSIP 69 "Assert Predicates" #183
Conversation
Remove `head_block_time_lt_time_predicate` since it's equivalent to a transaction expiration, and remove `head_block_height_...` predicates due to lack of a known use case at this time.
bsip-0069.md
Outdated
|
|
||
| ### Impacted committee parameters | ||
|
|
||
| An affected committee parameter is `max_predicate_opcode`, which limits the number of defined predicates supported by the assert operation. If new predicate types are added to the core protocol, this parameter will need to be increased in order to “activate” the new predicates on the network. Thus, the committee should be ready to increase this parameter if this BSIP finds support and is implemented. Using a committee parameter to activate protocol additions after block-producer adoption is an alternative to setting a hard-fork date for activation. |
There was a problem hiding this comment.
Nice find.
IMO we should do away with that parameter. We still will have to add hardfork protection because old nodes can't deserialize the new predicates, and the existing check does not prevent new assertions from being included in proposals.
The parameter currently has value "1" but we have three predicates defined (see above). This means that the block_id predicate currently cannot be used. I do not think this is intentional. (Original code allowed nodes to skip evaluation of unknown predicates. Later, this was replaced by the max_predicate_opcode flag. When the block_id predicate was added, increasing the default max_opcode seems to have been overlooked.) When removing the check, we need to be careful to not change history (the block_id predicate could be present within a proposal).
Please add the removal of the check to the "Specification" section. What do you think @abitmore ?
There was a problem hiding this comment.
Please add the removal of the check to the "Specification" section. What do you think @abitmore ?
Would like @ryanRfox and perhaps others' takes on this. We had some discussion on this during a previous core call. I feel a little apprehensive "stealing" a committee parameter away from them. But... if it's reason for being is no longer relevant,... then I guess so. Is it appropriate to "bundle" removal of this parameter into this BSIP? Or should it be it's own?
There was a problem hiding this comment.
The predicates need to be approved by BSIP approval vote (Community) and protocol upgrade code (Block Producers), there fore I do not see the need for the Committee to also "activate" them. Removing this ability is intentional, but I feel reflects the process we currently use for all other protocol upgrades. Perhaps we should seek some comments from current Committee members such as @bitcrab @xeroc @roelandp @OpenLedgerApp @clockworkgr
There was a problem hiding this comment.
I agree with @ryanRfox
The committee has nothing to do with blockchain features perse.
There was a problem hiding this comment.
Text has been updated to propose removal of max_predicate_opcode.
bsip-0069.md
Outdated
|
|
||
| ### Activation of new predicates and removal of max_predicate_opcode parameter: | ||
|
|
||
| A hard-fork date shall be chosen for activation of these new predicates and shall be sufficiently future-dated to allow time for nodes to upgrade. The outdated committee parameter `max_predicate_opcode` shall be removed, as it is not needed for safe upgrade (see next section for discussion). |
There was a problem hiding this comment.
Technically we're unable to remove the max_predicate_opcode field since it affects serialization of certain operation. We can have a hard fork to no longer use the parameter. And we can check whether the old guarding code can be removed, primarily due to proposals.
There was a problem hiding this comment.
Perhaps "disabled" instead of "removed".
There was a problem hiding this comment.
Not sure I know the specifics enough as to why it can't be removed (I guess any historical transactions that updated the parameter would become invalid if it was removed?), but I updated to say "disable, and, if possible without impacting history, remove". Hopefully this covers sufficient latitude for voting purposes.
|
I think that's it for edits now. I just need someone to give me "done" confirmation and I'll merge it. |
5 participants
Discussion: #175