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.

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

Request/Response adaptive timeouts #778

Open
rphmeier opened this issue Sep 14, 2022 · 6 comments
Open

Request/Response adaptive timeouts #778

rphmeier opened this issue Sep 14, 2022 · 6 comments
Labels
I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue.

Comments

@rphmeier
Copy link
Contributor

At the moment, the maximum timeout and response size are set for the entire request/response protocol. In fact, when the request is made with enough context it should be possible to configure the maximum timeout and response size accordingly. This would let us write more sophisticated networking protocols.

@eskimor
Copy link
Member

eskimor commented Sep 21, 2022

What are the exact goals - as in why is the timeout important/needed? The timeout in substrate is a hard cap, if it is hit we will cancel the existing download even if it already reached like 99% - wasting all the effort. The timeout has the potential of completely crippling a protocol if set too tight (no peer able to provide the data within the timeout).

What we used in the past, e.g. in availability-recovery is the notion of a soft timeout, when hit, we would start additional parallel requests, but leave the old ones running. Would that be applicable here?

@eskimor
Copy link
Member

eskimor commented Sep 21, 2022

Depending on the exact requirements, another option is also to play with queue sizes. For good distribution of load, if queue sizes on honest nodes are relatively small, we will get an error immediately when the peer is under load and don't have to waste time waiting for the timeout. In this scenario the timeout mostly exists to minimize harm malicious peers*) can have, hence we should be able to make it relatively generous.

*) and long latency between two particular peers.

@burdges
Copy link

burdges commented Sep 23, 2022

We could've subchunks in availability, complete with deeper merkle proof, if parablock size were every problematic for downloads.

@rphmeier
Copy link
Contributor Author

Yes, we can build higher-level timeout logic on top. The main goal is to do stuff like exponential back-off on requests and attempts and start with low timeouts with certain peers and move to higher ones.

@eskimor
Copy link
Member

eskimor commented Sep 23, 2022

Ok, I was actually aiming at one level deeper. Why do we want that exponential back-off, starting with low timeouts?

@rphmeier
Copy link
Contributor Author

rphmeier commented Sep 23, 2022

It may be useful in paritytech/polkadot#5999, for instance. Exponential back-off or other back-offs are useful in general as a tool in networking protocols, so it's good to make sure the low-level code can support such things.

@AndreiEres AndreiEres self-assigned this Jul 11, 2023
@Sophia-Gold Sophia-Gold transferred this issue from paritytech/polkadot Aug 24, 2023
@the-right-joyce the-right-joyce added I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue. T8-parachains_engineering and removed I4-annoyance labels Aug 25, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
* Added vault and vault tests

* Added skeleton for NativeTokens contract

* fmt

* removed underscores

* basic lock implemented

* using safeTransferFrom

* added decoding of messages

* fmt

* added docs

* changed event names

* fixed comments

* fixed typos in error

* added more docs

* moved messages to utils

* added deployment

* added tests for native tokens

* renamed unlock to doUnlock

* use custom errors

* get ERC20 metadata from the token

* get ERC20 metadata from the token

* removed message protocol

* lock and unlock use uint128

* removed docstrings

* removed not needed errors

* removed Uninitialized enum

* removed redundant comment
@AndreiEres AndreiEres removed their assignment May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I3-annoyance The node behaves within expectations, however this “expected behaviour” itself is at issue.
Projects
Status: Backlog
Status: To do
Development

No branches or pull requests

5 participants