backport: merge bitcoin#20146, #20477, #20624, #19972, #20724, #19884, #21165, #20721, #21254, partial bitcoin#19829 (networking backports) #5952
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
UdjinM6
reviewed
Mar 23, 2024
kwvg
changed the title
src/net.cpp
Outdated
Member
There was a problem hiding this comment.
20477: nit: missing added optional include? probably compiles fine due to other headers already including; but we should IWYU
src/net.cpp
Outdated
Comment on lines
+1074
to
+1061
Member
There was a problem hiding this comment.
20477: nit whitespace diff with upstream could result in conflicts in future
PastaPastaPasta
approved these changes
Mar 24, 2024
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK 99e87c98fab273965b05ea2b2d366bf925bca234
Makes resolving merge conflicts easier
… peers.dat is empty
Collaborator
Author
|
Branch re-pushed to have commits arranged in order of upstream merger. No changes in diff or conflicts caused by re-org. |
kwvg
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Additional Information
bitcoin#19884 doesn't seem to play nice on its own and necessitated two more backports (namely bitcoin#21165 and bitcoin#21254) before it did.
Trying to find why that is has been time consuming but there doesn't seem to be a concrete answer. If running the daemon normally (
dashd --regtest --dnsseed=1), the functionality behaves as expected but the test still fails.The closest explanation is that our OOO backports with relation to mocking time could explain why it isn't working as expected due to debug statements I added that always shown the time delta between each "enough time has passed" check was 0 seconds even when the log was advancing forward in time.
The usage of
dnsseed=0stems from bitcoin#16551 (link to diff in commit comment), a backport that was skipped due to complexity. Though, some aspects of the PR have made it with dash#3946.bitcoin#19829 does away with
CConnman::ForEachNodeusage inPeerManagerImpl::UpdatedBlockTip(source). Dash cannot do the same and continues to useForEachNodeas we use theCNode::CanRelaycheck, which is not accessible through thePeerstruct.m_masternode_connection) and migrate them toPeerto avoid Dash-specific patches and closer alignment with upstream.Breaking Changes
Potential change in behaviour in the GUI and RPC.
In RPC,
getpeerinfowill now displaypingwaitandstartingheightiffStateStatsis true (earlier behaviour was unconditional).startingheighthas been placed belowbanscore(earlier behaviour placed it above). In the GUI (Qt),peerHeightandpeerPingWaitare subject to similar conditionality as mentioned earlier.No changes in protocol or consensus. Changes are primarily related to refactoring, cleaning up, improving networking code and adding a new flag (
-fixedseeds).Checklist: