Closed fryorcraken closed 1 year ago
Waku store page size limit expected behaviour:
Checking the logs from nwaku, the issue seems that comes from the query:
❯ grep -E 'pageSize: [[:digit:]]+' ~/Downloads/nwaku_Waku_Store_Callback_on_promise._aborts_when_callback_returns_true.log
INF 2023-01-27 15:43:14.345+11:00 received history query topics="waku store" tid=2037443 file=protocol.nim:73 peerId=12D*aVcnPY requestId=d5b9c0ec-793f-4455-b3f5-3ea64d9d80a6 query="(pubsubTopic: some(\"/waku/2/default-waku/proto\"), contentTopics: @[\"/test/1/waku-store/utf8\"], cursor: none(HistoryCursor), startTime: none(CompiledIntTypes), endTime: none(CompiledIntTypes), pageSize: 18446744073709551612, ascending: false)"
Note the pageSize: 18446744073709551612
. As there are only 20 items the database, it returns all of them.
In other words:
The number
18446744073709551612
is greater than the max page size limit. So the maximum number of messages in the response would be 100. But, as there are only 20 in the node's Waku archive, the store node responds with all the messages it has, 20.
Waku store page size limit expected behaviour:
- If no page size limit is not specified (not present), the limit defaults to 20.
- If the page size limit is set to 0, the limit defaults to 20.
- If the page limit is grater than the max page size limit (100), the limit is set to 100.
Checking the logs from nwaku, the issue seems that comes from the query:
❯ grep -E 'pageSize: [[:digit:]]+' ~/Downloads/nwaku_Waku_Store_Callback_on_promise._aborts_when_callback_returns_true.log INF 2023-01-27 15:43:14.345+11:00 received history query topics="waku store" tid=2037443 file=protocol.nim:73 peerId=12D*aVcnPY requestId=d5b9c0ec-793f-4455-b3f5-3ea64d9d80a6 query="(pubsubTopic: some(\"/waku/2/default-waku/proto\"), contentTopics: @[\"/test/1/waku-store/utf8\"], cursor: none(HistoryCursor), startTime: none(CompiledIntTypes), endTime: none(CompiledIntTypes), pageSize: 18446744073709551612, ascending: false)"
Note the
pageSize: 18446744073709551612
. As there are only 20 items the database, it returns all of them.In other words:
The number
18446744073709551612
is greater than the max page size limit. So the maximum number of messages in the response would be 100. But, as there are only 20 in the node's Waku archive, the store node responds with all the messages it has, 20.
Yes but I sent 7
so why does it decode 18446744073709551612
? Has the Protobuf changed?
Also note that in the js-waku branch I provided to reproduce, one needs to pull origin master for in the nwaku submodule to reproduce with latest master.
Yes but I sent 7 so why does it decode 18446744073709551612? Has the Protobuf changed?
Nothing has changed in the Waku store protocol (which acts as an RPC query mechanism for the Waku archive service). I only fixed the Waku archive bug.
The page size limit continues being a uint64
in the protobuf:
https://github.com/vacp2p/waku/blob/main/waku/store/v2beta4/store.proto#L16-L18
And nwaku decodes it as:
https://github.com/waku-org/nwaku/blob/master/waku/v2/protocol/waku_store/rpc_codec.nim#L87-L91
~I need to check if the zint64
type has changed.~ Edit: Nothing has changed in nim-libp2p's minprotobuf code 😕
This number 18446744073709551612
(0xFFFFFFFFFFFFFFFC
) smells as an encoding issue. Where? I am not sure.
I will investigate more but the same test passes with v0.13.0 and fails with v0.14.0 so I am dubious on the fact that it comes from js-waku.
Ran nwaku v0.13.0 and v0.14.0 binary with the same js-waku software and test suite. Both node systematically interprets the pageSize differently:
nwaku v0.13.0:
INF 2023-01-30 21:02:19.398+11:00 received history query topics="waku store" tid=2294776 file=protocol.nim:232 peerId=12D*cGyWR3 requestId=8bb7f101-7af7-4372-a941-d360daa7ee2d query="(contentFilters: @[(contentTopic: \"/test/1/waku-store/utf8\")], pubsubTopic: \"/waku/2/default-waku/proto\", pagingInfo: (pageSize: 7, cursor: (pubsubTopic: \"\", senderTime: 0, receiverTime: 0, digest: (data: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])), direction: BACKWARD), startTime: 0, endTime: 0)"
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
nwaku v0.14.0:
INF 2023-01-30 21:05:02.052+11:00 received history query topics="waku store" tid=2295273 file=protocol.nim:73 peerId=12D*gr4ebx requestId=8dbacdf7-1177-4a46-8ceb-55f5eafea84c query="(pubsubTopic: some(\"/waku/2/default-waku/proto\"), contentTopics: @[\"/test/1/waku-store/utf8\"], cursor: none(HistoryCursor), startTime: none(CompiledIntTypes), endTime: none(CompiledIntTypes), pageSize: 18446744073709551612, ascending: false)"
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
Next step I was going to do is test with go-waku as a client instead of js-waku. I am not saying that js-waku protobuf encoding can be trusted, only saying it hasn't changed.
I am also dumping the nwaku Waku store client protobuf to double-check if anything changed between versions.
I am also dumping the nwaku Waku store client protobuf to double-check if anything changed between versions.
This is the dump of a pageSize: 7
query:
0a 14 63 65 63 39 34 36 39 65 65 62 65 62 64 38 30 33 31 36 32 66 12 27 1a 1f 0a 1d 2f 77 61 6b 75 2f 32 2f 64 65 66 61 75 6c 74 2d 63 6f 6e 74 65 6e 74 2f 70 72 6f 74 6f 22 04 08 0e 18 00
Which translates into the following dissection (https://protobuf-decoder.netlify.app/):
@fryorcraken Could you run the same test changing the following line's type to a signed int64?
https://github.com/waku-org/js-waku/blob/master/packages/proto/src/lib/store.ts#L134
@fryorcraken Could you run the same test changing the following line's type to a signed int64?
waku-org/js-waku@
master
/packages/proto/src/lib/store.ts#L134
Using proto definition:
message PagingInfo {
optional sint64 page_size = 1;
optional Index cursor = 2;
enum Direction {
DIRECTION_BACKWARD_UNSPECIFIED = 0;
DIRECTION_FORWARD = 1;
}
optional Direction direction = 3;
}
v0.14.0 behaviour matches expectations
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
INF 2023-01-30 23:19:27.742+11:00 received history query topics="waku store" tid=2308316 file=protocol.nim:73 peerId=12D*pyb2Nu requestId=de11d517-4d32-4e31-a2d6-91fdecaf9113 query="(pubsubTopic: some(\"/waku/2/default-waku/proto\"), contentTopics: @[\"/test/1/waku-store/utf8\"], cursor: none(HistoryCursor), startTime: none(CompiledIntTypes), endTime: none(CompiledIntTypes), pageSize: 7, ascending: false)"
However, v0.13.0 now interprets the expected value of 7 as 14 (as per your comment above)
INF 2023-01-30 23:26:17.648+11:00 received history query topics="waku store" tid=2309232 file=protocol.nim:232 peerId=12D*QxmUSB requestId=ead804a7-ab72-4f21-b531-01d0e6a13578 query="(contentFilters: @[(contentTopic: \"/test/1/waku-store/utf8\")], pubsubTopic: \"/waku/2/default-waku/proto\", pagingInfo: (pageSize: 14, cursor: (pubsubTopic: \"\", senderTime: 0, receiverTime: 0, digest: (data: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])), direction: BACKWARD), startTime: 0, endTime: 0)"
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
Do note the current RFC expects uint64
(I know store protocol is getting revamped):
message PagingInfo {
uint64 pageSize = 1;
Index cursor = 2;
enum Direction {
BACKWARD = 0;
FORWARD = 1;
}
Direction direction = 3;
}
Note: the js-waku test pass whether the value is interpreted as 7
or 14
due to how it's setup.
Ok, this confirms my suspicions.
Do note the current RFC expects uint64 (I know store protocol is getting revamped):
Yes, that is correct. It should be a uint64
, not a sint64
. A non-trivial issue could explain this in nwaku's Waku store RPC codec. It is not generated from protobuf files; it is manually written using nim-libp2p's minprotobuf library.
The Waku Store rpc_codec
at v0.13.0
: https://github.com/waku-org/nwaku/blob/v0.13.0/waku/v2/protocol/waku_store/rpc_codec.nim#L82-L84
var pageSize: uint64
discard ?pb.getField(1, pageSize)
pagingInfo.pageSize = pageSize
The same codec in master
(v0.14.0+): https://github.com/waku-org/nwaku/blob/master/waku/v2/protocol/waku_store/rpc_codec.nim#L87-L91
var pageSize: zint64
if not ?pb.getField(1, pageSize):
rpc.pageSize = none(uint64)
else:
rpc.pageSize = some(uint64(pageSize))
This type change from uint64
to zint64
explains the issue observed. Let me check if changing it back to uint64
solves the issue.
@fryorcraken Could you run the tests against the branch associated with PR #1520?
@richard-ramos Could you check if any changes are necessary for go-waku
to be compatible with this fix?
@fryorcraken Could you run the tests against the branch associated with PR #1520?
Confirmed that this resolves the issue:
INF 2023-01-31 14:45:03.276+11:00 received history query topics="waku store" tid=2363182 file=protocol.nim:73 peerId=12D*YimyUB requestId=1d0826f1-6016-48de-82d4-b41aa85892cf query="(pubsubTopic: some(\"/waku/2/default-waku/proto\"), contentTopics: @[\"/test/1/waku-store/utf8\"], cursor: some((pubsubTopic: \"/waku/2/default-waku/proto\", senderTime: 1675136702339000000, storeTime: 1675136702339000000, digest: (data: [50, 126, 61, 105, 234, 47, 218, 202, 51, 78, 99, 55, 209, 47, 149, 8, 157, 197, 31, 27, 9, 94, 122, 71, 40, 117, 100, 28, 234, 179, 251, 56]))), startTime: none(CompiledIntTypes), endTime: none(CompiledIntTypes), pageSize: 7, ascending: false)"
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
PR #1520 was merged. Closing the issue since the fix was confirmed to work by the js-waku interop tests.
Problem
When sending a query with page size 7, it returns 20 messages per page.
Impact
Sounds like a critical regression issue.
To reproduce
From a js-waku clone:
nwaku logs will be present in
./log
folderNote nwaku is executed with the following arguments:
and env var:
Screenshots/logs
nwaku logs:
Looks like decoding error:
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
Note the proto definition in js-waku:
js-waku logs:
nwaku version/commit hash
93a07babf28a264c2776e9e482fb533f0fa5987d