Enabling pubsub topic filter in history queries

[staheri14]

This PR is an increment towards https://github.com/status-im/nim-waku/issues/485

• Enables querying historical messages based on their pubsub topic
• Introduces a new field of pusubTopic to the HistoryQuery and IndexedWakuMessage
• Modifies the database schema to hold pubsub topic per waku message (@jm-clius Please let me know whether the updates on the database schema and the procedures of data insertion and load make sense, thanks!)
• Extends the message_store interface to return the pubsub topic
• Updates waku_message_store implementation
• The query process is updated. Previously, NO history messages would be retrieved UNLESS the history query specifies non empty filters (at least one of the content topics or pubsub topic). Now, this semantic has changed i.e., ALL the historical messages will be returned UNLESS the history query specifies non empty filters. In this case, messages would be extracted based on the filters. This is to account for the case that one may be interested in the entire history but is not aware of all the pubsub topics or content topics.
• The order of protobuf fields in HistoryQuery is also updated. The pubsubTopic field has taken over the position of the pagingInfo field and the order of rest of the fields are shifted by 1. This ordering makes more sense as the pubsubTopic field plays a more important role in the history query structure/ function compared to pagingInfo.
• It also fixes an issue re duplicates in the filtered historical messages. Duplicates may exist if a history query contains two content filters targeting the same content topic. In this PR, the retrieved messages are deduplicated.

The corresponding specs PR https://github.com/vacp2p/rfc/pull/347

Future work

The json rpc api of the store protocol must be updated to support pubsub topic filter.

[oskarth]

If we want to change field order, I'd probably change the store protocol id to avoid confusion. I.e. bump /vac/waku/store/2.0.0-beta1 to beta2 (or if it is beta3 now). Ditto in specs

[staheri14]

Generally LGTM! My only comment is that the schema change will break existing persistent message stores and the INSERT and SELECT queries will fail. Since we haven't released yet this is probably OK, though we'll have to advise existing store users to manually delete their current DBs for the change to take effect. Existing stores that I know of include WalletConnect and the wakuv2 cluster nodes.

Thanks @jm-clius! Yeah, that is very important to let them know about such changes. How do we usually do that, are we going to ping them in our next release? Also, are they, WalletConnect and the wakuv2 cluster nodes, currently using the latest version of the master branch?

[staheri14]

If we want to change field order, I'd probably change the store protocol id to avoid confusion. I.e. bump /vac/waku/store/2.0.0-beta1 to beta2 (or if it is beta3 now). Ditto in specs

@oskarth The store protocol id is inconsistent bw specs and nim-waku. The specs says /vac/waku/store/2.0.0-beta2 whereas the nim-waku is /vac/waku/store/2.0.0-beta1 https://github.com/status-im/nim-waku/blob/5f1bab146dee48670288994812cd007f9844ec6c/waku/v2/protocol/waku_store/waku_store.nim#L29 I assume that I should consider the higher version i.e., beta2 valid

Found the issue, it should be beta2, the protocol id in nim-waku version was behind the specs.

[staheri14]

If we want to change field order, I'd probably change the store protocol id to avoid confusion. I.e. bump /vac/waku/store/2.0.0-beta1 to beta2 (or if it is beta3 now). Ditto in specs @oskarth Done!

[oskarth]

DB schema - that's a good point, I missed that! Normally we'd want to write the code in such a way that upgrades work seamlessly. The first question would be, can we do that here in a simple way?

If not or we don't think it is worth it, then we could consider making it a specific release. If we think we are still too much in development, and things are likely to keep changing over next few weeks, I think it could be OK to update to master and just let people know. I.e. make sure cluster works, announce to waku channel and let WalletConnect know.

What do people think?

[staheri14]

If we think we are still too much in development, and things are likely to keep changing over next few weeks, I think it could be OK to update to master and just let people know. I.e. make sure cluster works, announce to waku channel and let WalletConnect know.

What do people think?

@oskarth I thinks it is likely that we need to update the DB schema in the next weeks e.g., right now I am aware of the version number which is going to be added to the DB. I also envision that waku message's sender generated timestamp would be another thing that we may want to include in the DB.

[jm-clius]

Thanks @jm-clius! Yeah, that is very important to let them know about such changes. How do we usually do that, are we going to ping them in our next release? Also, are they, WalletConnect and the wakuv2 cluster nodes, currently using the latest version of the master branch?

Ah, sorry, I must have missed this comment. Both WalletConnect and the test cluster use the latest version of the master branch, so will be affected. For WalletConnect you could ping them on their Telegram and explain that they would have to delete their current DB files (and lose some history). For cluster you should speak to Jakub.

I would suggest though that, while we are changing the schema, that we change this table name to "Message" (i.e. singular and capitalised). I think this is a slightly more idiomatic way of naming tables in SQL and gives us the added advantage of not breaking the current table. :) History will still reset though and the lingering "messages" table would have to be cleaned up/dropped at some point.

[staheri14]

I would suggest though that, while we are changing the schema, that we change this table name to "Message" (i.e. singular and capitalised). I think this is a slightly more idiomatic way of naming tables in SQL and gives us the added advantage of not breaking the current table. :) History will still reset though and the lingering "messages" table would have to be cleaned up/dropped at some point.

@jm-clius Nice idea! Thanks!

[staheri14]

Just something else came to my mind, would not reseting the history be an issue for walletConnect? I am not sure about the type and sensitivity of their data, but should not we be more careful about that?

Following this concern I did some research and realized that we might be able to alter the current table structure by adding a new column to it. This way we will still have access to the past stored messages. But, for the past messages the value of the newly added column would be null which must be carefully addressed. This approach might be a little bit complicated but depending on the sensitivity of the old db it may be worth it. wdyt? @oskarth @jm-clius

Otherwise, we can proceed with different table names.

[staheri14]

What is the Telegram group of WalletConnect? @jm-clius I think found it: "WalletConnect 2.0 <> Waku 2.0" (I am already a member)

[oskarth]

Following this concern I did some research and realized that we might be able to alter the current table structure by adding a new column to it. This way we will still have access to the past stored messages. But, for the past messages the value of the newly added column would be null which must be carefully addressed.

In general, this would be preferable.

Master is still in development though, and we haven't set any stability contract or so yet. So resetting and letting WalletConnect/Waku channel know how to delete old DB might be good enough as a heads up. If they are relying on it and not just for testing, we should indeed be more careful, and also probably encourage them to use tagged releases.

We could also consider migrations with up and down scripts. I don't know what the tooling for Nim and SQL looks like there.

Otherwise, we can proceed with different table names.

This could definitely work too. A simple way would be to just append a number to it, then use that. Once we cut a release we can get rid of the cruft and use the latest schema. For example.

[jm-clius]

Following this concern I did some research and realized that we might be able to alter the current table structure by adding a new column to it.

Yes, as Oskar said, this ALTER TABLE script will generally be the correct way to do this. The issue is usually to determine when to run such a migration script - you could provide all clients with a script and instructions to run or, better, find a way to automatically update existing DBs when they upgrade their clients to a new version. The latter is how we should approach it in future, using the user_version pragma provided by SQLite. Ours is currently set to 1 and this should be increased with every breaking change. When a Waku store node starts up it will check this version and run any necessary migration scripts to alter its tables as needed.

That said, it's a good idea to check with WalletConnect first, but my guess is that in this pre-release phase they would be OK with losing some history. If not, we could provide them with a standalone script that basically copies everything from the "messages" table to the "Message" table and adds a default value for the pubSubTopic column. This way we don't pollute our SQL with pre-release scripts while the schema is expected to be somewhat unstable.

[jm-clius]

What is the Telegram group of WalletConnect? @jm-clius I think found it: "WalletConnect 2.0 <> Waku 2.0" (I am already a member)

Yes, that's the one!

[staheri14]

Thanks @oskarth and @jm-clius!

Given that the store protocol is in the draft mode I also agree that renaming the table is a reasonable solution for now. However, before making this update and merging this PR, I am going to write to walletConnect and to Jakub about this change and see if they have any concerns.

[sbc64]

WalletConnect will likely always be following the nim-waku master as much as possible. We intend to use https://github.com/status-im/go-waku when it has better feature parity with nim-waku. go-waku is easier to package into our build system compared to nim-waku.

EDIT: I forgot to mention we currently have no issues with the new table schema.

[oskarth]

go-waku is easier to package into our build system compared to nim-waku.

Could you elaborate a bit more on this requirement? Would nim-waku exposing Go language bindings (via a C API) help with this?

[staheri14]

@oskarth I am getting the same exact windows error as this PR https://github.com/status-im/nim-waku/pull/506/checks#step:7:1332. Going to merge as it seems the error is not caused by the current PR.

[sbc64]

go-waku is easier to package into our build system compared to nim-waku.

Could you elaborate a bit more on this requirement? Would nim-waku exposing Go language bindings (via a C API) help with this?

Briefly, I change my mind about this. Go bindings aren't necessary. The issue was trying to replicate: https://github.com/status-im/nimbus-build-system (lots of good work in here btw) with this: https://github.com/sbc64/nix-nim-waku. It is more of finding a way of easily compiling a wakunode binary without having to build the nim compiler (by using nix nim instead and I'm aware of this https://github.com/status-im/nimbus-eth1/tree/master/nix). I spent several days thinking about a technical justification "easier packaging with go" and concluded that it was just a pet peeve of mine for a leaner build process. The docker images at https://hub.docker.com/u/statusteamauto fulfill our needs at WC. So all is good @oskarth, thanks.