Merge release branch to master branch for 4.0 release #2093
Conversation
libraries/plugins/custom_operations/custom_operations_plugin.cpp
Outdated
Show resolved
Hide resolved
libraries/plugins/custom_operations/custom_operations_plugin.cpp
Outdated
Show resolved
Hide resolved
libraries/plugins/custom_operations/include/graphene/custom_operations/custom_objects.hpp
Outdated
Show resolved
Hide resolved
libraries/plugins/custom_operations/include/graphene/custom_operations/custom_objects.hpp
Outdated
Show resolved
Hide resolved
libraries/plugins/custom_operations/include/graphene/custom_operations/custom_objects.hpp
Outdated
Show resolved
Hide resolved
libraries/chain/include/graphene/chain/custom_authority_object.hpp
Outdated
Show resolved
Hide resolved
| adjust_balance( obj.transfer.from, amount ); | ||
| // notify related parties | ||
| htlc_refund_operation vop( obj.id, obj.transfer.from, obj.transfer.to, amount, | ||
| obj.conditions.hash_lock.preimage_hash, obj.conditions.hash_lock.preimage_size ); |
There was a problem hiding this comment.
Simply adding fields to the vop does not automatically notify new parties unless also added code to get_impacted_account_visitor::operator()( const htlc_refund_operation& op ) in db_notify.cpp. However, if not implemented carefully, account history (2.9.x) numbering would be affected which would break 3rd client apps who rely on consistent numbering. The careful approach is similar to what's done in #1629.
There was a problem hiding this comment.
I thought I added them to _impacted. Will do. I will also add the boolean based on HF date to prevent account history numbering issues.
There was a problem hiding this comment.
Upon further research,
- I didn't write this, so that explains why I didn't add the necessary code :-)
- the original ticket mentions it not needing a hard fork since the virtual op does not involve consensus. Therefore we have an issue of when should we turn on this feature. My suggestion would be to make it "hardfork-like" and wrap it with hardfork protections tied to BSIP 64.
Your thoughts please @abitmore
There was a problem hiding this comment.
Thanks for the link to the original ticket. Just realized that what I wrote in the reviews are actually new features. So, I think we will only work on this if we have enough time.
Yes, my thought was to make it hardfork-like. For implementation, I'd like to refactor Nathan's code, pass in a time rather than several booleans, write different logic for different operations.
libraries/chain/htlc_evaluator.cpp
Outdated
| htlc_redeemed_operation virt_op( htlc_obj->id, htlc_obj->transfer.from, htlc_obj->transfer.to, | ||
| o.redeemer, asset(htlc_obj->transfer.amount, htlc_obj->transfer.asset_id ) ); | ||
| htlc_redeemed_operation virt_op( htlc_obj->id, htlc_obj->transfer.from, htlc_obj->transfer.to, o.redeemer, | ||
| amount, htlc_obj->conditions.hash_lock.preimage_hash, htlc_obj->conditions.hash_lock.preimage_size ); |
There was a problem hiding this comment.
If the HTLC was redeemed by to, she won't get notified by the virtual htlc_redeemed_operation, thus it's still inconvenient for her since the new added fields are not in htlc_redeem_operation. In addition, the redeemer won't get notified with the virtual operation. IMHO it's better to simply notify all parties in get_impacted_account_visitor::operator()( const htlc_redeemed_operation& op ). Need to take care of account history numbering if to take this approach.
| visit() { return true; } | ||
| template<typename Op> | ||
| std::enable_if_t<TL::contains<BSIP_40_ops, Op>(), bool> | ||
| visit() { return HARDFORK_BSIP_40_PASSED(now); } |
There was a problem hiding this comment.
I think it's better to put the code related to bsip40 in a new bsip40-hf-visitor class/struct which is derived from a generic hf-visitor class/struct.
There was a problem hiding this comment.
The hardfork_visitor is quite similar to the get_impacted_account_visitor -- it should work for all operation types and should tell you something about that operation in a reasonably polymorphic fashion. Thus it needs to deal with all operation types within a single struct; it can't reasonably be split over multiple structs or files.
It looks like the BSIP40 code is a special case here because it's the only case that existed at the time it was written that wasn't trivially true. Over time, all new operations will be added to this so that a generic operation can always be inspected to see if it's forked in yet or not, so the BSIP40 handling is not really out of place here.
There was a problem hiding this comment.
@oxarbitrage please check if have time. Thanks.
Update: I'm working on it.
Update: done with #2134.
libraries/protocol/include/graphene/protocol/custom_authority.hpp
Outdated
Show resolved
Hide resolved
| FC_REFLECT(graphene::protocol::custom_authority_update_operation, | ||
| (fee)(account)(authority_to_update)(new_enabled)(new_valid_from) | ||
| (new_valid_to)(new_auth)(restrictions_to_remove)(restrictions_to_add)(extensions)) | ||
| FC_REFLECT(graphene::protocol::custom_authority_delete_operation, (fee)(account)(authority_to_delete)(extensions)) |
| /* 16 */ withdraw_permission_id_type, \ | ||
| /* 17 */ vesting_balance_id_type, \ | ||
| /* 18 */ worker_id_type, \ | ||
| /* 19 */ balance_id_type, \ |
There was a problem hiding this comment.
The list was initially generated when HTLC is not implemented, so the data types used in HTLC-related operations E.G. htlc_id_type and htlc_hash are not in the list. I haven't fully reviewed the code so far, so I'm not sure how users could create custom active authorities (CAA) for the HTLC-related operations. Just think that the CAA feature should be ease to expand. Will update when have more info.
There was a problem hiding this comment.
The *_id_types are listed here mainly for the convenience to compare 2 containers. For individual id types, it's possible to create restrictions with func_attr which will recurse into the object_id struct and restrict the instance field directly.
There was a problem hiding this comment.
I was wrong. object_id::instance is not reflected, thus we're unable to recurse into it with func_attr. It means we need to add the missing types in this type list. Updated the previous comment.
There was a problem hiding this comment.
Best if we can have code to check whether there is a type missing, or a field that can't define restrictions for. For the fields that aren't supported, we need to document them. In my old implementation (https://github.com/bitshares/bitshares-core/pull/1362/files#diff-e9075b54e174b51fa26c8db6f535b8f2R440) I didn't support all types.
There was a problem hiding this comment.
Best if we can have code to check whether there is a type missing, or a field that can't define restrictions for. For the fields that aren't supported, we need to document them. In my old implementation (https://github.com/bitshares/bitshares-core/pull/1362/files#diff-e9075b54e174b51fa26c8db6f535b8f2R440) I didn't support all types.
See 3cd47d1
The list was initially generated when HTLC is not implemented, so the data types used in HTLC-related operations E.G.
htlc_id_typeandhtlc_hashare not in the list. I haven't fully reviewed the code so far, so I'm not sure how users could create custom active authorities (CAA) for the HTLC-related operations. Just think that the CAA feature should be ease to expand. Will update when have more info.
See b957a22
| namespace graphene { namespace protocol { | ||
| using result_type = object_restriction_predicate<operation>; | ||
|
|
||
| result_type get_restriction_predicate_list_1(size_t idx, vector<restriction> rs) { |
There was a problem hiding this comment.
Probably better if make a template file and generate these source files with a script or cmake configuration.
| if (typelist::contains<operation_list_8::list, Op>()) | ||
| return get_restriction_predicate_list_8(typelist::index_of<operation_list_8::list, Op>(), std::move(rs)); | ||
| if (typelist::contains<operation_list_9::list, Op>()) | ||
| return get_restriction_predicate_list_9(typelist::index_of<operation_list_9::list, Op>(), std::move(rs)); |
There was a problem hiding this comment.
Personally I don't like the long list of ifs, but perhaps it's just me. We can improve it in the future anyway.
For reference, in my first (partial) implementation I tried to do it via switch-case, see https://github.com/bitshares/bitshares-core/pull/1362/files#diff-e9075b54e174b51fa26c8db6f535b8f2R507.
There was a problem hiding this comment.
Yeah, this list annoys me too. It's there to break up the build and keep too many template instantiations from happening in one compilation unit. Unfortunately, it's annoying to get rid of because it's so arbitrary. You could replace it with some kind of mapping or jump table of list-type-to-callable but it's not clear to me that it would be an improvement.
| const auto& to_num(const I& i) { return i; } | ||
| template<typename I> | ||
| const auto& to_num(const fc::safe<I>& i) { return i.value; } | ||
| inline auto to_num(const fc::time_point_sec& t) { return t.sec_since_epoch(); } |
There was a problem hiding this comment.
Might deal with unsigned_int similarly here. Actually, IMHO unsigned_int should be treated as integral.
| // Simple comparison, same types | ||
| constexpr static bool valid = true; | ||
| constexpr int8_t operator()(const Field& f, const Argument& a) const { | ||
| return f<a? -1 : (f>a? 1 : 0); |
There was a problem hiding this comment.
It's possible that operator > is not implemented in some data types. I guess it's better to use a<f here.
| /* 10 */ committee_member_id_type, \ | ||
| /* 11 */ witness_id_type, \ | ||
| /* 12 */ limit_order_id_type, \ | ||
| /* 13 */ call_order_id_type, \ |
There was a problem hiding this comment.
This ID is not used in consensus.
| /* 6 */ fc::sha256, \ | ||
| /* 7 */ account_id_type, \ | ||
| /* 8 */ asset_id_type, \ | ||
| /* 9 */ force_settlement_id_type, \ |
There was a problem hiding this comment.
This ID is not used in consensus, but only used in virtual operation(s).
| /* 11 */ witness_id_type, \ | ||
| /* 12 */ limit_order_id_type, \ | ||
| /* 13 */ call_order_id_type, \ | ||
| /* 14 */ custom_id_type, \ |
There was a problem hiding this comment.
This ID is not used in consensus.
| /* 15 */ proposal_id_type, \ | ||
| /* 16 */ withdraw_permission_id_type, \ | ||
| /* 17 */ vesting_balance_id_type, \ | ||
| /* 18 */ worker_id_type, \ |
There was a problem hiding this comment.
This ID is not used in consensus.
| /* 16 */ withdraw_permission_id_type, \ | ||
| /* 17 */ vesting_balance_id_type, \ | ||
| /* 18 */ worker_id_type, \ | ||
| /* 19 */ balance_id_type, \ |
There was a problem hiding this comment.
As mentioned above, htlc_id_type is missing, as well as custom_authority_id_type. Perhaps it's good to exclude custom_authority_*_operation though.
| ::add_list<typelist::slice<operation::list, 47, 51>> | ||
| ::add<htlc_extend_operation> // 52 | ||
| ::finalize>; | ||
| using operation_list_9 = static_variant<typelist::slice<operation::list, 54>>; |
There was a problem hiding this comment.
We do not need to support CAA for these operations:
account_transfer_operationwhich is defined but not implemented;committee_member_update_global_parameters_operationwhich must be done withcommittee-account;blind_transfer_operationandtransfer_from_blind_operationwhich must be done withtemp-account;asset_update_issuer_operationwhich requires owner permission.
BTW custom_authority_*_operations are supported here, I'm not sure if it's a good idea to support them in the first release due to their complexities.
| /* 3 */ string, \ | ||
| /* 4 */ time_point_sec, \ | ||
| /* 5 */ public_key_type, \ | ||
| /* 6 */ fc::sha256, \ |
There was a problem hiding this comment.
We may need to support fc::ripemd160, fc::hash160, fc::sha1 and etc for assert_operation and HTLC operations.
| }; | ||
| template<typename Field, typename Argument> | ||
| struct predicate_eq<Field, Argument, std::enable_if_t<is_container<Field> && is_integral<Argument>>> { | ||
| // Compare container size against int |
There was a problem hiding this comment.
Best if support container size inequality comparison as well (E.G. restrict that a container's size is greater or less than X). That's why my implementation had a member modifier field (https://github.com/bitshares/bitshares-core/pull/1362/files#diff-33c4d93495a6f66f876600be88117390R35).
By the way, best if support comparison between a static_variant and an int, which implies compare(static_variant::which(), int).
| template<typename, typename = void> | ||
| struct is_container_impl : std::false_type {}; | ||
| template<typename T> | ||
| struct is_container_impl<T, make_void<typename T::value_type, decltype(declval<T>().size())>> : std::true_type {}; |
There was a problem hiding this comment.
IMHO a vector(char) should be treated as a string but not a container.
| template<typename Container, typename Element> | ||
| struct predicate_in<Container, flat_set<Element>, | ||
| std::enable_if_t<is_container<Container> && | ||
| comparable_types<typename Container::value_type, Element>>> { |
There was a problem hiding this comment.
The flat_map type used in graphene::authorities is not supported. Perhaps implement something similar to variant_assertion.
There was a problem hiding this comment.
That would be interesting, but I anticipate it'll be a fairly niche use case. I'm calling it future work. :P
Minor fixes for 4.0
Update mainnet seed nodes
Explicitly list cpp files for building wallet lib, fix Gitian build
See bitshares#2093 (comment) Add a cmake template file for the list_#.cpp files and autogenerate them from the template rather than having 12 almost identical copies of a file
for prettier error message
Update error messages
Update README.md
Update Copyright and contributors
Update docs to point to latest content in Wiki
Update version in Doxyfile to 4.0.0
to Thursday, July 30, 2020 13:55:00 UTC (2020-07-30T13:55:00Z)
Set BitShares Mainnet 4.0 protocol activation time
For final review and fix.
Note: not all issues found in review have to be fixed in this release, some of them could be addressed in future releases but don't forget to track them.