-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug: page size limit not respected #1514
Comments
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 In other words:
|
Yes but I sent 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. |
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 https://github.com/vacp2p/waku/blob/main/waku/store/v2beta4/store.proto#L16-L18 And nwaku decodes it as:
|
This number |
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:
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log nwaku v0.14.0:
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. |
This is the dump of a
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 |
Using proto definition:
v0.14.0 behaviour matches expectations nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log
However, v0.13.0 now interprets the expected value of 7 as 14 (as per your comment above)
nwaku_Waku_Store_Callback_on_promise,_aborts_when_callback_returns_true.log Do note the current RFC expects
Note: the js-waku test pass whether the value is interpreted as |
Ok, this confirms my suspicions.
Yes, that is correct. It should be a |
The Waku Store var pageSize: uint64
discard ?pb.getField(1, pageSize)
pagingInfo.pageSize = pageSize The same codec in var pageSize: zint64
if not ?pb.getField(1, pageSize):
rpc.pageSize = none(uint64)
else:
rpc.pageSize = some(uint64(pageSize)) This type change from |
@fryorcraken Could you run the tests against the branch associated with PR #1520? @richard-ramos Could you check if any changes are necessary for |
Confirmed that this resolves the issue:
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
The text was updated successfully, but these errors were encountered: