Refactor get_payments for future separation from wallet2 #5598

fullmetalScience · 2019-06-01T19:28:05Z

Current show_transfers min_height is off by one, meaning that show_transfers 42 42 will look for transactions in the range of 43 - 42. This PR fixes that.

Also, the corresponding functions get_payments and get_payments_out are now trampolines to other functions that each accept the set of transactions to be filtered from as an additional parameter.

This allows for those functions to be separated from the wallet instance in the future ("libmonerocore").

A unit test is added to verify functionality of one of the two new functions.

The original function gets payments starting at min_height+1. Though this is unexpected behaviour, it shall remain the default in order to maintain backwards compatibility.

…usive() This makes it compatible to m_payments' type. m_payments is used when called from get_payments().

The original function gets out-payments starting at min_height+1. Though this is unexpected behaviour, it shall remain the default in order to maintain backwards compatibility. The approach is analogous to the changes in commit:3104f4cb.

It was decided to ditch backwards compatibility (mentioned in commit:3104f4cb & commit:b4764435) in favor of a clean implementation going forward.

jtgrassie · 2019-06-02T00:51:34Z

Why such a large commit to fix an off-by-one? This PR creates 2 extra functions which seems unnecessary.

moneromooo-monero · 2019-06-02T08:24:44Z

src/wallet/wallet2.cpp

+    uint64_t min_height, uint64_t max_height, const boost::optional<uint32_t>& subaddr_account, const std::set<uint32_t>& subaddr_indices) const
+{
+  get_payments_out_min_height_inclusive(confirmed_payments, m_confirmed_txs, min_height, max_height, subaddr_account, subaddr_indices);
+}


Since you're breaking compatibility anyway, please remove the "min_height_inclusive" suffix.

Done (0c43135).

moneromooo-monero · 2019-06-02T08:26:17Z

tests/unit_tests/get_payments.cpp

+        matched_payments.clear();
+	m_wallet.get_payments_min_height_inclusive(matched_payments, actual_payments, block_height_that_has_transaction, block_height_that_has_transaction);
+        ASSERT_EQ(1, matched_payments.size());
+}


Can you add more of them ? At the moment it's just one, does not test much.

This should do: e65fd7f

I cleaned the structure for the tests and added several new variants, all of which are now also implemented for the get_payments_out function.

There's just one payment in the list. I'd expect a test to test when there are no payments, more than one payment per height, payments both in and out of the range you query, etc.

fullmetalScience · 2019-06-04T13:13:18Z

Why such a large commit to fix an off-by-one? This PR creates 2 extra functions which seems unnecessary.

The off-by-one is one issue this PR tackles. The additional functions are to help separate the functionality from wallet2 in order to get to an independent "wallet library", in the long-term. (An effort @paulshapiro said he's interested in on IRC.)

Now the reason those two things come as one PR is that I did a TDD approach and this allowed me to directly unit test the functionality. Sure, otherwise it would have made sense to request inclusion separately.

jtgrassie · 2019-06-04T15:13:54Z

Is the off-by-one really a bug or is it more of a documentation issue though? Current functionality it returns txs greater than min height and less than or equal to max height. This PR changes this existing functionality to be inclusive of the heights. My concern is this may break existing third party integrations that may be reliant on the current implementation.

Regardless of this point, I'm not a fan of merging what essentially are two discrete changes into a single PR. E.g. changes for a wallet library interface are separate to changing an inclusive/exclusive change. IMHO.

fullmetalScience · 2019-06-04T16:01:32Z

Current functionality it returns txs greater than min height and less than or equal to max height.

That's the thing. Min is exclusive while max in inclusive. For me, the names "min/max" indicate meaning of "minimum/maximum number that's to be filtered for". Otherwise they should be named as "lower bound/upper bound", or, with the current configuration, "lower bound/max height" (=> even more confusing).

Usability would be my next argument. Say the user copy-pastes 1849630 from somewhere, because that's the block height he wants to inspect. Now he has to delete the last two digits and replace them by "29". Easy, but a bit annoying; The same example with 1800000 would be even more annoying - Changing 6 digits just to lower the number by one. It feels off.

This PR changes this existing functionality to be inclusive of the heights. My concern is this may break existing third party integrations that may be reliant on the current implementation.

That was our concern too. We had a bit of discussion on IRC and ended up with this approach as opposed to further bloating the code by implementing two parallel variants. Release notes should reflect the change. Further, I hinted at semantinc versioning as a possible versioning scheme, which would mean a bump to the major version number in Monero in such a case. However, I never investigated what the current versioning logic is and have no strong opinion at all on that topic.

Regardless of this point, I'm not a fan of merging what essentially are two discrete changes into a single PR.

I am open to separating the two things. I'll have a look at it.

moneromooo-monero · 2019-06-04T17:10:12Z

Same PR is fine. It's the commits that are important. These will get squashed when finished, into either one or two commits, along the lines of what jtgrassie said.

moneromooo-monero · 2019-06-04T17:15:34Z

About the "min is inclusive, max is exclusive" thing: as I said on IRC, it allows empty ranges, and is more canonical, like what STL does. It allows doing nice things like incremental scanning, eg:

last_height = 0
while True:
  blockchain_height = get_current_blockchain_height()
  get_payments(last_height, blockchain_height)
  last_height = blockchain_height

In this case, you want the data to return the txes that are in blocks last_height-1 to blockchain_height-1. If blockchain_height equals last_height, then it's an empty range: it means that if the blockchain has not changed since last scan, then you don't get txes back. This way, you're sure to get all txes, and never get dupes.

fullmetalScience · 2019-06-04T18:52:47Z

This way, you're sure to get all txes, and never get dupes.

Thanks for explaining this benefit. I figure mixing inclusive/exclusive is logical from a coders perspective, while I am looking at it from a users perspective.

Nothing holds the user back from amending the filter as explained in my recent comment, just as nothing holds the coder back from amending the value upon calling get_payments in your example. Anyway I get that the latter may be more intuitive for developers.

Suggestions:

Maintaining the refactoring, I revert the function to it's original logic (and make the tests pass accordingly).
Allow me to find a better name for the min parameter (that is currently exclusive).
I add additional tests as per your recommendation. (My original testing was just to confirm the "bug before fixing, not to verify the already existing functionality, thus the low case coverage.)

After this, I make a separate commit, amending the show_transfers command so it calls get_payments in a manner that satisfies the user perspective (as in get_payments(--min_height, max_height)).

Thoughts?

jtgrassie · 2019-06-04T19:30:27Z

I understand the reasons for wanting to change the way these are working, but we must not forget there could be third parties reliant on how they currently work (these changes affect the wallet RPC methods).

…ginal get_payments logic Dictionaries define **minimum** as "the least quantity or amount possible", while threshold is "the point that must be exceeded to begin producing a given effect".

fullmetalScience · 2019-06-05T12:07:42Z

Here are my previously suggested commits. They maintain the original logic, but I took the freedom to rename min_height to threshold, since "minimum" would be wrong, according to several dictionaries (definitions in 0710b8f).

If these changes are fine for you as they are, I would like to go on and finish up by amending show_transfers to call get_payments with --threshold and thus mimic an actual "minimum".

(According to mooo that does not interfere with RPC in any way.)

jtgrassie · 2019-06-05T12:20:41Z

"threshold" can mean beginning or end boundary. Therefore the usage here is ambiguous.

fullmetalScience · 2019-06-05T13:19:23Z

"threshold" can mean beginning or end boundary. Therefore the usage here is ambiguous.

Surprisingly, after an hour of investigation, "threshold" was the best result, with "border" being the second-best contender. (For some reason, "verge" doesn't work at all 😁.)

On the other hand, it was clear immediately that "minimum" (just like bound, limit, ...) is definitely wrong, so I went for the above.

Happy to replace with any perfectly matching suggestion;

jtgrassie · 2019-06-05T14:51:34Z

Per your own link, 2a and 2b, threshold can mean both start and end. Minimum and maximum are the correct terms to use here (as they were). Documentation should specify whether they are each inclusive/exclusive (and any programmer seeing min/max knows to check this).

fullmetalScience · 2019-06-11T07:00:03Z

To finish this up, would you be okay with min_height_excl as a name or is your preference for the original names final?

jtgrassie · 2019-06-11T11:09:08Z

My preference is keeping the original names, as I have already explained.

- as per hyc's explanation that renaming words was out of scope

- as per moneromoo's hint that the project typically used 2 or 4

fullmetalScience · 2019-06-12T10:12:36Z

All done.

moneromooo-monero

It looks like the actual fix got undone, or am I missing it ?

moneromooo-monero · 2019-09-17T12:33:06Z

Well that unfortunately didn't go anywhere.
Guess I'll go back to the off by one sometime and see what can be done.
Please reopen if you happen to get back to this and just fix the bug in a way that doesn't break existing users.

+invalid

fullmetalScience added 4 commits May 31, 2019 21:34

wallet2: new function that gets payments starting at min_height

3104f4c

The original function gets payments starting at min_height+1. Though this is unexpected behaviour, it shall remain the default in order to maintain backwards compatibility.

wallet2: use const payment_container& in get_payments_min_height_incl…

1a82ad0

…usive() This makes it compatible to m_payments' type. m_payments is used when called from get_payments().

wallet2: default get_payments functions correctly regard min_height

2e2e1d9

It was decided to ditch backwards compatibility (mentioned in commit:3104f4cb & commit:b4764435) in favor of a clean implementation going forward.

moneromooo-monero reviewed Jun 2, 2019

View reviewed changes

fullmetalScience added 2 commits June 4, 2019 12:50

wallet2: function names simplified and certain parameters made optional

0c43135

unit_tests: tests for get_payments improved and extended

e65fd7f

fullmetalScience added 2 commits June 5, 2019 10:45

wallet2: substituted "min_height" for "threshold" and reverted to ori…

0710b8f

…ginal get_payments logic Dictionaries define **minimum** as "the least quantity or amount possible", while threshold is "the point that must be exceeded to begin producing a given effect".

wallet2: added unit tests to check previosly existing logic

e32ff8f

fullmetalScience changed the title ~~Make show_transfers regard min_height as the lower block height to look for~~ Refactor get_payments for future separation from wallet2 Jun 5, 2019

fullmetalScience added 2 commits June 12, 2019 09:56

wallet2: renamed "threshold" back to "min_height"

c0eed5b

- as per hyc's explanation that renaming words was out of scope

unit_tests: reduced indents from eight to two

0b573e3

- as per moneromoo's hint that the project typically used 2 or 4

moneromooo-monero reviewed Jun 12, 2019

View reviewed changes

issue-helper added the invalid label Sep 17, 2019

issue-helper closed this Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor get_payments for future separation from wallet2 #5598

Refactor get_payments for future separation from wallet2 #5598

fullmetalScience commented Jun 1, 2019

jtgrassie commented Jun 2, 2019

moneromooo-monero Jun 2, 2019

fullmetalScience Jun 4, 2019

moneromooo-monero Jun 2, 2019

fullmetalScience Jun 4, 2019

moneromooo-monero Jun 4, 2019

fullmetalScience commented Jun 4, 2019

jtgrassie commented Jun 4, 2019

fullmetalScience commented Jun 4, 2019

moneromooo-monero commented Jun 4, 2019

moneromooo-monero commented Jun 4, 2019

fullmetalScience commented Jun 4, 2019

jtgrassie commented Jun 4, 2019

fullmetalScience commented Jun 5, 2019 •

edited

jtgrassie commented Jun 5, 2019

fullmetalScience commented Jun 5, 2019

jtgrassie commented Jun 5, 2019

fullmetalScience commented Jun 11, 2019

jtgrassie commented Jun 11, 2019

fullmetalScience commented Jun 12, 2019

moneromooo-monero left a comment

moneromooo-monero commented Sep 17, 2019

Refactor get_payments for future separation from wallet2 #5598

Refactor get_payments for future separation from wallet2 #5598

Conversation

fullmetalScience commented Jun 1, 2019

jtgrassie commented Jun 2, 2019

moneromooo-monero Jun 2, 2019

Choose a reason for hiding this comment

fullmetalScience Jun 4, 2019

Choose a reason for hiding this comment

moneromooo-monero Jun 2, 2019

Choose a reason for hiding this comment

fullmetalScience Jun 4, 2019

Choose a reason for hiding this comment

moneromooo-monero Jun 4, 2019

Choose a reason for hiding this comment

fullmetalScience commented Jun 4, 2019

jtgrassie commented Jun 4, 2019

fullmetalScience commented Jun 4, 2019

moneromooo-monero commented Jun 4, 2019

moneromooo-monero commented Jun 4, 2019

fullmetalScience commented Jun 4, 2019

jtgrassie commented Jun 4, 2019

fullmetalScience commented Jun 5, 2019 • edited

jtgrassie commented Jun 5, 2019

fullmetalScience commented Jun 5, 2019

jtgrassie commented Jun 5, 2019

fullmetalScience commented Jun 11, 2019

jtgrassie commented Jun 11, 2019

fullmetalScience commented Jun 12, 2019

moneromooo-monero left a comment

Choose a reason for hiding this comment

moneromooo-monero commented Sep 17, 2019

fullmetalScience commented Jun 5, 2019 •

edited