-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Try out CAP-0071 xdr with xdr ifdefs #5214
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
Open
sisuresh
wants to merge
5
commits into
stellar:master
Choose a base branch
from
sisuresh:CAP71-xdr
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+938
−104
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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'm not sure I understand why do we need detailed per-feature flags, or at least why are they opt-in.
I thought that we still will have a single next protocol flag that would enable the features that have been implemented. We may add flags that disable the features on-demand in case if that helps with downstream integration, though I would imagine it shouldn't matter most of the time.
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.
The idea was to let downstream systems enable exactly what they have support for. What your talking about is somewhat disruptive in that a core update would require downstream to pass in a new flag because it doesn't have support for that feature.
Uh oh!
There was an error while loading. Please reload this page.
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.
Downstream systems should generally not create custom core builds and I don't think they should normally pick the features from core - it's up to us to provide a proper build.
I thought the idea is that they can set the feature flags in XDR when necessary in order to avoid supporting stuff like new enum variants etc. while trying to implement some other feature. CAP-71 is actually a good example: building CAP-71 transactions requires rather involved downstream integration, but they can still use a Core build that accepts CAP-71 XDR even if they have none of the support. Since XDR is generally backwards-compatible, I think we should rarely need intermediate builds that disable features, which is why opt-out seems more reasonable than opt-in to me. The main way to break downstream with XDR is likely meta, but it probably could be guarded by a runtime config flag anyways.
Basically I'd like to keep the build configuration as simple as possible. There is seemingly no need to guard CAP-71 on the core side (and of course there shouldn't be a need to guard 'test feature'), so we shouldn't make the configs more complex because of these.
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.
One of the goals of this was to be able to release a subset of the protocol changes whenever we want. Lets say for some reason CAP-71 isn't implemented in core in time, or downstream hasn't adapted in time. We don't want to have to go in and remove or opt out of CAP-71 everywhere. Maybe we need to rethink this priority, but that's the point of this change. CAP-71 isn't as complex, but a more complex CAP that takes a while to implement might not be ready by our self-imposed deadline, which will then need to get pushed. Also re: config-complexity - these flags will be removed once a change is included into a protocol.
You do make an interesting point about changes that downstream should be able to just pass through like CAP-71 though. Maybe in this specific case the ifdefs make less sense.
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.
That's a pretty general goal that doesn't dictate the implementation. I don't think that encoding releases in build flags is a good idea (do you imagine making a release by just specifying the flags e.g. in Jenkins? that would be really brittle and error-prone). I think that should be driven by checked-in artifacts. Build flags should only be necessary for one-off/integration builds etc.
Well, that's what we can use a disable flag for. Though again, I would argue a better way would be to merge a respective change that explicitly disables it (e.g. in Config.h or wherever we find suitable). Basically we should have a single canonical definition that says 'p27 is features X,Y,Z'. Then if we do want to disable feature X, we change the definition to 'p27 is features Y,Z'. The question when do we promote feature to be canonical is debatable, but probably it's better to do sooner than later (to avoid integration issues), and then disable if necessary. Also, keep in mind that disabling a feature is supposed to be simple, but I'm sure that there may be more complex situations where disabling it would lead to test failures, which would require a test PR anyways. But I think that's good and still much better than running CI/tests on all the possible feature combinations, or ignoring the features until the very last moment.
Yes, disabling the feature for downstream integration is very different from disabling it from the release. The former should be rarely necessary at the core level, and as I've mentioned before we might be better off with runtime flags anyways (like meta). But for the release configuration we need something more stable than a build config flag.
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.
To be clear. a release should not enable any of these ifdefs. We should remove the ifdefs and feature flags for the CAPs being released, which should happen when we decide to cut the protocol. These flags should be used by downstream teams for integration.
Are you saying that a release build will contain these disable flags? That seems error prone, and I don't see how this is easier to automate integration tests with. A code change also isn't desirable because downstream will have to update and pick it up.
Our CI will continue running for vnext, which will include all features. We won't be using these individual flags in CI.
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.
To reiterate my point, I'm not convinced downstream teams should do custom core builds, and also shouldn't normally need to customize this at all. We can use these ourselves of course, but again, I'd prefer to avoid this as much as possible until actually necessary (i.e. when we have features that we're concerned about).
No, these are compile-time flags, aren't they? What I'm suggesting is to keep the single vnext config flag that we use for vnext builds, and add --disable-$feature flags for features that actually need it for on-demand builds (so that you have a non-prod vnext build with a feature disabled explicitly).
I would argue a code change is desirable and necessary if we want to change the protocol contents. Obviously downstream will have to pick it up (because the protocol has been changed), but that's find and by design, isn't it?
I think we agree on this point, my main suggestion is to not introduce the flags for the features that don't need to be disabled (and also to switch from opt-in to opt-out, though this is debatable and not as important).
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.
So all of this is an experiment that we can rollback if it doesn't yield what we're looking for. While CAP-0071 doesn't need to be behind a flag for downstream integration, it'd be helpful to see how this flag will flow downstream. Also, disable flags work against what we've been discussing wrt FSR.
It'd also be good to get @Shaptic's input on this, and I'd prefer not to stall on what we're experimenting with here unless there's a good reason.
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 thought the point of experiment is to figure out what works/doesn't work. I find the proposed approach concerning. Yes, we can merge this, but I don't see what exactly does this achieve or prove, given that it doesn't seem like the build granularity is not necessary now and I will repeat myself yet again, I believe it's conceptually problematic.
I don't really want to block this, but on the other hand I'm not sure I understand the intended design fully (like 'disable flags work against what we've been discussing wrt FSR' point), so maybe this is worth some kind of a process doc and a team-wide discussion.
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.
To be clear, I don't want to dismiss your feedback, but I'd like to get input from other before deviating from what we have here. We can wait for @Shaptic and @leighmcculloch to chime in before merging.