Catch transaction deserialization error in doLedgerGrpc

[cjcobb23]

• Allow clio to extract ledgers with transactions that can no longer be deserialized. The problem transactions will be skipped.

Some old transactions cannot be deserialized by current rippled code. Clio can still extract the ledger headers and state objects, just not the transactions.

[mDuo13]

Does this error only apply to gRPC deserialization, or do these transactions also fail to deserialize to JSON in the rippled APIs directly? Do you have an example tx hash to look up? (If there's no way to deserialize these transactions from their binary form, that's very undesirable; I don't like having "black box" transactions in history; the 32570 missing ledgers are bad enough.)

Note, I am not saying this PR should fix the problem of deserializing things, but the answers to my question will inform whether or not we open a ticket to fix the issue more broadly, and the scope thereof.

[cjcobb23]

Here's one:

hash: "C7CE7D48C50D6ABEBC8DA01447C8BDA21324094B58C0738AED30D1FCD399FAD6"
ledger sequence: 7266393
actual transaction:
            {
               "Account" : "rEX2Q18vQziVAJoiLwoVHN5u87CXm29LCQ",
               "Fee" : "10",
               "Flags" : 2147483648,
               "Memos" : [
                  {
                     "TransactionMetaData" : {
                        "TransactionMetaData" : {
                           "TransactionMetaData" : {
                              "TransactionMetaData" : {
                                 "TransactionMetaData" : {
                                    "TransactionMetaData" : {
                                       "TransactionMetaData" : {
                                          "TransactionMetaData" : {
                                             "TransactionMetaData" : {
                                                "TransactionMetaData" : {
                                                   "TransactionMetaData" : {
                                                      "TransactionMetaData" : {
                                                         "TransactionMetaData" : {
                                                            "TransactionMetaData" : {
                                                               "TransactionMetaData" : {}
                                                            }
                                                         }
                                                      }
                                                   }
                                                }
                                             }
                                          }
                                       }
                                    }
                                 }
                              }
                           }
                        }
                     }
                  }
               ],
               "Sequence" : 51,
               "SigningPubKey" : "033D098A1F0664D894D0D542006541CDDC997E1B5A5F1A6E63BE5B83E12AAF00D2",
               "TransactionType" : "AccountSet",
               "TxnSignature" : "304402204DF684C465B2D110F9DF39100E205084C972FEF885AF2A9036DBB58866E224AE022055C4DF3345F8111552C4D01B896D3E22CBFAD450EF020737DD9F2C3907294D3E",
               "hash" : "C7CE7D48C50D6ABEBC8DA01447C8BDA21324094B58C0738AED30D1FCD399FAD6",
               "metaData" : {
                  "AffectedNodes" : [
                     {
                        "ModifiedNode" : {
                           "FinalFields" : {
                              "Account" : "rEX2Q18vQziVAJoiLwoVHN5u87CXm29LCQ",
                              "Balance" : "32005425",
                              "Flags" : 0,
                              "OwnerCount" : 2,
                              "Sequence" : 52
                           },
                           "LedgerEntryType" : "AccountRoot",
                           "LedgerIndex" : "BCCEA6B346A29656AFD37B8AB7F573DA705AB7A86D9FE197AC6CB1E9D5772EDB",
                           "PreviousFields" : {
                              "Balance" : "32005435",
                              "Sequence" : 51
                           },
                           "PreviousTxnID" : "82FC7296AE62216C6A58DFF6707CD441C57ECF835DD784449C8C0CA78A9ED143",
                           "PreviousTxnLgrSeq" : 7266389
                        }
                     }
                  ],
                  "TransactionIndex" : 6,
                  "TransactionResult" : "tesSUCCESS"
               }
            },

I had to modify rippled to actually print this transaction out. Right now rippled will not return this txn.

[cjcobb23]

The error applies to both gRPC deserialization as well as JSON deserialization. The error actually just lies at transaction deserialization itself, where rippled cannot properly deserialize the transaction at all anymore.

[scottschurr]

Okay. I now understand the previous comments. The identified problem applies both to clio and, more generally, to any time rippled tries to deserialize these transactions. The supplied fix only works for clio. I'm good with that, but that understanding gives me context for what I'm reviewing.

[scottschurr]

I have a couple of thoughts that may be improvements. But let me know what you think. I'm open to being wrong.

[ximinez]

I'm going to second @scottschurr's suggestions. Allowing the loop to continue on failure, and structuring the code so that no incomplete data may be hanging around are really good improvements.

Also, could you share the patch that you used to get that malformed transaction? It might be generalizable to a solution that we could also include in rippled (in a separate PR).

[cjcobb23]

I actually just copied the way the JSON handler does this:

rippled/src/ripple/app/ledger/impl/LedgerToJson.cpp

Line 176 in ebbf4b6

try

I agree it would be better to continue on failure, but maybe that should be a separate PR? I just wanted to get things back up and running for clio, because right now the entire call returns an error (unlike the JSON version of the call).

I unfortunately don't have the patch that actually deserialized this transaction, it was so long ago.

[scottschurr]

@cjcobb23, I'm not sure I understand the motivation for putting the suggested change in a separate pull request. The suggested code changes are not extensive and they are localized. I'm not guaranteeing the suggested change is bug free; it needs testing. But the implementation in the pull request has bugs that have been pointed out.

You haven't yet convinced me that it's a better idea to stay with the implementation in the pull request when we know that the pull request has fixable flaws. But I'm listening.

[cjcobb23]

The implementation in the pull request mirrors the JSON implementation that already exists. The exception is actually thrown as part of the loop iteration, not in the body of the loop. So there won't be any incomplete data. We will skip later transactions, as does the JSON implementation. However, I'd rather solve that issue in a later pull request.

[scottschurr]

I now understand what @cjcobb23 means by...

The exception is actually thrown as part of the loop iteration, not in the body of the loop.

It means the exception is thrown here:

for (auto& i : ledger->txs)

So the restructure I suggested will not actually catch the exception.

Thanks for the explanation. I withdraw my suggestion. I think this pull request is good to go.

[ximinez]

Thanks to @cjcobb23 and @scottschurr 's explanations, I see now why the solution was implemented this way. It'll work for now until we can make the iterator smarter or more robust. And thanks for clarifying the log message!

[intelliot]

Mentioning @manojsdoshi - please sync with @cjcobb23 to see how this should be tested.