backport: merge bitcoin#22879, #22762, #23041, #22734, #22950, #23053, #22839, #23140, #23306, #23354, #23380 (addrman backports: part 2) #6043
Conversation
This file contains 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
5 tasks
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
PastaPastaPasta
added a commit
that referenced
this pull request
Jun 7, 2024
, bitcoin#22697, bitcoin#22740, bitcoin#22849, bitcoin#22791, bitcoin#22848, bitcoin#22915, bitcoin#22911, bitcoin#22974, bitcoin#20234 (addrman backports) f619f8f merge bitcoin#20234: don't bind on 0.0.0.0 if binds are restricted to Tor (Kittywhiskers Van Gogh) 1698336 merge bitcoin#22974: Improve performance of Good (Kittywhiskers Van Gogh) 29f4482 merge bitcoin#22911: Minor cleanups to asmap (Kittywhiskers Van Gogh) 5706eda merge bitcoin#22915: Remove confusing CAddrDB (Kittywhiskers Van Gogh) 3f69606 merge bitcoin#22848: Expose BanMapToJson / BanMapFromJson (Kittywhiskers Van Gogh) 9065eed merge bitcoin#22791: Fix asmap/addrman initialization order bug (Kittywhiskers Van Gogh) 99b7812 merge bitcoin#22849: Remove unused SERIALIZE_METHODS for CBanEntry (Kittywhiskers Van Gogh) a30379c merge bitcoin#22740: Move serialization code to cpp (Kittywhiskers Van Gogh) d4e79aa merge bitcoin#22697: Remove CAddrMan::Clear() function (Kittywhiskers Van Gogh) 77d8f6c merge bitcoin#22725: Move addrman ser/deser tests to addrman_tests.cpp (Kittywhiskers Van Gogh) 4ba3f49 merge bitcoin#22627: De-duplicate Add() function (Kittywhiskers Van Gogh) 49af818 merge bitcoin#20233: Make consistency checks a runtime option (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6043 * [bitcoin#22915](bitcoin#22915) is backported before [bitcoin#21850](bitcoin#21850), which is the reason {`Dump`, `Read`}`PeerAddresses` takes in an `const ArgsManager&` but doesn't do anything with it. * This will need to be accounted for when backporting [bitcoin#21850](bitcoin#21850) ## Breaking Changes None expected. No changes to serialization format. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK f619f8f Tree-SHA512: 19599f0322f049b123790a45530ecb2ec74b1463b47e0102921b489bad1d5cb671a94900b3b35dbd7c1d80bc0680f1ed01e6fac675ac0be81621878388be0bcd
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
mostly; LGTM a93fec6; see a few comments with unrelated changes it seems
| { | ||
| AssertLockHeld(cs); | ||
| LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size()); |
There was a problem hiding this comment.
22734: why'd this change?
src/addrman.cpp
Outdated
| @@ -569,7 +569,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT | |||
| AddrInfo& info = *pinfo; | |||
|
|
|||
| // check whether we are talking about the exact same CService (including same port) | |||
| if (info != addr) | |||
| if (!m_discriminate_ports && info != addr) | |||
There was a problem hiding this comment.
23306: changes unrelated?
Dash has supported the storage of address-port pairs and multi-port support before upstream for ease of development (on select networks, most notably, not mainnet). Bitcoin has introduced support for the above mentioned features and has modified tests to validate new functionality. As Dash already has this functionality and *selectively* allows it, we need to test both cases, the new upstream case with distinguishment enabled and Dash's case of distinguishment disabled (the former upstream case).
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jun 18, 2024
76b4300 fix: Let addrman handle multiple ports for all networks (UdjinM6) Pull request description: ## Issue being fixed or feature implemented There is really no need to do have this restriction in `addrman`, we check `AllowMultiplePorts()` in `connman` which should be enough already. We can safely re-align `addrman` to its upstream implementation as suggested in #6043. ## What was done? Drop port "discrimination" in `addrman`, remove related tests. ## How Has This Been Tested? Run tests, run dash-qt on mainnet/testnet ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: ACK 76b4300 PastaPastaPasta: utACK 76b4300; is it really a fix or a refactor? doesn't change observable system behavior knst: utACK 76b4300 Tree-SHA512: 50363b5de7a6c6ad4de18c08b0a5cc31e89be8e5304f9684ce2cc609df4de07ff6d8d010556b5f6651c698e1a86960dba8005cc26fdb00bd03c856752d3e06ef
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jun 27, 2024
…g of inconsistent peers.dat adba609 addrman: allow for silent overwriting of inconsistent peers.dat (Kittywhiskers Van Gogh) fbb2b51 merge bitcoin#26909: prevent peers.dat corruptions by only serializing once (Kittywhiskers Van Gogh) Pull request description: ## Additional Information [bitcoin#22762](bitcoin#22762) (backported as part of [dash#6043](#6043)) did away with then-existing behaviour of overwriting `peers.dat` silently if found corrupt with the rationale of preventing situations where the wrong file is pointed at or the file is written by a higher version of Core. Alongside a change in behaviour, refactoring also took place and further changes were built on top of them. Since then, there have been reports of an increasing number of "Corrupt data. Consistency check failed with code -5: iostream error" errors from builds based on `develop`. Reverting the pull request that introduced this change in behaviour is non-trivial due to the number of backports that build on top of the refactoring brought along with it. Nor were any other error messages found except for the one mentioned above. The tendency for `peers.dat` to corrupt itself has also been documented upstream ([bitcoin#26599](bitcoin#26599)), with the issue marked as closed with the merger of [bitcoin#26909](bitcoin#26909). Therefore, to remedy the above problem, alongside backporting [bitcoin#26909](bitcoin#26909), to avoid inconvenience, instead of reverting all progress made from backporting (as the benefits of not overwriting `peers.dat` for having the wrong magic altogether, for example, is something that doesn't need to be reverted), only inconsistent `peers.dat` files will be overwritten and the action logged with no user intervention required. ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK adba609 knst: utACK adba609 PastaPastaPasta: utACK adba609 Tree-SHA512: 3e09e7a77c82cce333fe9f02f137485e362f7816c450aef3d18950b9fd57276b4a21cbd1fe90b3eefd62ede0947970ed367c5917930a0656833bc38c0629e408
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
fAllowMultiplePortsenabled (which at the time wasdevnetand as it stands currently, on every network exceptmainnet).mainnetis outside the scope of this PR), when backporting bitcoin#23306, changes have been made to retain the effects ofdiscriminate_ports.!m_discriminate_portscondition to behaviour that otherwise would be removed entirely.addrman_tests(the old default was to keep it disabled, to mirrormainnetand pre-change upstream behaviour)._nondiscriminatesuffix.I would propose at some point to rename the flag to
ignore_port/suppress_port(if not remove it altogether should themainnetrestriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished using ports (i.e. considered) or ports will be discriminated against (i.e. ignored).Breaking Changes
It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on
mainnetby setting zero-ing out the port (source), meaning even with port discrimination disabled, the serialization format should remain the same.Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.