feat!: version the application constants #1768
Conversation
| @@ -49,7 +49,7 @@ func TestNewDataAvailabilityHeader(t *testing.T) { | |||
| { | |||
| name: "max square size", | |||
| expectedHash: []byte{0xce, 0x5c, 0xf3, 0xc9, 0x15, 0xeb, 0xbf, 0xb0, 0x67, 0xe1, 0xa5, 0x97, 0x35, 0xf3, 0x25, 0x7b, 0x1c, 0x47, 0x74, 0x1f, 0xec, 0x6a, 0x33, 0x19, 0x7f, 0x8f, 0xc2, 0x4a, 0xe, 0xe2, 0xbe, 0x73}, | |||
| squareSize: appconsts.DefaultMaxSquareSize, | |||
| squareSize: uint64(appconsts.DefaultMaxSquareSize), | |||
There was a problem hiding this comment.
What do you think about defaulting to unit64 as the type for any square size type/variable?
There was a problem hiding this comment.
lol I went back and forth a few times between uint64 and int. On one hand uint64 is the type that protobuf encodes this variable; on the other hand this is used a lot with len(...) i.e. len(shares) == squareSize * squareSize so it has value as an int. I'm fine with either. In either case there will be conversion and there would have to be some other bug for it to overflow in conversion
There was a problem hiding this comment.
I'm fine with either. In either case there will be conversion and there would have to be some other bug for it to overflow in conversion
I see. The point is to make sure that the data type remains consistent throughout the code (but when sending the data on the wire, as you said, the conversion would be inevitable), I have no strong preference for int or uint64 as well, totally up to you.
Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Rootul P <rootulp@gmail.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## main #1768 +/- ##
==========================================
- Coverage 21.90% 21.76% -0.14%
==========================================
Files 116 116
Lines 13208 13236 +28
==========================================
- Hits 2893 2881 -12
- Misses 10024 10063 +39
- Partials 291 292 +1
|
| if govMaxSquareSize > appconsts.MaxSquareSize { | ||
| return fmt.Errorf( | ||
| "gov max square size cannot exceed the max square size: max %d", | ||
| appconsts.MaxSquareSize, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
[question] out of curiosity why is this part of validation no longer necessary?
There was a problem hiding this comment.
This was discussed out of band. The crux is that validation of params does not accept any arguments yet this capped value depends on the version of the network, thus we can no longer correctly validate it here and need to do so elsewhere
Co-authored-by: Rootul P <rootulp@gmail.com>
| @@ -282,6 +282,9 @@ func NewAppServer(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts se | |||
| baseapp.SetTrace(cast.ToBool(appOpts.Get(server.FlagTrace))), | |||
| baseapp.SetIndexEvents(cast.ToStringSlice(appOpts.Get(server.FlagIndexEvents))), | |||
| baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(cast.ToUint64(appOpts.Get(server.FlagStateSyncSnapshotInterval)), cast.ToUint32(appOpts.Get(server.FlagStateSyncSnapshotKeepRecent)))), | |||
| func(b *baseapp.BaseApp) { | |||
| b.SetProtocolVersion(appconsts.LatestVersion) | |||
There was a problem hiding this comment.
[blocking question]
what happens if we're syncing from a previous block that is not at the latest version?
There was a problem hiding this comment.
That's okay. The ProtocolVersion that gets set can be interpreted as the highest version the binary can execute on. All prior versions should be handled
There was a problem hiding this comment.
@cmwaters answered my question during a sync, updating the version here doesn't have an effect on past headers, so syncing from a past block will work as expected.
one additional [non blocking] question, will this have an effect for single binary upgrades? like, if a validator upgrades early, sets the protocol version to the latest here when does that value get conveyed back to tendermint and included in the consensus logic?
There was a problem hiding this comment.
ahh I need hit refresh more often, wrote my comment w/o the context of the second one.
I'll look into the exact mechanism more
There was a problem hiding this comment.
Yeah I need to look into this more. There's definitely an incongruence in the way we want to do things and the way that the SDK is set up to do things. I definitely think there needs to be more thinking in how we bridge the gap
There was a problem hiding this comment.
The app version is used in the Info handshake to ensure that the app has the same version as the last block processed by Comet. To ensure we always give the correct version to Comet, the app needs to know the last version it saw in a block. In the case this is the first height, it should actually be the first version i.e. 1. We should even have it hardcoded such that users can't start with a non 1 version.
There was a problem hiding this comment.
I'm going to merge this PR for now as upgrades is more of a different issue that we need to address
There was a problem hiding this comment.
also it looks like we are using the initialization logic from the cmd pkg in the testnode, which is what I missed before with #1768 (review)
celestia-app/test/util/testnode/full_node.go
Line 104 in e730980
LGTM! 👍 👍
Closes: #1625 --------- Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Rootul P <rootulp@gmail.com>
Closes: #1625 --------- Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Rootul P <rootulp@gmail.com>
Closes: #1625