Generalize configuration-related XDR. #56
Conversation
dmkozh
force-pushed
the
config_xdr_upd
branch
from
f42cd76
to
371bef6
Compare
The changes here are summarized in proposed updated to CAP-47 (stellar/stellar-protocol#1291). - Use unique keys to identify settings - Only distribute a hash for config upgrades in `StellarValue` - Add SCP messages for exchanging config upgrade sets
dmkozh
force-pushed
the
config_xdr_upd
branch
from
371bef6
to
1e5a8cf
Compare
| @@ -263,6 +267,11 @@ case SURVEY_REQUEST: | |||
| case SURVEY_RESPONSE: | |||
| SignedSurveyResponseMessage signedSurveyResponseMessage; | |||
|
|
|||
| case GET_CONFIG_UPGRADE_SET: | |||
| uint256 configUgradeSetHash; | |||
There was a problem hiding this comment.
Nit: typo configUgradeSetHash -> configUpgradeSetHash
| FLOOD_DEMAND = 19, | ||
|
|
||
| // Configuration upgrades | ||
| GET_CONFIG_UPGRADE_SET = 20, |
There was a problem hiding this comment.
I have a high level question about the CAP. Would it be possible to force validators to have the ConfigUpgradeSets locally, and avoid having to make these overlay changes? Validators would call the upgrade command with the full xdr, save it, but then vote on the hash. I'm still working out if this can be used maliciously.
There was a problem hiding this comment.
That's an interesting idea. My only concern is that if we ever allow fuzzy upgrade matching, then this won't work. E.g. if a validator wants to use its own calibration data and computes a coefficient a 1000, while another one computers a coefficient of 1001. I don't know if that's ever a good idea though.
There was a problem hiding this comment.
Yeah I don't see how adding this fuzzy matching complexity is a good idea. Is that flexibility helpful? Which setting would be applied to the network (1000 vs 1001 in your example)?. Upgrade values are meant to be determined offline. Once validator operators agree on what the value should be, they submit the settings to the network.
There was a problem hiding this comment.
The consensus could be done on e.g. the smallest hash. The reason would be so that validators could run the calibration themselves. Otherwise there needs to be some centralized distribution of the calibration data. That's probably fine in practice, but I'm not sure if we should eliminate this possibility. Maybe it's ok to leave it out at least for v1...
There was a problem hiding this comment.
Is there a CAP where I can read about this "calibration"? I don't think I have a good understanding of what you're referring to, and adding support for this in the overlay and consensus seems like overkill for what you get.
| @@ -520,20 +520,13 @@ case CONFIG_SETTING_TYPE_UINT32: | |||
|
|
|||
There was a problem hiding this comment.
I think ConfigSettingType and ConfigSetting can be removed as they are not being used any more.
There was a problem hiding this comment.
Not sure I understand; why are these not being used?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm pretty sure they're deleted, maybe you are looking at some outdated version of the file (FWIW I see your comment attached to the initial version and not the changed one)
|
Relevant work from this PR was merged in #75. |
The changes here reflect the recent updates to CAP-0046-09:
StellarValue