PaymentOpFrame cannot handle PATH_PAYMENT_MALFORMED result

[jonjove]

PathPaymentOpFrame::doApply can produce the operation result PATH_PAYMENT_MALFORMED but PaymentOpFrame::doApply does not handle this result. This leads to txINTERNAL_ERROR, instead of producing the corresponding operation result PAYMENT_MALFORMED. Two options to fix this:

• Don't return PATH_PAYMENT_MALFORMED during apply, returning more specific results instead that PaymentOpFrame can handle
• Handle PATH_PAYMENT_MALFORMED in PaymentOpFrame

[MonsieurNicolas]

I see, I just looked and PATH_PAYMENT_MALFORMED is just wrong (MALFORMED is normally used to mean "bad arguments"), but here it returns that in other cases (not sure I understand when those actually happen as it's in the NATIVE case, is it liabilities related?)

[jonjove]

I see two places in PathPaymentOpFrame::doApply that can return PATH_PAYMENT_MALFORMED.

stellar-core/src/transactions/PathPaymentOpFrame.cpp

Lines 78 to 86 in 4833465

	if (curB.type() == ASSET_TYPE_NATIVE)
	{
	auto destination = stellar::loadAccount(ltx, mPathPayment.destination);
	if (!addBalance(ltx.loadHeader(), destination, curBReceived))
	{
	innerResult().code(PATH_PAYMENT_MALFORMED);
	return false;
	}
	}

This return code was already there as of (at least) v9.0.0. But I think this was intended to be an unexpected error condition at that time, along the lines of how we use txINTERNAL_ERROR now. Since the introduction of liabilities, this case can occur (imagine the account owns an offer trying to buy a huge amount of native asset). I think we can make this return PATH_PAYMENT_LINE_FULL starting in version 11, which PaymentOpFrame::doApply is able to handle.

stellar-core/src/transactions/PathPaymentOpFrame.cpp

Lines 202 to 210 in 4833465

	if (header.current().ledgerVersion > 7)
	{
	sourceAccount = stellar::loadAccount(ltx, getSourceID());
	if (!sourceAccount)
	{
	innerResult().code(PATH_PAYMENT_MALFORMED);
	return false;
	}
	}

Correct me if I'm wrong, but I don't think this should be reachable in the current protocol version. OperationFrame::checkValid checks the existence of the source account and returns opNO_ACCOUNT if it does not exist, and it always executes before OperationFrame::doApply. Should we do anything for this case?

[MonsieurNicolas]

cool, yes both make sense (return line full when dealing with liabilities) and for the second one, I don't think there is anything to do as it's probably unreachable code now.