chore: Rename ModStateProofNextRound to StateProofNext. #5265
Conversation
@algorandskiy What are the semantics? |
|
|
|
@algorandskiy changed this to be a codec tag. That should be sufficient to fix backwards compatibility with the SDK. |
|
I needed to add |
winder
requested review from
Eric-Warehime
and removed request for
jannotti and
id-ms
ledger/ledgercore/statedelta.go
Outdated
| @@ -113,7 +115,7 @@ type StateDelta struct { | |||
| // ModStateProofNextRound represents modification on StateProofNextRound field in the block header. If the block contains | |||
| // a valid state proof transaction, this field will contain the next round for state proof. | |||
| // otherwise it will be set to 0. | |||
| ModStateProofNextRound basics.Round | |||
| ModStateProofNextRound basics.Round `codec:"StateProofNextRound,omitempty"` | |||
There was a problem hiding this comment.
I think it is better to add a comment that explains why ModStateProofNextRound is the only field that has codec tag and why the codec id is different.
There was a problem hiding this comment.
We're working on some CI automation that would warn when these types diverge from the ones in the sdk.
Hopefully that will serve better than comments like this.
There was a problem hiding this comment.
are we renaming this field in the sdk, too? it's currently StateProofNext.
https://github.com/algorand/go-algorand-sdk/blob/develop/types/statedelta.go#L317
There was a problem hiding this comment.
I think that's why it was renamed here -- easy to mistake the block header field and the ledgercore state delta field. This codec change matches the current SDK.
There was a problem hiding this comment.
@shiqizng thanks, you're right:
| ModStateProofNextRound basics.Round `codec:"StateProofNextRound,omitempty"` | |
| ModStateProofNextRound basics.Round `codec:"StateProofNext,omitempty"` |
ledger/ledgercore/statedelta.go
Outdated
| @@ -88,6 +88,8 @@ type KvValueDelta struct { | |||
| // If adding a new field not explicitly allocated by PopulateStateDelta, make sure to reset | |||
| // it in .ReuseStateDelta to avoid dirty memory errors. | |||
| // If adding fields make sure to add them to the .Reset() method to avoid dirty state | |||
| // | |||
| //msgp:ignore StateDelta | |||
There was a problem hiding this comment.
This will prevent us from ever msgp-encoding this data structure right? Not necessarily a bad thing, just want to be clear.
There was a problem hiding this comment.
I believe this is only for the pre-generated Marshal/Unmarshal functions. We're still able to use the reflection based msgp encoding functions.
There was a problem hiding this comment.
Right, the codegen which might be used when sending these messages over the network for example.
There was a problem hiding this comment.
I added this in because adding the codec tag caused that codegen to start triggering for this struct. There were a bunch of errors with it because we don't set allocbounds in any of the slices.
ledger/ledgercore/statedelta.go
Outdated
| @@ -113,7 +115,7 @@ type StateDelta struct { | |||
| // ModStateProofNextRound represents modification on StateProofNextRound field in the block header. If the block contains | |||
| // a valid state proof transaction, this field will contain the next round for state proof. | |||
| // otherwise it will be set to 0. | |||
| ModStateProofNextRound basics.Round | |||
| ModStateProofNextRound basics.Round `codec:"StateProofNext,omitempty"` | |||
There was a problem hiding this comment.
since the consensus name is StateProofNext for API, maybe we rename the field to StateProofNext as well?
It will be different from StateProofNextRound that is used in block headers (matching name here and there was a confusion point and the trigger for the original renaming)
winder
changed the title
Summary
Remove
Modprefix becauseModStateProofNextRoundis an absolute round rather than some sort of mod/delta/offset.Test Plan
Non functional change, existing regression tests.