Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Testground tooling #2456
feat: Testground tooling #2456
Changes from 82 commits
a008844
6e7cc58
86d492b
25d60cd
f4c8718
47e1a80
a383494
39b306a
e855a67
e16be41
e8b4926
eda02ec
e2de741
83f0f76
db8d2f2
84f3980
96fb988
d39e44b
0ca3996
cf50bb4
d8e2fed
195383e
cc6950e
5949133
ef17302
42b578c
507059a
baf266e
6bc03cd
0b4bf7a
c0ab032
ce19605
71d6a8d
fa1952c
aadd52e
bc943c0
e4dbbf6
e1ccf1c
e397795
4ba33a6
112df3c
2a5185a
b7a8597
6a95382
b53e9b7
1000798
28d3953
9d2369c
9e4ceb7
b03b38e
cb5414f
0ff161f
d79f274
2022867
bb24860
de36333
41b960c
06309a7
796eb34
1cb7536
e608781
6305141
4a39b27
db0fdfd
3a5fcdd
5e3a71e
1fce736
c4a0261
64641d9
5fb00a8
22e7107
c680cf1
3cf0697
261214a
4fc5f8f
3104b5c
1c6c20a
07b0a18
651b85c
6d02f50
d33b898
b10c6e5
1b7ab87
d698845
93cb5d4
39b36da
3d69fc0
50bc61e
ecbfb5e
3b8d83f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Version could use a GoDoc that explains why this number looks crazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved in 93cb5d4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] related to https://github.com/celestiaorg/celestia-app/pull/2456/files#r1417502424
why define this constant if it is never used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved in 93cb5d4
we should probably do the same for v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Why doesn't this have case statements for each version?
even if v1 and v2 have the same constants, this seems more readable and future proof in case we update the v2 constant prior to cutting the v2 release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually disagree with this, as with each version, we'd have to go change each if statement across the entire codebase. besides being a bit annoying, there's no way to tell if we missed one or are being incosistent places. imo, just saying default reads as all versions use the same constant, which feels more concise then having to read each version and compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this thread shouldn't block this PR and instead we should create a new issue for this.
Go doesn't have exhaustive checking (like Rust) but I don't understand why silently defaulting to an old version is preferable over a
panic
. I see the options as:Current
Proposal
ref: https://rustc-dev-guide.rust-lang.org/pat-exhaustive-checking.html#pattern-and-exhaustiveness-checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a thread of two discussing this in the version PRs, so apologies if you are already aware of this point, but essentially were relying on there not being any human error to avoid a network halting bug. There's also not really any benefit, since the appversion MUST be checked when verifying the header, both by Comet and the light client.
If the appversion is not expected or the node/LC doesn't know how to handle that version, it must halt. This means that there are no scenarios where the panic would serve a positive purpose. The only time that it would get hit is when we accidentally forget to update one of the ever increasing if statements, in which case the entire network halts for no reason, even if they upgraded.
imo we definitely should not introduce a panic. I'm also of the opinion that specifying only the changes for a constant is actually more readable. If we specify each value for each version and we want to find when something changed, we have to look up each constant and manually compare, which feels weird to me. That's just a readability opinion, and won't halt the network, so I don't hold that strong of a view on that.
I think we've casually discussed this synchronously, but since this has come up multiple times, I'm happy to schedule a sync or open a discussion specifically for it if need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] This Go code may become stale with respect to the
Role
interface defined intest/testground/network.go
. We may remove the code and replace with a permalink.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved in 93cb5d4