Improve manifest relaying: #3722
Conversation
|
|
||
| for (std::size_t i = 0; i < n; ++i) | ||
| { | ||
| auto& s = m->list().Get(i).stobject(); | ||
|
|
||
| if (auto mo = deserializeManifest(s)) | ||
| { | ||
| uint256 const hash = mo->hash(); | ||
| if (!hashRouter.addSuppressionPeer(hash, from->id())) |
There was a problem hiding this comment.
With the deletion of this check, we will be blindly forwarding the manifests even if its duplicates. Consider using addSuppressionPeerWithStatus with a timeout for dupes so that at least we don't spam the network?
There was a problem hiding this comment.
We will not: manifests are only forwarded if they are successfully applied—that is, if we haven't seen them before.
|
|
||
| for (std::size_t i = 0; i < n; ++i) | ||
| { | ||
| auto& s = m->list().Get(i).stobject(); | ||
|
|
||
| if (auto mo = deserializeManifest(s)) | ||
| { | ||
| uint256 const hash = mo->hash(); | ||
| if (!hashRouter.addSuppressionPeer(hash, from->id())) |
There was a problem hiding this comment.
OverlayImpl.cpp:661:11: warning: unused variable 'hashRouter' [-Wunused-variable]
auto& hashRouter = app_.getHashRouter();
^
There was a problem hiding this comment.
Looks good assuming the hashRouter warning pointed out by @HowardHinnant is fixed.
Codecov Report
@@ Coverage Diff @@
## develop #3722 +/- ##
===============================
===============================
Continue to review full report at Codecov.
|
The manifest relay code would only ever relay manifests from validators on a server's UNL which means that the manifests of validators that are not broadly trusted can fail to propagate across the network, which can make it difficult to detect and track such validators. This commit, if merged, propagates all manifests on a best-effort basis resulting in broader availability of manifests on the network and avoid the need to introduce on-ledger manifest storage or to establish one or more manifest repositories.
The existing code attempts to validate the provided node public key using a function that assumes that the encoded public key is for an account. This causes the parsing to fail. This commit fixes #3317 by letting the caller specify the type of the public key being checked.
- The changes to manifest relaying introduced with commit 940ea1b will cause newly accepted manifests to be sent back to the peer from which they were received. This no longer happens: a newly accepted manifest is never sent back to the peer we received it from. - When encountering a manifest without a domain set, the `manifest` and `validator_info` commands would include an empty string as the domain associated with the manifest. This no longer happens: if a domain is not present, the `domain` field will not be.
The manifest relay code would only ever relay manifests from validators on a server's UNL which means that the manifests of validators that are not broadly trusted can fail to propagate across the network, which can make it difficult to detect and track such validators.
This commit, if merged, propagates all manifests on a best-effort basis resulting in broader availability of manifests on the network and avoid the need to introduce on-ledger manifest storage or to establish one or more manifest repositories.