Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: relax message size constraint #5298

Closed
wants to merge 1 commit into from
Closed

feat: relax message size constraint #5298

wants to merge 1 commit into from

Conversation

hansieodendaal
Copy link
Contributor

Description

Relaxed the message size constraint from 4 MiB to 5 MiB as it prevented faucet outputs from being streamed in an RPC request when a wallet requested all outputs.

Motivation and Context

Faucet outputs could not be streamed to the console wallet:

2023-04-06 17:08:52.630699700 [wallet::utxo_scanning] WARN  Failed to scan UTXO's from base node 6b8a34873128fc5323a86fbe6e: RpcStatus: `MalformedResponse: The response size exceeded the maximum allowed payload size. Max = 4.0000 MiB, Got = 4.5924 MiB`
2023-04-06 17:08:52.631420600 [wallet::utxo_scanning] WARN  Failed to scan UTXO's from base node 6b8a34873128fc5323a86fbe6e: RpcError: `Remote peer unexpectedly closed the RPC connection`
2023-04-06 17:08:52.631441300 [wallet::utxo_scanning] ERROR Error scanning UTXOs: Utxo Scanning Error: 'Failed to scan UTXO's after 2 attempt(s) using sync peer(s). Aborting...

How Has This Been Tested?

System-level testing on igor

What process can a PR reviewer use to test or verify this change?

System-level testing on igor

  • Prior to this PR: create a new wallet and monitor the log file for the error
  • With this PR: create a new wallet and verify in the log file that UTXO scanning succeeds

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@hansieodendaal
Relax message size constraint
a0a689b 
Relaxed the message size constraint from 4 MiB to 5 MiB as it prevented faucet
outputs being streamed in an RPC request when a wallet requested all outputs.
@ghpbot-tari-project ghpbot-tari-project added the P-acks_required Process - Requires more ACKs or utACKs label Apr 11, 2023
@SWvheerden SWvheerden mentioned this pull request Apr 11, 2023
Closed
SWvheerden
SWvheerden reviewed Apr 11, 2023
View reviewed changes
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on hind sight, perhaps better to it this way: #5299

@hansieodendaal
Copy link
Contributor Author

on hind sight, perhaps better to it this way: #5299

Maybe we can merge this, and revert the size constraint once #5299 is done so that testing on igor can continue.

@SWvheerden SWvheerden mentioned this pull request Apr 11, 2023
Merged
4 tasks
@SWvheerden
Copy link
Collaborator

This is an alternative to PR #5302

The issue we have is that the Genesis block has too many utxos to send over the wire with the 4mb size limit.
This should only be an issue with the Genesis block as with any other block the whole block should be less than 4mb.
We can increase the size limit, but then increase the size for everybody. The other PR slightly ever increases the send/receive code to handle streaming of sub block batches of utxos.

stringhandler pushed a commit that referenced this pull request Apr 12, 2023
@SWvheerden
feat!: add paging to utxo stream request (#5302)
3540309 
Description
---
Adds paging to the utxo stream to handle the large amount of faucet
utxos

Motivation and Context
---
See: #5299

How Has This Been Tested?
---
unit tests

Fixes: #5299

What process can a PR reviewer use to test or verify this change?
---
See PR #5298 


Breaking Changes
---
Changes request service

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
@SWvheerden
Copy link
Collaborator

Closing this in favour of: #5302

@SWvheerden SWvheerden closed this Apr 12, 2023
@hansieodendaal hansieodendaal deleted the ho_utxo_streaming_size branch May 15, 2023 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWvheerden SWvheerden SWvheerden approved these changes

Assignees
No one assigned
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hansieodendaal @SWvheerden @ghpbot-tari-project