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

neutrino: improve perf in high latency networks #300

Merged
merged 1 commit into from
Jun 20, 2024
Merged

neutrino: improve perf in high latency networks #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion neutrino.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ func NewChainService(cfg Config) (*ChainService, error) {
if s.persistToDisk {
cfg := &chanutils.BatchWriterConfig[*filterdb.FilterData]{
QueueBufferSize: chanutils.DefaultQueueSize,
MaxBatch: 1000,
MaxBatch: 10,
djkazic marked this conversation as resolved.
Show resolved Hide resolved
DBWritesTickerDuration: time.Millisecond * 500,
PutItems: s.FilterDB.PutFilters,
}
Expand Down
2 changes: 1 addition & 1 deletion query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ var (
// each peer before we've concluded we aren't going to get a valid
// response. This allows to make up for missed messages in some
// instances.
QueryNumRetries = 2
QueryNumRetries = 8

// QueryPeerConnectTimeout specifies how long to wait for the
// underlying chain service to connect to a peer before giving up
Expand Down
2 changes: 1 addition & 1 deletion rescan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,7 @@ func (rs *rescanState) handleBlockConnected(ntfn *blockntfns.Connected) error {

// Otherwise, we'll attempt to fetch the filter to retrieve the relevant
// transactions and notify them.
queryOptions := NumRetries(0)
queryOptions := NumRetries(2)
Copy link
Member

Choose a reason for hiding this comment

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

would it not make sense for high latency environments to instead make the timeout longer instead of bumping num retries? (or both lol)

also - wondering if we should make this configurable rather than hard code it?

Copy link
Contributor Author

@djkazic djkazic May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. So the timeouts and retries are directly connected to one another, as when we have a timeout we'll do a retry (bounded by the retry count). What we found from trace logs was that the networks didn't consistently timeout, but rather during higher bandwidth utilization there was a chance of hitting the low retry count limit. That's why we raised the retries, since raising the timeout would also indirectly influence the amount of extra timeout added (it doubles each time it fails).

In my highly non-scientific testing there seemed to be a trend where if we bumped timeouts alone, reliability would still be bad. So this is just kind of the solution that we converged.

  1. Yes, I've thought about making them configurable settings. However, we did a grid search and these were the best settings that balanced perf for slow and fast networks. I figured a follow-up PR where we break out these as configurables would be good, but let me know if we should bundle the changes. This PR is mostly focused on "saner" defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it not make sense for high latency environments to instead make the timeout longer instead of bumping num retries? (or both lol)

Increasing timeout is kinda a bigger change as the number gets increased exponentially after each try.
I think increasing both retry and timeout makes sense. But I could look into that in a follow-up PR instead.

also - wondering if we should make this configurable rather than hard code it?

Yeah, I've thought of that too. It would be a very nice thing to have.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha gotcha

blockFilter, err := chain.GetCFilter(
newStamp.Hash, wire.GCSFilterRegular, queryOptions,
)
Expand Down
Loading