net: don't retry failed oneshot connections forever #12329
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laanwj
reviewed
Feb 1, 2018
| @@ -1953,29 +1952,29 @@ void CConnman::ThreadOpenAddedConnections() | |||
| } | |||
|
|
|||
| // if successful, this moves the passed grant to the constructed node | |||
| bool CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot, bool fFeeler, bool manual_connection) | |||
| void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot, bool fFeeler, bool manual_connection) | |||
There was a problem hiding this comment.
Do we really want to remove the return argument here? I think it's useful for it to be able to signal failure, for example to log an error, even if we don't use that at the moment.
There was a problem hiding this comment.
Yes, this is actually a really big help for the libevent code. This needs to be a one-way function so that it can be run async, with failures optionally triggering callbacks.
The oneshot reinsertion was the only thing preventing making that change trivially.
Edit: See here for that change on the libevent PR: 03e73a7. Notice the only thing the failed-connect callback does: 03e73a7#diff-9a82240fe7dfe86564178691cc57f2f1R2013
|
utACK 660f5f1 |
|
utACK 660f5f1 |
|
@sipa mentioned on IRC that these were originally retried because they may be tor seed attempts, which can only try one address, as opposed to normal seed resolves which may return a long list. So by not retrying these, if using tor, you only get seednode_count chances to make a connection and learn some new addresses. To avoid that, I think we should just re-run the entire ThreadDNSAddressSeed() as necessary until we have the desired connections. |
|
utACK 660f5f1 |
Let's leave that for a future PR, and take this fix for rc2. |
laanwj
added a commit
that referenced
this pull request
Feb 2, 2018
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd
HashUnlimited
pushed a commit
to chaincoin/chaincoin
that referenced
this pull request
Mar 16, 2018
Github-Pull: bitcoin#12329 Rebased-From: 660f5f1 Tree-SHA512: 7e7401b0ade3a2482dd246cc92c795230b37001a13fd7d050847ea532b619106dfdfc113e95c2891c689d421b3dd775d1833789061cd90dc094f13c4f5f6b278 Conflicts: src/net.cpp src/net.h
lyricidal
added a commit
to TheArcadiaGroup/DAPS
that referenced
this pull request
Nov 17, 2019
"As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was." bitcoin/bitcoin#12329
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 26, 2020
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 26, 2020
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 26, 2020
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 27, 2020
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 27, 2020
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Jun 25, 2021
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves.
Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it.
Maybe @sipa can shed a light on what the original intention was.