TransactionError::ALREADY_IN_CHAIN inconsistency

[hebasto]

If a transaction is already in the block chain, and all of its outputs are spent, the BroadcastTransaction() fails to detect the correct transaction status. It returns TransactionError::MISSING_INPUTS instead of TransactionError::ALREADY_IN_CHAIN.

The related code:

bitcoin/src/node/transaction.cpp

Lines 32 to 37 in 80fd474

	for (size_t o = 0; o < tx->vout.size(); o++) {
	const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o));
	// IsSpent doesn't mean the coin is spent, it means the output doesn't exist.
	// So if the output does exist, then this transaction exists in the chain.
	if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN;
	}

I think that TransactionError::ALREADY_IN_CHAIN should be returned in all cases when the transaction is in the block chain, or it should be dropped from the code at all as inconsistent.

What are other opinions?

[willcl-ark]

Although it is inconsistent I slightly prefer the idea of returning the most accurate information available to the user.

Following this logic instead of removing this error type, we could take two new actions to try and increase the chance that the caller gets more accurate information back:

1. Inside BroadcastTransaction() add a call to GetTransaction() on the condition that the user has txindex enabled
2. Augment HandleATMPError to include a more detailed error string in the TxValidationResult::TX_MISSING_INPUTS case, which might prompt a recipient to find another way to check if the transaction is already in the chain.