chore: bump retries for request tx by hash#13675
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Maddiaa0
force-pushed
the
md/bump-retries
branch
2 times, most recently
from
bfb1903 to
20f4a99
Compare
| // Set collective timeout based on the number of txs we are requesting | ||
| const timeoutMs = Math.max(5000, txHashes.length * 100); | ||
| const maxPeers = Math.ceil(txHashes.length / 3); | ||
| const maxRetryAttempts = Math.min(txHashes.length / maxPeers, 5); |
There was a problem hiding this comment.
I may be missing something, but isn't this always equal to 3, since maxPeers is already defined based on txHashes.length?
| const txs = await this.p2pService.sendBatchRequest(ReqRespSubProtocol.TX, txHashes); | ||
| // Set collective timeout based on the number of txs we are requesting | ||
| const timeoutMs = Math.max(5000, txHashes.length * 100); | ||
| const maxPeers = Math.ceil(txHashes.length / 3); |
There was a problem hiding this comment.
Shouldn't we set a minimum of let's say 5, in case we're requesting very few txs?
| public async requestTxsByHash(txHashes: TxHash[]): Promise<(Tx | undefined)[]> { | ||
| const txs = await this.p2pService.sendBatchRequest(ReqRespSubProtocol.TX, txHashes); | ||
| // Set collective timeout based on the number of txs we are requesting | ||
| const timeoutMs = Math.max(5000, txHashes.length * 100); |
There was a problem hiding this comment.
I'd put a hard cap on this one.
There was a problem hiding this comment.
We also need to ensure this is capped by the validation reexecution deadline, but that's another story.
There was a problem hiding this comment.
Same for prover node's proving deadline
There was a problem hiding this comment.
For re-ex deadline, if it times out as it is unable to get all of the txs, then it will not be able to attest anyway no?
PhilWindle
left a comment
PhilWindle
left a comment
There was a problem hiding this comment.
Slight concern over request timeouts.
| public async requestTxsByHash(txHashes: TxHash[]): Promise<(Tx | undefined)[]> { | ||
| const txs = await this.p2pService.sendBatchRequest(ReqRespSubProtocol.TX, txHashes); | ||
| // Set collective timeout based on the number of txs we are requesting | ||
| const timeoutMs = Math.min(8000, txHashes.length * 100); |
There was a problem hiding this comment.
Slight concern over this. From the looks of how this all works, if I need say 2 txs, we would only allow 200ms for the full round trip. Is that correct?
I think that is too low. I think there is a fixed time delay to any request, regardless of how large, for the request to go across the world, the recipient to open their db and the response to come all the way back again. Given that these requests should in general be for small numbers of transactions I'm worried this change could make things worse.
I think at the very least it should be something like
const timeoutMs = Math.min(8000, Math.max(txHashes.length * 100, 1000));
073526b to
86bd4fc
Compare
4 participants
Overview
With better sampling logic, we can more safetly bump the retries for the tx requests