The MASP conversions query now requires a MASP epoch to be specified.

[murisi]

Describe your changes

Querying the conversion state becomes more difficult as nodes progress to higher and higher epochs. For instance, on a mainnet clone running on accelerated epochs, client queries for conversions always timeout (even after modifying timeout_broadcast_tx_commit) when the node goes beyond a certain MASP epoch. This happens because the conversion state grows linearly with the MASP epoch counter. This PR attempts to address this problem by making the conversions RPC endpoint require that clients specify the MASP epoch they want conversions from. This implies two things: first the size of the response from the conversions RPC endpoint should now be constant (equal to the number of tokens in the shielded rewards program), and second a client requiring all conversions now has to do a separate query for each MASP epoch.

Checklist before merging

• If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
  • This will require 2 reviewers to approve the changes
• If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
  • Relevant PR if applies:
• If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
  • Relevant PR if applies:

[brentstone]

@murisi - it was mentioned to me from @Fraccaman that we desire this work. Is this mergeable (I see the label that might suggest otherwise). Does it need review?

[murisi]

@murisi - it was mentioned to me from @Fraccaman that we desire this work. Is this mergeable (I see the label that might suggest otherwise). Does it need review?

@brentstone It's been a while since I've looked at this. I believe this PR did help when the mainnet clone's epochs got very high. I probably put the do-not-merge tag because these changes are not purely client-side; they change an RPC function. So there could be client-node version/compatibility issues arising. Yes, it needs a review and some testing to ensure that the PR is compatible with the latest releases of Namada, and that it has no adverse effects (due to having lots of smaller queries). The PR might also need to be touched up with a changelog entry.

[brentstone]

@batconjurer would you mind peeking at this?

[batconjurer]

This looks good to me. In the future, it would also be possible to speed up the server side processing by keying the conversions by epoch since right now the server still needs to do a linear scan over the whole conversion state. That is a much more invasive change though.

[mergify]

🧪 CI Insights

Here's what we observed from your CI run for 882fbbd.

🟢 All jobs passed!

But CI Insights is watching 👀

[sug0]

we can optimize conversion queries. right now, we need to wait for the full round-trip time of the conversions at a specific masp epoch. say we had 100 masp epochs to go through, and the RTT was 100 ms. we would need to wait 10 seconds for each query to be resolved. with 100 concurrent requests, ideally we would complete all queries in 100 ms. of course in practice they'll be slower, but there should be a major speed-up from launching these queries concurrently.

[sug0]

there's no reason why we cannot run these queries concurrently (e.g. with this). the Stream can be collected into a BTreeMap (see StreamExt::collect)

[grarco]

I've attempted this in 0d1074c even though I had to fold the maps instead of collect because of the limitations of the Extend trait on maps. Should we also add an extra argument to the query command to allow the user to specify the from epoch to avoid querying from 0 all the times?

[grarco]

Force-pushed to rebase on main

[sug0]

lgtm, just a small question

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!