Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix cleos to get table rows only when satisfying condition #6070

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

conr2d
Copy link
Contributor

@conr2d conr2d commented Oct 19, 2018

Change Description

Fixes #6067
When cleos gets table rows to print info, the condition is given only to lower bound, so not exact matched rows are included to the result. This patch adds upper bound, so the rows which not matching exactly will not included.

Copy link
Contributor

@brianjohnson5972 brianjohnson5972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the better solution, I will close #6068 .

@brianjohnson5972
Copy link
Contributor

None of the tests touch this code, @conr2d did you manually test this? (I manually tested your bidnameinfo change, since that didn't require much setup)

@conr2d
Copy link
Contributor Author

conr2d commented Oct 19, 2018

Yes, I tested following commands that can be affected by this PR:

  • cleos system voteproducer approve
  • cleos system voteproducer unapprove
  • cleos system bidnameinfo
  • cleos multisig review

@brianjohnson5972 brianjohnson5972 merged commit ffb72ab into EOSIO:develop Oct 19, 2018
@conr2d conr2d deleted the get_table_rows branch October 23, 2018 18:16
@arhag
Copy link
Contributor

arhag commented Nov 7, 2018

This PR revealed another bug in how chain_plugin handles upper_bound. It does the same thing as lower_bound. This is why the newname.value + 1 as an upper_bound in get table call within the bidnameinfo sub-command solves #6067, but using newname.value as the upper_bound (the sensible choice for upper_bound if one wants an equal_range) breaks it.

The meaning of upper_bound in get_table_rows is supposed to be the same as std::upper_bound: the first element in the range [first, last) that is greater than the supplied value or last if no such element is found.

This means that one can use the same value for the lower_bound and upper_bound and the sub-range [lower_bound, upper_bound) would include the set of all items that match that value. With the current implementation of chain_plugin's get_table_rows, passing in the same value for both lower_bound and upper_bound will always give you no results.

This PR assumes a different definition of upper_bound (the one that is unfortunately currently implemented in chain_plugin). And with that assumed definition it does an effective job fixing #6067. But I think the ideal bug fix (if we didn't have to consider versioning issues) is to first fix chain_plugin and then change the cleos code touched in this PR to pass in the same value for both the lower_bound and upper_bound field.

Unfortunately, we need to also consider how a new version of cleos may be talking to an old version of nodeos. Passing in the same value for both lower_bound and upper_bound in the calls for the system voteproducer approve, system voteproducer unapprove, system bidnameinfo, and multisig review sub-commands would break these sub-commands if cleos is talking to an old version of nodeos. For this reason, the more permissive requested range (with the + 1 on the upper_bound) as is done in this PR is still required even with the proper bug fix.

Furthermore, the bug fix that corrects the definition of upper_bound in chain_plugin (tracked in #6273) will mean that if cleos is talking to the new nodeos with this bug fixed and it requests the larger range, it is no longer safe to assume, as this PR does, that the first row of the returned rows is the correct one. So the additional checks that this PR removes need to be returned back (and the additional check within the bidnameinfo cleos sub-command from #6068 must also be included).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants