Tune relaying of untrusted proposals & validations: #3391
Conversation
The sfLedgerSequence field is designated as optional in the object template but it is effectively required and validations which do not include it were, correctly, rejected. This commit migrates the check outside of the peer code and into the constructor used for validations being deserialized for the network. Furthermore, the code will generate an error if a validation that is generated by a server does not include the field.
In deciding whether to relay a proposal or validation, a server would consider whether it was issued by a validator on that server's UNL. While both trusted proposals and validations were always relayed, the code prioritized relaying of untrusted proposals over untrusted validations. While not technically incorrect, validations are generally more "valuable" because they are required during the consensus process, whereas proposals are not, strictly, required. The commit introduces two new configuration options, allowing server operators to fine-tune the relaying behavior: The `[relay_proposals]` option controls the relaying behavior for proposals received by this server. It has two settings: "trusted" and "all" and the default is "trusted". The `[relay_validations]` options controls the relaying behavior for validations received by this server. It has two settings: "trusted" and "all" and the default is "all". This change does not require an amendment as it does not affect transaction processing.
|
Proposals are necessary to be able to even do validations though, right? I would prefer if both untrusted proposals and validations are by default relayed (as it is already the case). If server operators want to limit this for some reason, they now have the option to do so. Otherwise this creates incentives to collaborate more than maybe healthy or necessary with other node operators to even be able to reliably broadcast proposals. |
Even if you never see a single trusted proposal, as long as you see the validation you will continue. Of course, if nobody else sees a trusted proposal either, that's a problem.
This is not already the case: currently trusted proposals and validations are always relayed. Untrusted validations are, at present, never relayed and untrusted proposal are only if they're from a cluster peer or they're proposals on top of the last ledger we consider part of the consensus set. The not relaying untrusted validations but relaying some untrusted proposals is topsy-turvy. Again, validations are more important than proposals. With that said, I do see your point (even if I don't entirely agree with it) and we can certainly make the default for proposals be At some point, we ought to try and use a slot-based relay mechanism anyways. |
Codecov Report
@@ Coverage Diff @@
## develop #3391 +/- ##
===========================================
+ Coverage 70.44% 70.46% +0.01%
===========================================
Files 682 682
Lines 54392 54387 -5
===========================================
+ Hits 38315 38322 +7
+ Misses 16077 16065 -12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks great and easy to follow. I only have a couple minor comments.
sfLedgerSequence is still marked as soeOPTIONAL in STValidation::validationFormat, but is required now. The reason is we don't want to change the API of RPC etc, right? Maybe add a comment in STValidation::validationFormat to explain?
I don't think there's any reason to not change it to |
|
Right, changing from |
|
@pwang200: addressed the typo and marked the |
…:uint8_t. Thanks MSVC!
nbougalis
added
the
Passed
| @@ -136,6 +136,8 @@ class Config : public BasicConfig | |||
| std::size_t NETWORK_QUORUM = 1; | |||
|
|
|||
| // Peer networking parameters | |||
| bool RELAY_UNTRUSTED_VALIDATIONS = true; | |||
| bool RELAY_UNTRUSTED_PROPOSALS = false; | |||
There was a problem hiding this comment.
| bool RELAY_UNTRUSTED_PROPOSALS = false; | |
| bool RELAY_UNTRUSTED_PROPOSALS = true; |
In deciding whether to relay a proposal or validation, a server would consider whether it was issued by a validator on that server's UNL.
While both trusted proposals and validations were always relayed, the code prioritized relaying of untrusted proposals over untrusted validations. While not technically incorrect, validations are generally more "valuable" because they are required during the consensus process, whereas proposals are not, strictly, required.
The commit introduces two new configuration options, allowing server operators to fine-tune the relaying behavior:
The
[relay_proposals]option controls the relaying behavior for proposals received by this server. It has two settings: "trusted" and "all" and the default is "trusted".The
[relay_validations]options controls the relaying behavior for validations received by this server. It has two settings: "trusted" and "all" and the default is "all".This change does not require an amendment as it does not affect transaction processing.