Fix upper_bound bug in chain_plugin #6285
Conversation
…ting from upper_bound < lower_bound
#6273 Still need to request larger range than necessary and do additional checks just so v1.5.0 of cleos can still work with nodeos versions older than v1.5.0.
…finition of upper_bound
| auto upper = secidx.lower_bound(boost::make_tuple(next_tid)); | ||
| if( t_id != nullptr && index_t_id != nullptr ) { | ||
| using secondary_key_type = std::result_of_t<decltype(conv)(SecKeyType)>; | ||
| static_assert( std::is_same<decltype(IndexType::value_type::secondary_key), secondary_key_type>::value, "Return type of conv does not match type of secondary key for IndexType" ); |
There was a problem hiding this comment.
isn't decltype(IndexType::value_type::secondary_key) == IndexType::value_type::secondary_key_type ? or was this just less characters because of the need for typename?
There was a problem hiding this comment.
Yeah, I guess I didn't notice the secondary_key_type typedef. Either way works.
|
@arhag I am confused about this quote
Is it not the opposite? Since upper_bound is now exclusive, rather than inclusive. For example, if I was searching on a uint64 index and let's say I wanted all rows where the index = 0. Previously, I would set lower_bound to 0 and upper_bound to 1. With this new behaviour, if I understood correctly, I can keep both lower_bound and upper_bound to 0. So, if I didn't change my code and I left lower to 0 and upper to 1, I would now receive all rows with the index value being 0 or 1. That is more rows, not less. Is there a misunderstanding somewhere? |
You are understanding correctly. Perhaps my wording was just unclear. What I was trying to say, continuing with your example, was that if your client was updated to now set both |
|
Ah when talking to an old nodeos. Got it. |
Change Description
Resolves #6273.
PR #6070 revealed an existing bug in the implementation of
chain_pluginwith regards to how it handles theupper_boundfield in theget_table_rowsandget_table_by_scopeRPC calls.The intended definition for this
upper_boundfield is similar to how it is defined in thestd::upper_boundfunction in the C++ standard library: the first element within the specified ordered range with a value greater than the given comparison value.Instead, the implementation in
chain_pluginhandledupper_boundas if the definition was the same as that oflower_bound: the first element within the specified ordered range with a value greater than or equal to the given comparison value.The intended definition has the advantage of being consistent with the standard definition of
upper_boundwell known to C++ programmers and it also enables callers to request anequal_rangeby simply passing in the same comparison value for both thelower_boundandupper_boundfields. Attempting to do this with the old definition required knowing and calculating the value immediately succeeding thelower_boundvalue and passing that in as theupper_boundfield (as PR #6070 did within cleos). Passing in the same comparison value for both thelower_boundandupper_boundusing the old definition would result in no ranges ever returned.The existing implementation of
chain_pluginhas a further bug (which this PR also fixes) in the case whereupper_boundis less thanlower_bound. The correct behavior in such a situation is to never return any results.This PR fixes the bug in the case where
upper_boundis less thanlower_boundand it also changes the implementation ofchain_pluginto handle theupper_boundfield according to the intended definition rather than the existing definition. The implications of these changes to users of the chain API are described in detail under the "API Changes" section.Furthermore, this PR adds back the additional checks in cleos for the
system voteproducer approve,system voteproducer unapprove,multisig reviewsub-commands that were removed in #6070 and ensure the results returned from the chain API RPC are exactly what was requested. It also adds a similar check for thesystem bidnameinfosub-command (similar to what was in #6068) to fix the bug described in #6067. These additional checks are necessary (even though they were not necessary in PR #6070) because this PR keeps the changes to theupper_boundvalue specified by cleos made in PR #6070 in which theupper_boundvalue is the value immediately succeeding thelower_boundvalue; thatupper_boundworkaround was kept just so that the version of cleos incorporating the changes in this PR can still behave correctly when talking to an old version of nodeos that has not incorporated the changes in this PR. (Also note that an old cleos at version v1.4.x or older would still behave correctly with thesystem voteproducer approve,system voteproducer unapprove, andmultisig reviewsub-commands even when talking to a new version of nodeos that incorporates the changes in this PR. However, in such a situation, the bug described in #6067 would still of course apply when usingsystem bidnameinfo.)Consensus Changes
None
API Changes
Users of the
get_table_rowsandget_table_by_scopeRPCs of the chain API are unaffected by these changes if they are not already using theupper_boundfield. Existing users of theupper_boundfield of those two RPCs should read on to understand the changes introduced by this PR.get_table_rowsFirst some background on
get_table_rows:A particular index type (either the primary key type or one of the five secondary index types) is selected (whether implicitly or explicitly using the
key_typefield) in theget_table_rowscall. The index type defines the type of the index key within table records to use to sort the table rows in the table for the purposes of this RPC.A particular index ordinal is selected (whether implicitly or explicitly using the
index_positionfield). This index ordinal must correspond to the selected index type (based on how the tables are structured in the persistent state on the blockchain), and the selected index ordinal along with the selected index type (together can be referred to as the selected index) determine how the tables rows in the table are ordered. For selected indices that are secondary indices, the sort order is based first on the index key (in ascending order) and then ties are broken by the primary key (in ascending order).A particular table is specified by: the index type, index ordinal, and the tuple
(code, scope, table). The rows in the table have an index key (of index type) which is the value used to sort the order of the table rows within the selected index.If
lower_boundis specified, letcandidate_rowsbe the set of table rows in the specified table which have an index key greater than or equal to the value specified inlower_bound. Otherwise, letcandidate_rowsbe the set of all table rows in the specified table.Behavior prior to this PR:
If
upper_boundis specified, letexcluded_rowsbe the set of table rows in the specified table which have an index key greater than or equal to the value specified inupper_bound. Otherwise, letexcluded_rowsbe the empty set.If the value specified in
upper_boundis less than the value specified inlower_bound, the behavior ofget_tables_rowsis undefined. Otherwise,get_table_rowsreturns the set of tables rows (in sorted order) that are incandidate_rowsbut not inexcluded_rows.Behavior as of this PR:
If
upper_boundis specified, letexcluded_rowsbe the set of table rows in the specified table which have an index key greater than the value specified inupper_bound. Otherwise, letexcluded_rowsbe the empty set.If the value specified in
upper_boundis less than the value specified inlower_bound,get_table_rowsreturns no rows. Otherwise,get_table_rowsreturns the set of tables rows (in sorted order) that are incandidate_rowsbut not inexcluded_rows.Implications to existing users of
upper_bound:get_table_rowswith anupper_boundless thanlower_bound. If they currently are doing so and using the returned results, they are relying on undefined behavior. This PR fixes that undefined behavior so that a logically consistent result is returned should users continue to make this inappropriate call.get_table_rowsthat call it with anupper_boundvalue greater than or equal to thelower_boundvalue may get back less rows with the new behavior than they received with the old behavior. If users adjust their call by settingupper_boundto the value immediately succeeding the value they provided originally, they will receive no less rows with the new behavior than they did before with the old behavior (they can then further filter on the client side).get_table_by_scopeFirst some background on
get_table_by_scope:The tables within a given
codeaccount are sorted (for the purposes ofget_table_by_scope) first byscope(in ascending order) and then ties are broken bytablename (in ascending order).If
lower_boundis specified, letcandidate_tablesbe the set of tables in the specifiedcodeaccount which havescopegreater than or equal to the value specified inlower_bound. Otherwise, letcandidate_tablesbe the set of all tables in the specifiedcodeaccount.If the
tableis not specified in the RPC (or its value is 0), then letexcluded_tables1be the empty set. Otherwise, letexcluded_tables1be the set of tables in the specifiedcodeaccount which have atablename equal to the specifiedtablevalue in the RPC.Behavior prior to this PR:
If
upper_boundis specified, letexcluded_tables2be the set of tables in the specifiedcodeaccount which have ascopegreater than or equal to the value specified inupper_bound. Otherwise, letexcluded_tables2be the empty set.If the value specified in
upper_boundis less than the value specified inlower_bound, the behavior ofget_table_by_scopeis undefined. Otherwise,get_table_by_scopereturns the set of tables (in sorted order) that are incandidate_tablesbut not in the union ofexcluded_tables1andexcluded_tables2.Behavior as of this PR:
If
upper_boundis specified, letexcluded_tables2be the set of tables in the specifiedcodeaccount which have ascopegreater than the value specified inupper_bound. Otherwise, letexcluded_tables2be the empty set.If the value specified in
upper_boundis less than the value specified inlower_bound,get_table_by_scopereturns no tables. Otherwise,get_table_by_scopereturns the set of tables (in sorted order) that are incandidate_tablesbut not in the union ofexcluded_tables1andexcluded_tables2.Implications to existing users of
upper_bound:get_table_by_scopewith anupper_boundless thanlower_bound. If they currently are doing so and using the returned results, they are relying on undefined behavior. This PR fixes that undefined behavior so that a logically consistent result is returned should users continue to make this inappropriate call.get_table_by_scopethat call it with anupper_boundvalue greater than or equal to thelower_boundvalue may get back less tables with the new behavior than they received with the old behavior. If users adjust their call by settingupper_boundto the value immediately succeeding the value they provided originally, they will receive no less tables with the new behavior than they did before with the old behavior (they can then further filter on the client side).Documentation Additions
Documentation about the
get_table_rowsandget_table_by_scopeRPCs of the chain API may need to be updated to reflect the new definition ofupper_boundand the implications of that as discussed in the "API Changes" section above.