AMMBid: use tecINTERNAL for 'impossible' errors #4674
Merged
Conversation
This file contains 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
Upgrade log level to warning since it indicates some kind of (possibly network-wide) unexpected condition when these two errors occur.
|
It seems I don't know how to run clang-format. I tried, but it didn't work. ¯_(ツ)_/¯ |
gregtatcam
approved these changes
Aug 25, 2023
|
I don't have permission to push to your branch, but I applied the clang-format patch and pushed the commit here: intelliot@925552f Feel free to merge or cherry-pick it |
|
Just a quick comment before I dig into this, I think it would be more accurate to log those messages as "error". |
|
notes:
|
|
note: this is arguably an API change but it only affects the result code from |
|
note: no impact on Clio |
ximinez
requested changes
Aug 29, 2023
src/ripple/app/tx/impl/AMMBid.cpp
Outdated
Comment on lines
227
to
229
| JLOG(ctx_.journal.warn()) | ||
| << "AMM Bid: LP Token burn exceeds AMM balance " << burn << " " | ||
| << lptAMMBalance; |
There was a problem hiding this comment.
So far as I can tell, every tecINTERNAL result that logs, logs as error() or higher, other than a few in AMM code, which is new, and which I'd also consider an oversight.
These two log messages should be changed to error(), and I wouldn't mind seeing those others changed to error(), too.
|
notes:
|
|
As for changing to log levels to fatal in other cases where we return |
|
Yes, that should be a separate PR. No urgency, so I'm content to leave it as an open issue for now. Maybe a |
|
@ximinez - please review this as well |
|
This is a "tiny" PR and review by @seelabs would be welcome, but not required. |
ximinez
approved these changes
Sep 1, 2023
If those do turn up anything, feel free to open a new issue/PR. This one will be merged shortly... |
ckeshava
pushed a commit
to ckeshava/rippled
that referenced
this pull request
Sep 22, 2023
Modify two error cases in AMMBid transactor to return `tecINTERNAL` to more clearly indicate that these errors should not be possible unless operating in unforeseen circumstances. It likely indicates a bug. The log level has been updated to `fatal()` since it indicates a (potentially network-wide) unexpected condition when either of these errors occurs. Details: The two specific transaction error cases changed are: - `tecAMM_BALANCE` - In this case, this error (total LP Tokens outstanding is lower than the amount to be burned for the bid) is a subset of the case where the user doesn't have enough LP Tokens to pay for the bid. When this case is reached, the bidder's LP Tokens balance has already been checked first. The user's LP Tokens should always be a subset of total LP Tokens issued, so this should be impossible. - `tecINSUFFICIENT_PAYMENT` - In this case, the amount to be refunded as a result of the bid is greater than the price paid for the auction slot. This should never occur unless something is wrong with the math for calculating the refund amount. Both error cases in question are "defense in depth" measures meant to protect against making things worse if the code has already reached a state that is supposed to be impossible, likely due to a bug elsewhere. Such "shouldn't ever occur" checks should use an error code that categorically indicates a larger problem. This is similar to how `tecINVARIANT_FAILED` is a warning sign that something went wrong and likely could've been worse, but since there isn't an Invariant Check applying here, `tecINTERNAL` is the appropriate error code. This is "debatably" a transaction processing change since it could hypothetically change how transactions are processed if there's a bug we don't know about.
ckeshava
pushed a commit
to ckeshava/rippled
that referenced
this pull request
Sep 25, 2023
Modify two error cases in AMMBid transactor to return `tecINTERNAL` to more clearly indicate that these errors should not be possible unless operating in unforeseen circumstances. It likely indicates a bug. The log level has been updated to `fatal()` since it indicates a (potentially network-wide) unexpected condition when either of these errors occurs. Details: The two specific transaction error cases changed are: - `tecAMM_BALANCE` - In this case, this error (total LP Tokens outstanding is lower than the amount to be burned for the bid) is a subset of the case where the user doesn't have enough LP Tokens to pay for the bid. When this case is reached, the bidder's LP Tokens balance has already been checked first. The user's LP Tokens should always be a subset of total LP Tokens issued, so this should be impossible. - `tecINSUFFICIENT_PAYMENT` - In this case, the amount to be refunded as a result of the bid is greater than the price paid for the auction slot. This should never occur unless something is wrong with the math for calculating the refund amount. Both error cases in question are "defense in depth" measures meant to protect against making things worse if the code has already reached a state that is supposed to be impossible, likely due to a bug elsewhere. Such "shouldn't ever occur" checks should use an error code that categorically indicates a larger problem. This is similar to how `tecINVARIANT_FAILED` is a warning sign that something went wrong and likely could've been worse, but since there isn't an Invariant Check applying here, `tecINTERNAL` is the appropriate error code. This is "debatably" a transaction processing change since it could hypothetically change how transactions are processed if there's a bug we don't know about.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Modifies two error cases in AMMBid to return
tecINTERNALrather than another error code, to more clearly indicate that these types of errors should not be possible unless operating in some sort of unforeseen circumstances. Upgrade the related log message to warning level, since it likely indicates a bug.Caution: I have not actually built this branch myself. Reviewers, please confirm that the code compiles and passes tests.
High Level Overview of Change
Modifies two specific transaction error cases in the processing for the AMMBid transactor:
tecAMM_BALANCE- In this case, this error (total LP Tokens outstanding is lower than the amount to be burned for the bid) is a subset of the case where the user doesn't have enough LP Tokens to pay for the bid. When this case is reached, the bidder's LP Tokens balance has already been checked first. The user's LP Tokens should always be a subset of total LP Tokens issued, so this should be impossible.tecINSUFFICIENT_PAYMENT- In this case, the amount to be refunded as a result of the bid is greater than the price paid for the auction slot. This should never occur unless something is wrong with the math for calculating the refund amount.Context of Change
Both error cases in question are "defense in depth" measures meant to protect against making things worse if the code has already reached a state that is supposed to be impossible, likely due to a bug elsewhere. (For example, some sort of race condition where multiple threads are accessing the same ledger entries in parallel without proper locks, simple memory corruption, or some sort of math failure elsewhere—rounding errors, missing range checks, and so on.)
If these cases have "normal" error codes, that suggests that QA should be able to add test cases that intentionally trigger them, and if they occur then the code is functioning as intended. But, in reality, it should be impossible for an integration test to trigger this case, and if you can trigger these reliably it's indicative of a bug that should be fixed elsewhere. (It's possible that this sort of thing could occur randomly due to, say, cosmic rays flipping bits at random, in which case reproducing the error is likely to be, well, very difficult.)
Any "shouldn't ever occur" checks should use an error code that categorically indicates a larger problem. This is similar to how
tecINVARIANT_FAILEDis a warning sign that something went wrong and likely could've been worse, but since there isn't a separate-level Invariant Check applying here,tecINTERNALis the appropriate error code. The official description fortecINTERNALin the docs is:Type of Change
This is "debatably" a transaction processing change since it could hypothetically change how transactions are processed if there's a bug we don't know about, but as far as I know it doesn't actually change anything that can actually happen. Regardless, as long as the changes are incorporated into the AMM amendment before said amendment is part of any stable release, no additional amendment is necessary.