Skip to content

addrman: Fix new table bucketing during unserialization #20557

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

Merged
merged 7 commits into from
Feb 9, 2021
Merged

addrman: Fix new table bucketing during unserialization #20557

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b4c5fda
[addrman] Fix new table bucketing during unserialization
jnewbery Dec 3, 2020
009b8e0
[addrman] Improve variable naming/code style of touched code.
jnewbery Dec 3, 2020
8062d92
[addrman] Rename asmap version to asmap checksum
jnewbery Dec 3, 2020
a5c9b04
[addrman] Don't rebucket new table entries unnecessarily
jnewbery Dec 3, 2020
ac3547e
[addrman] Improve variable naming/code style of touched code.
jnewbery Dec 3, 2020
4362923
[addrman] Improve serialization comments
jnewbery Jan 29, 2021
4676a4f
[addrman] Don't repeat "Bucketing method was updated" log multiple times
jnewbery Jan 29, 2021
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
94 changes: 54 additions & 40 deletions src/addrman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,22 +335,20 @@ friend class CAddrManTest;
* * nNew
* * nTried
* * number of "new" buckets XOR 2**30
* * all nNew addrinfos in vvNew
* * all nTried addrinfos in vvTried
* * for each bucket:
* * all new addresses (total count: nNew)
* * all tried addresses (total count: nTried)
* * for each new bucket:
* * number of elements
* * for each element: index
* * for each element: index in the serialized "all new addresses"
* * asmap checksum
*
* 2**30 is xorred with the number of buckets to make addrman deserializer v0 detect it
* as incompatible. This is necessary because it did not check the version number on
* deserialization.
*
* Notice that vvTried, mapAddr and vVector are never encoded explicitly;
* vvNew, vvTried, mapInfo, mapAddr and vRandom are never encoded explicitly;
* they are instead reconstructed from the other information.
*
* vvNew is serialized, but only used if ADDRMAN_UNKNOWN_BUCKET_COUNT didn't change,
* otherwise it is reconstructed as well.
*
* This format is more complex, but significantly smaller (at most 1.5 MiB), and supports
* changes to the ADDRMAN_ parameters without breaking the on-disk structure.
*
Expand Down Expand Up @@ -413,13 +411,13 @@ friend class CAddrManTest;
}
}
}
// Store asmap version after bucket entries so that it
// Store asmap checksum after bucket entries so that it
// can be ignored by older clients for backward compatibility.
uint256 asmap_version;
uint256 asmap_checksum;
if (m_asmap.size() != 0) {
asmap_version = SerializeHash(m_asmap);
asmap_checksum = SerializeHash(m_asmap);
}
s << asmap_version;
s << asmap_checksum;
}

template <typename Stream>
Expand Down Expand Up @@ -500,47 +498,63 @@ friend class CAddrManTest;
nTried -= nLost;

// Store positions in the new table buckets to apply later (if possible).
std::map<int, int> entryToBucket; // Represents which entry belonged to which bucket when serializing

for (int bucket = 0; bucket < nUBuckets; bucket++) {
int nSize = 0;
s >> nSize;
for (int n = 0; n < nSize; n++) {
int nIndex = 0;
s >> nIndex;
if (nIndex >= 0 && nIndex < nNew) {
entryToBucket[nIndex] = bucket;
// An entry may appear in up to ADDRMAN_NEW_BUCKETS_PER_ADDRESS buckets,
// so we store all bucket-entry_index pairs to iterate through later.
std::vector<std::pair<int, int>> bucket_entries;
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "[addrman] Fix new table bucketing during unserialization" (b4c5fda)

Seems like having a data structure here is superfluous if it's just going to be filled up then immediately iterated over and discarded. Maybe it would make sense to remove this vector later.

Copy link
Contributor

@vasild vasild Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that vector could be avoided only if we could seek into the stream. Then we could change the current:

read new buckets
read asmap checksum
process the new buckets that were read, depending on the asmap checksum

to

seek forward and read the asmap checksum (last 32 bytes)
seek back and read+process the new bucketing data, depending on the asmap checksum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only know how we're going to use the vector after we've deserialized the asmap_checksum field, so I don't think we can get rid of it.


for (int bucket = 0; bucket < nUBuckets; ++bucket) {
int num_entries{0};
s >> num_entries;
for (int n = 0; n < num_entries; ++n) {
int entry_index{0};
s >> entry_index;
if (entry_index >= 0 && entry_index < nNew) {
bucket_entries.emplace_back(bucket, entry_index);
}
}
}

uint256 supplied_asmap_version;
// If the bucket count and asmap checksum haven't changed, then attempt
// to restore the entries to the buckets/positions they were in before
// serialization.
uint256 supplied_asmap_checksum;
if (m_asmap.size() != 0) {
supplied_asmap_version = SerializeHash(m_asmap);
supplied_asmap_checksum = SerializeHash(m_asmap);
}
uint256 serialized_asmap_version;
uint256 serialized_asmap_checksum;
if (format >= Format::V2_ASMAP) {
s >> serialized_asmap_version;
s >> serialized_asmap_checksum;
}
const bool restore_bucketing{nUBuckets == ADDRMAN_NEW_BUCKET_COUNT &&
serialized_asmap_checksum == supplied_asmap_checksum};

for (int n = 0; n < nNew; n++) {
CAddrInfo &info = mapInfo[n];
int bucket = entryToBucket[n];
int nUBucketPos = info.GetBucketPosition(nKey, true, bucket);
if (format >= Format::V2_ASMAP && nUBuckets == ADDRMAN_NEW_BUCKET_COUNT && vvNew[bucket][nUBucketPos] == -1 &&
info.nRefCount < ADDRMAN_NEW_BUCKETS_PER_ADDRESS && serialized_asmap_version == supplied_asmap_version) {
if (!restore_bucketing) {
LogPrint(BCLog::ADDRMAN, "Bucketing method was updated, re-bucketing addrman entries from disk\n");
}

for (auto bucket_entry : bucket_entries) {
int bucket{bucket_entry.first};
const int entry_index{bucket_entry.second};
CAddrInfo& info = mapInfo[entry_index];

// The entry shouldn't appear in more than
// ADDRMAN_NEW_BUCKETS_PER_ADDRESS. If it has already, just skip
// this bucket_entry.
if (info.nRefCount >= ADDRMAN_NEW_BUCKETS_PER_ADDRESS) continue;

int bucket_position = info.GetBucketPosition(nKey, true, bucket);
if (restore_bucketing && vvNew[bucket][bucket_position] == -1) {
// Bucketing has not changed, using existing bucket positions for the new table
vvNew[bucket][nUBucketPos] = n;
info.nRefCount++;
vvNew[bucket][bucket_position] = entry_index;
++info.nRefCount;
} else {
// In case the new table data cannot be used (format unknown, bucket count wrong or new asmap),
// In case the new table data cannot be used (bucket count wrong or new asmap),
// try to give them a reference based on their primary source address.
LogPrint(BCLog::ADDRMAN, "Bucketing method was updated, re-bucketing addrman entries from disk\n");
bucket = info.GetNewBucket(nKey, m_asmap);
nUBucketPos = info.GetBucketPosition(nKey, true, bucket);
if (vvNew[bucket][nUBucketPos] == -1) {
vvNew[bucket][nUBucketPos] = n;
info.nRefCount++;
bucket_position = info.GetBucketPosition(nKey, true, bucket);
if (vvNew[bucket][bucket_position] == -1) {
vvNew[bucket][bucket_position] = entry_index;
++info.nRefCount;
}
}
}
Expand Down