search

celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
17 stars 16 forks source link

misc(syncer): make timeout configurable #155

Closed vgonkivs closed 3 months ago

vgonkivs commented 4 months ago

We should add additional field to the syncer config struct - maxAwaitingTime that specifies max time for processing Headrequest in case we are getting it from the network. This field should be configurable on the startup.

AryanGodara commented 3 months ago

Can I work on this issue @vgonkivs? Also, in case a value isn't provided, do we take 2s as the default value?

vgonkivs commented 3 months ago

Hey, @AryanGodara. Yes, sure you can take it. Thank you 🙂. Yes, the default value for maxAwaitingTime should be 2 seconds.

AryanGodara commented 3 months ago

Hey, @AryanGodara. Yes, sure you can take it. Thank you 🙂. Yes, the default value for maxAwaitingTime should be 2 seconds.

Thanks @vgonkivs :D I'll be pushing a PR for this shortly!

renaynay commented 3 months ago

Making a comment here, and this is a broader comment about configuration options in celestia-node / go-header:

Not everything deserves to be a configurable parameter. IMO, it's not worth the additional code to allow something as granular as a context timeout on a head request inside the syncer to be configurable. We should set a good constant and keep to it, and if it proves ineffective, then we bump it or try to figure out why requests are taking so long.

We should be a bit more picky about what deserves configurability vs what we can just hardcode. It is unlikely anyone external to the development team will try to configure this param.

renaynay commented 3 months ago

Closing as unplanned

Wondertan commented 3 months ago

Agreed. @AryanGodara, I suggest looking at https://github.com/celestiaorg/go-header/issues/149 instead