Identify non-standard received legacy enotes #43
Conversation
| LegacyContextualBasicEnoteRecordV1 temp_contextual_record{}; | ||
| bool found_an_enote{false}; | ||
|
|
||
| for (std::size_t enote_index{0}; enote_index < legacy_additional_enote_ephemeral_pubkeys.size(); ++enote_index) | ||
| for (std::size_t enote_index{0}; legacy_additional_enote_ephemeral_pubkeys.size() > 0 && |
There was a problem hiding this comment.
| for (std::size_t enote_index{0}; legacy_additional_enote_ephemeral_pubkeys.size() > 0 && | |
| for (std::size_t enote_index{0}; enote_index < legacy_additional_enote_ephemeral_pubkeys.size() && enote_index < enotes_in_tx.size(); ++enote_index) |
Otherwise it's UB it can segfault if legacy_additional_enote_ephemeral_pubkeys.size() > 0 && legacy_additional_enote_ephemeral_pubkeys.size() < enotes_in_tx.size()
There was a problem hiding this comment.
Oopsie,
thanks for the review :)
| // 4. check if there is a main enote ephemeral pubkey | ||
| if (legacy_main_enote_ephemeral_pubkey == rct::rct2pk(rct::I)) | ||
| // 3. check if there is a main enote ephemeral pubkey | ||
| if (legacy_main_enote_ephemeral_pubkeys.front() == rct::rct2pk(rct::I)) |
There was a problem hiding this comment.
This looks like another mismatch to wallet2. wallet2 skips processing the transaction's enotes entirely if there is no main enote ephemeral pub key:
Can be handled in a follow-up PR, but I would suggest extract_legacy_enote_ephemeral_pubkeys_from_tx_extra can just return false if there is no main enote ephemeral pub key (don't even add rct::I), and try_find_legacy_enotes_in_tx can also return false immediately in that case too (and not process additional tx pub keys).
There was a problem hiding this comment.
To be clear, this comment is totally unrelated to this PR and this looks like a distinct issue already present :)
There was a problem hiding this comment.
I don't think it is necessary to replicate this particular quirk of wallet2's spaghetti, since it is strictly a 'feature reduction'.
There was a problem hiding this comment.
Ya I was debating this. Fine with me to keep it
There was a problem hiding this comment.
Biggest issue I can think of is you could theoretically send dust to someone who confirms / denies receipt in order to see if they're running a Seraphis lib wallet versus wallet2
There was a problem hiding this comment.
Biggest issue I can think of is you could theoretically send dust to someone who confirms / denies receipt in order to see if they're running a Seraphis lib wallet versus wallet2
Don't think this is significant enough to warrant a change.
SNeedlewoods
force-pushed
the
x_identify_edge_cases
branch
from
c699605
to
b4a3e7f
Compare
| // 4. check if there is a main enote ephemeral pubkey | ||
| if (legacy_main_enote_ephemeral_pubkey == rct::rct2pk(rct::I)) | ||
| // 3. check if there is a main enote ephemeral pubkey | ||
| if (legacy_main_enote_ephemeral_pubkeys.front() == rct::rct2pk(rct::I)) |
There was a problem hiding this comment.
I don't think it is necessary to replicate this particular quirk of wallet2's spaghetti, since it is strictly a 'feature reduction'.
| // - this step is automatically skipped if legacy_additional_enote_ephemeral_pubkeys.size() == 0 | ||
| crypto::key_derivation temp_DH_derivation; | ||
| std::vector<crypto::key_derivation> temp_DH_derivations; |
There was a problem hiding this comment.
| // - this step is automatically skipped if legacy_additional_enote_ephemeral_pubkeys.size() == 0 | |
| crypto::key_derivation temp_DH_derivation; | |
| std::vector<crypto::key_derivation> temp_DH_derivations; | |
| // - this step is automatically skipped if legacy_additional_enote_ephemeral_pubkeys.size() == 0 | |
| crypto::key_derivation temp_DH_derivation; | |
| std::vector<crypto::key_derivation> temp_DH_derivations{}; | |
| temp_DH_derivations.reserve(legacy_main_enote_ephemeral_pubkeys.size()); |
Also, move temp_DH_derivations closer to where it will be used so this allocation is below the early-return.
There was a problem hiding this comment.
I'd honestly go even further and make a variant<rct::key, vec<rct::key>> for this and legacy_main_enote_ephemeral_pubkeys since most of the time there will only be one normal pubkey, and this function is right on the hot path.
There was a problem hiding this comment.
I tried to implement this suggestion, but I'm not sure about it.
E.g. the castings between crypto::key_derivation<->rct::key<->crypto::public_key seem messy.
Also I initialize the variant legacy_main_enote_ephemeral_pubkey to rct::key, but then in extract_legacy_enote_ephemeral_pubkeys_from_tx_extra() I set it to rct::keyV, because this way the function needs less changes, but not sure if that defeats the whole purpose of introducing the new variant.
--------- Co-authored-by: SNeedlewoods <sneedlewoods_1@protonmail.com>
This should address the issue #27