fix: store cursors should be exclusive and match a DB item

[jm-clius]

This fixes two related bugs in nwaku v0.12.0 with regards to cursors in the store protocol:

Bug 1: message indexed by cursor excluded from results

Summary of unexpected behaviour:

• a history response returns a cursor pointing to the beginning of the next page, if such a page exists
• the message pointed to by this cursor is excluded from a subsequent history response as per RFC 13 and is therefore excluded from the retrievable history

Fix

This fix:

• reverts to using the index of the last message within the returned response as the cursor (as it was pre v0.12.0 and is described in the RFC)
• continues with the post v0.12.0 behaviour of only returning a cursor when a next page is available. This is how most clients are currently implemented and is necessary to fix a fairly urgent bug.

Comments:

• an alternative solution would be to continue using the index of the next message as cursor to retrieve the next page and simply change the cursor logic (for all stores) to be inclusive of the cursor rather than exclusive. I prefer not doing this, as it violates the RFC (and requires the store client to make use of the index of a message it has not yet received)
• after some deliberation, I'm not sure that I agree with the logic of "no cursor returned if no next page available". Why wouldn't we? A cursor is a pointer to a place in history, not some contractual agreement that more history exists. This is up to the client application to determine. If someone, for example, is querying forward in history they may reach "real time", with no newer messages available in history yet. However, they should still receive a cursor from which they can continue their forward query at some point when they believe new messages may have arrived. This logic is also the only way the RFC can really be interpreted currently. Since several clients rely on this "empty pagination/cursor info" condition to stop querying, we can temporarily continue implementing the cursor this way, but in future we either need to amend the RFC or change the clients' behaviour.

Bug 2: cursor retrieval from sqlite DB using incorrect length

Summary of unexpected behaviour

• when reading the message digest (id) from the DB, the wrong number of bytes were read, resulting in HistoryResponses containing the wrong cursor
• this implies that subsequent queries would use a cursor with a digest component that doesn't match any message in the DB. The impact here was somewhat mitigated in that a cursor was never directly matched, but only compared to other cursors (with the storedAt component playing an important role).

Fix

• use correct column when determining the byte length of DB columns

Tests

Since these bugs were missed in tests, I've modified the unit tests to verify that we can page through an entire history in both directions.
This:

• ensures that the message pointed to by a pagingInfo cursor are excluded as expected from subsequent queries
• ensures that an exact match does exist for the cursor (otherwise one of the paging directions would wrongly reinclude the message supposedly pointed to by the non-matching cursor)

[status-im-auto]

Jenkins Builds

Click to see older builds (4)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	44f7dc6	#1	2022-10-13 15:41:49	~16 min	linux	📦bin
✔️	44f7dc6	#1	2022-10-13 15:44:01	~18 min	macos	📦bin
✔️	de27de3	#2	2022-10-18 16:09:58	~16 min	linux	📦bin
✔️	de27de3	#2	2022-10-18 16:15:11	~22 min	macos	📦bin

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	aa85c5f	#3	2022-10-18 23:02:00	~14 min	macos	📦bin
✔️	4499e9e	#3	2022-10-20 08:50:32	~15 min	linux	📦bin
✔️	4499e9e	#4	2022-10-20 09:01:58	~26 min	macos	📦bin

[jm-clius]

Note that this bug has the side effect of allowing a workaround for the issue reported in #1157. We therefore first need to fix that issue, before merging this.

[rymnc]

LGTM!

[LNSD]

LGTM ✅

It took me some time to review it. Thanks for the fixes :)

[LNSD]

after some deliberation, I'm not sure that I agree with the logic of "no cursor returned if no next page available". Why wouldn't we? A cursor is a pointer to a place in history, not some contractual agreement that more history exists. This is up to the client application to determine. If someone, for example, is querying forward in history they may reach "real time", with no newer messages available in history yet. However, they should still receive a cursor from which they can continue their forward query at some point when they believe new messages may have arrived. This logic is also the only way the RFC can really be interpreted currently. Since several clients rely on this "empty pagination/cursor info" condition to stop querying, we can temporarily continue implementing the cursor this way, but in future we either need to amend the RFC or change the clients' behaviour.

As I see it, this is a tradeoff decision. With current behavior, we avoid doing an extra query to see that the next page is empty; and I think that all implementations should be able to build a cursor based on the last message they received.

The rationale behind this decision was: Having the clients to query once more to stop the queries loop is terrible for the waku store nodes as it implies a new request to process and a new DB query with the corresponding CPU usage.

This is the typical behavior when implementing pagination on a REST API: https://developer.atlassian.com/server/confluence/pagination-in-the-rest-api/#how-do-i-know-if-there-are-more-pages-

[LNSD]

an alternative solution would be to continue using the index of the next message as cursor to retrieve the next page and simply change the cursor logic (for all stores) to be inclusive of the cursor rather than exclusive. I prefer not doing this, as it violates the RFC (and requires the store client to make use of the index of a message it has not yet received)

This behavior aligned with the "no-cursor if no-more messages" logic. I am keener to implement it by fixing the message store queries by including the message on the next page. But if your changes fixes the bug with another approach, I am ok with it 😁

[alrevuelta]

lgtm!

as a nitpick/question, any reason behind using a non deterministic timestamp getNanosecondTime(getTime().toUnixFloat()) instead of hardcoded value?

In this case it shouldn't matter but tests should be reproduzable/deterministic/idempotent/etc.

[jm-clius]

In this case it shouldn't matter but tests should be reproduzable/deterministic/idempotent/etc.

I agree! In this case using real time was necessary, because the underlying store message validity check uses real time to determine if a message will be inserted or not. The test would therefore fail if it uses timestamps that do not fall within 20 seconds of current time. Out of scope for this PR, but the way to fix it would be to change the underlying validity check to take a reference timestamp as parameter (which defaults to current time).