TxResultsTests are buggy and should be fixed

[vogel]

There are several exceptions thrown during testnet catchup, we need to check what is causing those:

2018-01-31T22:21:33.695 GCB2O [History INFO] Starting catchup with configuration:
  lastClosedLedger: 2
  toLedger: 1762688
  count: 4294967295

2018-02-01T04:39:33.202 GCB2O [History INFO] Catching up: applying checkpoint 5969/27542 (21%)
2018-02-01T04:39:33.205 GCB2O [Ledger ERROR] Exception during tx->apply: Could not update data in SQL [LedgerManagerImpl.cpp:946]
2018-02-01T04:39:33.404 GCB2O [History INFO] Catching up: applying checkpoint 5970/27542 (21%)

2018-02-01T05:47:11.939 GCB2O [History INFO] Catching up: applying checkpoint 26500/27542 (96%)
2018-02-01T05:47:11.990 GCB2O [Ledger ERROR] Exception during tx->apply: XLM TrustLine? [LedgerManagerImpl.cpp:946]
2018-02-01T05:47:12.081 GCB2O [History INFO] Catching up: applying checkpoint 26501/27542 (96%)

2018-01-31T22:21:32.604 GCB2O [History INFO] Starting catchup with configuration:
  lastClosedLedger: 1
  toLedger: 5286144
  count: 0

2018-02-01T06:03:22.632 GCB2O [History INFO] Catching up: applying checkpoint 18951/27532 (68%)
2018-02-01T06:03:22.650 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.668 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.699 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.741 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.772 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.783 GCB2O [Ledger ERROR] Exception during tx->apply: Unexpected error code from pathPayment [LedgerManagerImpl.cpp:946]
2018-02-01T06:03:22.803 GCB2O [History INFO] Catching up: applying checkpoint 18952/27532 (68%)

Those exceptions do not cause catchup error, so we probably ends with txINTERNAL_ERROR, but this is interesting what causes those.

[mfontanini]

I was looking at this one. I'm catching up and still haven't hit the path payment ones but for the first 2 you posted:

• The "XLM TrustLine?" one is easily reproducible: this is caused by a change trust operation being submitted using the native asset as the asset being trusted. This currently fails with internal error but I imagine it should return a more user friendly error. I've pushed a commit addressing this. This would require bumping the ledger protocol version. If that works, I can make a PR for it.
• The "Could not update data in SQL" one is caused by this transaction which tries to perform an account merge and a payment which fails for obvious reasons. I noticed there is actually a test that ensures this sort of transaction returns internal error. Is that really the desired behavior? This could probably be better handled by e.g. returning some error code to indicate the transaction tries to do something impossible rather than this being an internal error, which doesn't really help the user understand what happened.

[MonsieurNicolas]

cool, thanks for looking into these!

First one you should open a PR with the following changes

• don't bump the version to 11, current master already bumped it to 10 (9 is current pubnet version)
• move the check to ChangeTrustOpFrame::doCheckValid (there is a check already for isAssetValid but native is a valid asset)
• Use the same CHANGE_TRUST_MALFORMED error code

Second one, the test code is actually buggy and should be fixed

if you look at https://github.com/stellar/stellar-core/blob/master/src/transactions/TxResultsTests.cpp#L205 it's actually not comparing the result of the transaction if it runs into an internal error.

This is wrong: the 3rd operation in the current version of the protocol (since version 7 I think, I am going to add a complete protocol release notes shortly to make it easier to track those down) will fail with opNO_ACCOUNT as expected).

This validate lambda is error prone: we should never skip the apply check if applyResult is set. Also, I wonder if most of the code in that lambda should be replaced to use txtest::applyCheck.

Note that when running transaction tests you should run tests with the --all-versions option on the command line, for example: ./stellar-core --test --all-versions '[tx]'

[mfontanini]

Ah I didn't scroll down far enough to see ChangeTrustOpFrame::doCheckValid. Also, CHANGE_TRUST_MALFORMED seems to be appropriate for this error as well. I'll create the PR with those changes later today.

As for the transaction test, I'll check it out later. Also, I wasn't aware of the --all-versions parameter.

[MonsieurNicolas]

I am renaming this issue to track the second issue that was identified (tests are wrong)