Fix for non-spendable merged mining reward

[amphibia]

No description provided.

[monero-project]

I want to look into this a little more before merging, as it appears to ignore false returns of parse_tx_extra which could be a bad thing.

In the future please test outputs from your code to ensure it passes checks by adding a new testing function in tests/unit_tests/test_format_utils.cpp.

[amphibia]

Currently, there is a problem that you can't spend coins which you've mined with merged mining. This fix is resolving this issue - I would recommend you to merge it as soon as possible since the more we wait - the more people are facing this issue.
This fix is the same as the code in wallet2::process_new_transaction() method. Parse_tx_extra() is called and in case of an error work continues. Wallet2::process_new_transaction() method is ending with an error only in case of false response from find_tx_extra_field_by_type<tx_extra_pub_key>() function.
Now get_tx_pub_key_from_extra() function is working this way: if parse_tx_extra() returns false - it ignores it, null_pkey returns only if find_tx_extra_field_by_type<tx_extra_pub_key>() returns false.

[monero-project]

This issue has been fixed by these commits:
5c820f9
73158c7

This adds a custom handler/serializer for merge mining datafields contained in tx_extra, and still causes the wallet to throw errors when bizarre data is added in tx_extra.

Please let me know if these changes are unsatisfactory.

[amphibia]

Yes, indeed your changes are correct you’re receiving MM transactions just fine, for that I apologise. But my changes are intended for transaction’s extra processing. This way your daemon will be able to receive any transactions - including the ones with pretty much anything in extra field and users will not face “unspendable money” issue.

[monero-project]

Right, I agree it may be a bug in ByteCoin's code to throw an error there. I think perhaps parse_tx_extra itself should be modified so that it reports an error ("unidentified tx_extra fields contained in transaction [hash]") but does not throw an error through CHECK_AND_NO_ASSERT_MES that causes the transfer function to crash. Let me confer with the other devs and see what they think.

edit:
Oh, sorry, it looks like that may be an inaccurate description of the crash; I think it goes through bool lookup_acc_outs(...) {...} when it returns false unless I'm mistaken. So corrected behaviour should be something like (pseudocode):

deserialize tx_extra
determine tx_extra_type
if (tx_extra_type == UNKNOWN) { log error }
else if (tx_extra_type == pubkey_containing) { extract_pubkeys }

and the error is thrown at the level of

bool r = lookup_acc_outs(m_account.get_keys(), tx, tx_pub_key, outs, tx_money_got_in_outs);
THROW_WALLET_EXCEPTION_IF(!r, error::acc_outs_lookup_error, tx, tx_pub_key, m_account.get_keys());

in wallet2.cpp