Skip to content
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

ensure that operation level meta is cleared during an internal error #2383

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

MonsieurNicolas
Copy link
Contributor

This PR fixes an issue in the meta generation when encountering txINTERNAL_ERROR:
for a transaction with more than one operation, the code was keeping meta for operations that succeeded before the operation that triggered the internal error, even though the actual changes were never committed.

Thanks @bartekn for the report

@jonjove
Copy link
Contributor

jonjove commented Jan 13, 2020

While this change looks correct, I don't think it is the best solution. I think it would be better to no-throw swap the meta into place as the last step of the function for two reasons:

  1. It's the idiomatic way to write exception-safe code
  2. Makes it extremely clear that all code paths must reach that point in order to change the meta

As an example of why my proposed approach would be better, consider the following segment of TransactionFrame::applyOperations
https://github.com/stellar/stellar-core/blob/c5ca2a3167fb2b8c64310587ffad0899adddf83a/src/transactions/TransactionFrame.cpp#L634-L640
The implementation says it should never happen, but we have code there to handle it. Did it happen? I have no idea, but if it did your change would still permit the wrong meta to be generated. My proposed change would generate no meta in this case, as expected. A future maintainer could similarly add other fast-fail paths, not recognizing that it would lead to incorrect meta generation.

@MonsieurNicolas
Copy link
Contributor Author

Yeah I was going for the minimal change but your suggestion cleans up the code a little bit, thanks for the suggestion @jonjove

@jonjove
Copy link
Contributor

jonjove commented Jan 13, 2020

r+ affb766

latobarita added a commit that referenced this pull request Jan 13, 2020
ensure that operation level meta is cleared during an internal error

Reviewed-by: jonjove
@latobarita latobarita merged commit affb766 into stellar:master Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants