feat: 13135 Added PlatformState protobuf representation #349
Conversation
imalygin
force-pushed
the
13135-add-platform-state
branch
2 times, most recently
from
145c149
to
75049c9
Compare
imalygin
force-pushed
the
13135-add-platform-state
branch
from
899555f
to
69ac2fc
Compare
imalygin
force-pushed
the
13135-add-platform-state
branch
2 times, most recently
from
f6f6194
to
8ead05f
Compare
imalygin
force-pushed
the
13135-add-platform-state
branch
5 times, most recently
from
39010c2
to
d208126
Compare
platform/state/platform_state.proto
Outdated
| /** | ||
| * A string network address.<br/> | ||
| * This value is the hostname assigned on the "internal" network | ||
| * interface behind any network firewalls, protocol gateways, or | ||
| * DNS translations. | ||
| * <p> | ||
| * This value SHALL be either a FQDN host name or an IPv4 address. | ||
| */ | ||
| string hostname_internal = 5; | ||
|
|
||
| /** | ||
| * Network port number.<br/> | ||
| * This value is the "translated" port number assigned on the "internal" | ||
| * network behind any network firewalls or other address translations. | ||
| * <p> | ||
| * This value SHALL be between 1 and 65535. | ||
| */ | ||
| uint32 port_internal = 6; |
There was a problem hiding this comment.
Before committing to supporting these fields forever, it might be worth having another discussion about this. I never quite wrapped my head around the truly irreplaceable need for these to be present in the address book.
There was a problem hiding this comment.
Who should I have this conversation with?
There was a problem hiding this comment.
I think devops are going to be the important people to talk to on this one. They should be knowledgeable about whether or not this is in any way relevant. @nathanklick should also probably be included in the conversation.
platform/state/platform_state.proto
Outdated
| /** | ||
| * A string that provides additional information about this consensus node. | ||
| */ | ||
| string memo = 11; |
There was a problem hiding this comment.
This also probably doesn't belong in the platform data structure in the long term. If we can't get rid of it in the short term, we should move it to a high index.
There was a problem hiding this comment.
The field is updated with a high index.
platform/state/platform_state.proto
Outdated
| /** | ||
| * A string that provides additional information about this consensus node. | ||
| */ | ||
| string memo = 11; |
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com> Co-authored-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
Updated the documentation according to the guidelines. Moved platform_state.proto to platform/state directory. Updated the documentation according to the guidelines. Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com> Co-authored-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
…laced it with `SemanticVersion` Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
imalygin
force-pushed
the
13135-add-platform-state
branch
from
d854ee2
to
8a331b6
Compare
platform/state/platform_state.proto
Outdated
| * This SHALL be the consensus timestamp of the first transaction in | ||
| * the current round. | ||
| */ | ||
| proto.Timestamp consensus_timestamp = 2; |
There was a problem hiding this comment.
This data is also duplicated inside of ConsensusSnapshot. Can it be removed?
There was a problem hiding this comment.
Yes, I can remove it if you can confirm that these two fields mean exactly the same thing in the context of persisted platform state
There was a problem hiding this comment.
@cody-littley @lpetrovic05 Could you weigh in here too?
There was a problem hiding this comment.
Same data in both places, there is only a single concept of the term "round" with respect to the data stored in the state. (Consensus overloads the term "round", but that data should never be leaving the consensus black box.)
There was a problem hiding this comment.
@imalygin The value in the platform state gets populated from the ConsensusSnapshot, so it must be removed from the platform state, not the snapshot
There was a problem hiding this comment.
@lpetrovic05 okay, sounds good
| message MinimumJudgeInfo { | ||
| /** | ||
| * A consensus round.<br/> | ||
| * The round this judge information applies to. |
There was a problem hiding this comment.
The round this judge information applies to.
I don't know how to interpret this. Does this mean the round here will match the round field in ConsensusSnapshot? Or something else?
There was a problem hiding this comment.
I think @cody-littley have the answer.
There was a problem hiding this comment.
We will have a list of these objects, with one object corresponding to a particular round.
Now that I think about it, I wonder if we actually need to encode the round number in this object. We could interpret the first element in this list as having the current round, the next as having round N-1, and so on. In fact, we may be able to get away with not having a type for this... we could simply use list of long values.
There was a problem hiding this comment.
Even though this is a reasonable idea, I suggest implementing it separately.
…d_mode` and updated its order. Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
…ancient_threshold`. Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
… from `ConsensusSnapshot`, as these fields duplicate fields in `PlatformState` and mean exactly the same thing. Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com> Co-authored-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
imalygin
force-pushed
the
13135-add-platform-state
branch
from
c959af0
to
f1f4dd1
Compare
…put it back to `ConsensusSnapshot` Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
…ormState` and put it back to `ConsensusSnapshot` Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
imalygin
force-pushed
the
13135-add-platform-state
branch
from
111bad5
to
b8e1110
Compare
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
| uint64 lowest_judge_generation_before_birth_round_mode = 10001 [deprecated = true]; | ||
|
|
||
| /** | ||
| * A consensus round.<br/> | ||
| * The last round before the birth round mode was enabled. | ||
| * Will be removed after the birth round migration. | ||
| * <p> | ||
| * This SHALL be `MAX_UNSIGNED` if birth round mode has not yet been enabled. | ||
| */ | ||
| uint64 last_round_before_birth_round_mode = 10002 [deprecated = true]; |
There was a problem hiding this comment.
I don't know if calling these fields deprecated is correct. We still very much need them up until we do the birth round migration. It's only after that migration that we should mark them as deprecated.
There was a problem hiding this comment.
I think Joseph @jsync-swirlds proposed to mark all the fields that we're intending to remove as deprecated. That is, if a field has index 10000+, then it should be marked as deprecated. I don't have a strong opinion on that. I will remove it if Joseph agrees with your comment.
There was a problem hiding this comment.
In protobuf, deprecated should not be a "this field is no longer used" indicator; protobuf would say to remove the field at that point and reserve the number. For a protobuf, deprecated should be "we are working to stop using this field and will remove it as soon as we can", which is what it sounds like is happening with respect to birth round migration.
| * This will be _removed_ and the field number reserved once the consensus | ||
| * event stream is retired. | ||
| */ | ||
| bytes legacy_running_event_hash = 10000 [deprecated = true]; |
There was a problem hiding this comment.
We want to delete this field, but we currently have a hard dependency on it. Should we mark it as deprecated if we are still using this field, or only after we are no longer reading/writing it?
There was a problem hiding this comment.
Typically deprecated is aded when a field is still in use, but the plan is to remove it. When the field is no longer in use the typical action would be to remove it and reserve the field number.
There was a problem hiding this comment.
The idea is that if we can foresee the deletion of this field, then we should mark it as deprecated. And we clearly can tell that we're going to remove it relatively soon.
| message MinimumJudgeInfo { | ||
| /** | ||
| * A consensus round.<br/> | ||
| * The round this judge information applies to. |
There was a problem hiding this comment.
We will have a list of these objects, with one object corresponding to a particular round.
Now that I think about it, I wonder if we actually need to encode the round number in this object. We could interpret the first element in this list as having the current round, the next as having round N-1, and so on. In fact, we may be able to get away with not having a type for this... we could simply use list of long values.
…h_round_mode` to the set of deprecated fields. Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Description:
This PR adds representation of
com.swirlds.platform.state.PlatformStateobject. It's a prerequisite for hashgraph/hedera-services#11771Related issue(s):
Fixes # hashgraph/hedera-services#13135
Related # hashgraph/hedera-services#11771