| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
Exposing address manager table entries in a hidden RPC allows to introspect
addrman tables in tests and during development.
As response JSON object the following FORMAT1 is choosen:
{
"table": {
"<bucket>/<position>": { "address": "..", "port": .., ... },
"<bucket>/<position>": { "address": "..", "port": .., ... },
"<bucket>/<position>": { "address": "..", "port": .., ... },
...
}
}
An alternative would be FORMAT2
{
"table": {
"bucket": {
"position": { "address": "..", "port": .., ... },
"position": { "address": "..", "port": .., ... },
..
},
"bucket": {
"position": { "address": "..", "port": .., ... },
..
},
}
}
FORMAT1 and FORMAT2 have different encodings for the location of the
address in the address manager. While FORMAT2 might be easier to process
for downstream tools, it also mimics internal addrman mappings, which
might change at some point. Users not interested in the address location
can ignore the location key. They don't have to adapt to a new RPC
response format, when the internal addrman layout changes. Additionally,
FORMAT1 is also slightly easier to to iterate in downstream tools. The
RPC response-building implemenation complexcity is lower with FORMAT1
as we can more easily build a "<bucket>/<position>" key than a multiple
"bucket" objects with multiple "position" objects (FORMAT2).
|
|
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
|
|
`Assume` is safer since the checks are non-fatal- errors in these functions
should provide feedback in debug builds, but do not need to deter further node
operations in production.
|
|
retain the values needed to prevent redundant node lookups
|
|
Add an optional parameter to the addrman Select function that allows callers to
specify which network the returned address should be on. Ensure that the proper
table is selected with different cases of whether the new or tried table has
network addresses that match.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/fChanceFactor/chance_factor/g' src/addrman.cpp
sed -i 's/nBucketPos/initial_position/g' src/addrman.cpp
sed -i 's/nBucket/bucket/g' src/addrman.cpp src/addrman_impl.h
sed -i 's/newOnly/new_only/g' src/addrman.cpp src/addrman_impl.h src/addrman.h src/test/addrman_tests.cpp
-END VERIFY SCRIPT-
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
in preparation for consolidating the logic for searching the new and tried
tables, generalize the call paths for both
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Unused until later commit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Extract the logic that decides whether the new or the tried table is going to
be searched to the beginning of the function.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
CService and use better naming
c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91fb12fba516dee896f1f97692cfa2104
achow101:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
jonatack:
re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
|
|
improve precision of adding fixed seeds
80f39c99ef2d30e3e2d8dbc068d25cf92aa32344 addrman, refactor: combine two size functions (Amiti Uttarwar)
4885d6f197736cb89fdfac250b280ec10829d903 addrman, refactor: move count increment into Create() (Martin Zumsande)
c77c877a8e916878e09f64b2faa12eeca7528cc8 net: Load fixed seeds from reachable networks for which we don't have addresses (Martin Zumsande)
d35595a78a4a6cae72d3204c1ec3f82f77a10d56 addrman: add function to return size by network and table (Martin Zumsande)
Pull request description:
AddrMan currently doesn't track the number of its entries by network, it only knows the total number of addresses. This PR makes AddrMan keep track of these numbers, which would be helpful for multiple things:
1. Allow to specifically add fixed seeds to AddrMan of networks where we don't have any addresses yet - even if AddrMan as a whole is not empty (partly fixing #26035). This is in particular helpful if the user abruptly changes `-onlynet` settings (such that addrs that used to be reachable are no longer and vice versa), in which case they currently could get stuck and not find any outbound peers. The second commit of this PR implements this.
1. (Future work): Add logic for automatic connection management with respect to networks - such as making attempts to have at least one connection to each reachable network as suggested [here](https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1249420209). This would involve requesting an address from a particular network from AddrMan, and expanding its corresponding function `AddrMan::Select()` to do this requires internal knowledge of the current number of addresses for each network and table to avoid getting stuck in endless loops.
1. (Future work): Perhaps display the totals to users. At least I would find this helpful to debug, the existing option (`./bitcoin-cli -addrinfo`) is rather indirect by doing the aggregation itself in each call, doesn't distinguish between new and tried, and being based on `AddrMan::GetAddr()` it's also subject to a quality filter which we probably don't want in this spot.
ACKs for top commit:
naumenkogs:
utACK 80f39c9
stratospher:
ACK 80f39c9
achow101:
ACK 80f39c99ef2d30e3e2d8dbc068d25cf92aa32344
vasild:
ACK 80f39c99ef2d30e3e2d8dbc068d25cf92aa32344
Tree-SHA512: 6359f2e3f4db7c120c0789d92d74cb7d87a2ceedb7d6a34b5eff20c7f55c5c81092d10ed94efe29afc1c66947820a0d9c14876ee0c8d1f8e068a6df4e1131927
|
|
The functionality of the old size() is covered by the new Size()
when no arguments are specified, so this does not change behavior.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Create() is only called in one spot, so this doesn't
change behavior.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
|
|
For now, the new functionality will be used in the context of
querying fixed seeds. Other possible applications for
future changes is the use in the context of making automatic
connections to specific networks, or making more detailed info
about addrman accessible via rpc.
|
|
The previous logic would call it once for serializing into the filestream,
and then again for serializing into the hasher. If AddrMan was changed
in between these calls by another thread, the resulting peers.dat would
be corrupt with non-matching checksum and data.
Fix this by using HashedSourceWriter, which writes the data
to the underlying stream and keeps track of the hash in one go.
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
|
|
Both methods do the same thing, so simplify to having just one.
Further, `CService` inherits `CNetAddr` and `CService::ToString()`
overrides `CNetAddr::ToString()` but the latter is not virtual which
may be confusing. Avoid such a confusion by not having non-virtual
methods with the same names in inheritance.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren nLastTry m_last_try
ren nLastSuccess m_last_success
ren nLastGood m_last_good
ren nLastCountAttempt m_last_count_attempt
ren nSinceLastTry since_last_try
ren nTimePenalty time_penalty
ren nUpdateInterval update_interval
ren fCurrentlyOnline currently_online
-END VERIFY SCRIPT-
|
|
|
|
All other places calculate "now - nLastTry", which is safe and correct
to do when nLastTry is 0. So do the same here.
|
|
nTime is always initialized on deserialization or default-initialized
with TIME_INIT, so special casing 0 does not make sense.
|
|
This makes the code less verbose. Also, future changes that change how
to get the time are less verbose.
Moreover, GetAdjustedTime() might arbitrarily change the value during
the execution of this function. For example, the system time advances
over a second boundary, or the network adjusts the time arbitrarily.
Most of the time however the value will not change, so it seems better
to always lock the value in this scope for clarity.
|
|
|
|
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
|
|
These currently call through to the CNetAddr methods. The logic will be moved in a future commit.
|
|
|
|
|
|
Also remove uint8_t{} casts from values that are already of the same
type.
|
|
fixes #24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.
|
|
interface, extend coverage
ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866 test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b556ec93c9b8f758783fda4d050da491 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836dab2018049390884220177c6db9b92 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c8784dfc8db80a21ab6508f7c99298255 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb82493f7a14580335ce719d5be81c8713e test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b29232057600df5ddd8351888253b95 test: Remove tests for internal helper functions (Martin Zumsande)
0538520091bf2982a029a0298835400f5afbdc15 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bbf61bb558cf7e18f7aff99b19f68508 test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2363822c3a1cfafda8ffc9528905058 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59a325ca6cb140757067dd5e0c7c249b test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f760211df314d650999e0a76edb0151b4fe1 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)
Pull request description:
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.
This includes the following steps:
* Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
* Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
* Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
* Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.
All in all, this PR increases the unit test coverage of AddrMan by a bit.
ACKs for top commit:
jnewbery:
ACK ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
josibake:
reACK https://github.com/bitcoin/bitcoin/pull/23826/commits/ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
|
|
If AddrMan::Good is unable to add an entry
to tried (for a number of reasons), return false.
This makes it much easier and cleaner to directly
test for tried collisions. It also allows anyone
calling Good() to handle the case where adding an
address to tried is unsuccessful.
Update docs to doxygen style.
|
|
in milliseconds
22b44fc696dc1078c40d17e2d497c74c7b4ae750 p2p: improve checkaddrman logging with duration in milliseconds (Jon Atack)
ec65bed00ee2e403e39b3c5977caf4abd31ccc87 log, timer: add LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE macro (Jon Atack)
325da75a5396f3161a6eade74b349105ed5722ab log, timer: allow not repeating log message on completion (Jon Atack)
Pull request description:
This patch:
- updates the `logging/timer.h::Timer` class to allow not repeating the log message on completion
- adds a `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro that prints the descriptive message when logging the start but not when logging the completion
- updates the checkaddrman logging to log the duration, and renames the function like the `-checkaddrman` configuration option in order to prefix every log message with `CheckAddrman` instead of the longer, less pleasant, and different-from-checkaddrman `ForceCheckAddrman` (the Doxygen documentation on the function already makes clear that it is unaffected by `m_consistency_check_ratio`).
before
```
2021-09-21T18:42:50Z [opencon] Addrman checks started: new 64864, tried 1690, total 66554
2021-09-21T18:42:50Z [opencon] Addrman checks completed successfully
```
after
```
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
```
To test, build and run bitcoind with `-debug=addrman -checkaddrman=<n>` for a value of `n` in the range of, say, 10 to 40.
ACKs for top commit:
laanwj:
Code review ACK 22b44fc696dc1078c40d17e2d497c74c7b4ae750
Tree-SHA512: 658c0dfaaa9d07092e2418f2d05007c58cc35be6593f22b3c592ce793334a885dd92dacc46bdeddc9d37939cf11174660a094c07c0fa117fbb282953aa45a94d
|
|
and logging
61ec0539b26a902a41a2602187a71f9dba3c6935 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7bfcb9ab0c5710a93112f7f341e753c9 [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68460272deefb3fcc653b03f6ec6e7cf [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833d5737900fe3c4369e26f2a08166892 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)
Pull request description:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
ACKs for top commit:
naumenkogs:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
mzumsande:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
shaavan:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
|
|
Keep the internal {Function}_() functions grouped together.
Review with `git diff --color-moved=dimmed-zebra`
|
|
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
|
|
|
|
92617b7a758c0425330fba4b886296730567927c effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.
Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
|
|
92617b7a758c0425330fba4b886296730567927c Make AddrMan support multiple ports per IP (Pieter Wuille)
Pull request description:
For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.
The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in https://github.com/bitcoin/bitcoin/issues/5150#issuecomment-853888909 e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).
And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.
One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based.
This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
ACKs for top commit:
jnewbery:
Code review ACK 92617b7a758c0425330fba4b886296730567927c
naumenkogs:
ACK 92617b7a758c0425330fba4b886296730567927c
ajtowns:
ACK 92617b7a758c0425330fba4b886296730567927c
vasild:
ACK 92617b7a758c0425330fba4b886296730567927c
Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f
|
|
|
|
positions, first
632aad9e6d8369750f4327a886ca5b3d3fed89bd Make CAddrman::Select_ select buckets, not positions, first (Pieter Wuille)
Pull request description:
The original CAddrMan behaviour (before #5941) was to pick a uniformly random non-empty bucket, and then pick a random element from that bucket. That commit, which introduced deterministic placement of entries in buckets, changed this to picking a uniformly random non-empty bucket position instead.
I believe that was a mistake. Buckets are our best metric for spreading out requests across independently-controlled nodes. That
does mean that if a bucket has fewer entries, its entries are supposed to be picked more frequently.
This PR reverts to the original high-level behavior, but on top of the deterministic placement logic.
ACKs for top commit:
jnewbery:
utACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
naumenkogs:
ACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
mzumsande:
ACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
Tree-SHA512: 60768afba2b6f0abd0dddff04381cab5acf374df48fc0e883ee16dde7cf7fd33056a04b573cff24a1b4d8e2a645bf0f0b3689eec84da4ff330e7b59ef142eca1
|
