check transaction before broadcasting / accepting

[HarukaMa]

Motivation

Check if the transaction is valid before broadcasting. Prevents users accidentally generating invalid transactions then wonder where did they go after it got rejected by other nodes.

This check was there before Sep 2022. Not sure why it was removed.

Test Plan

(If you changed any code, please provide clear instructions on how you verified your changes work.) My invalid tx could be rejected.

⚠️  ❌ Failed to broadcast execution 'credits.aleo/transfer_private' to https://explorer.hamp.app/testnet3/transaction/broadcast: (status code 500: "Something went wrong: Invalid transaction: The input ID '7798950585313772830287778856136311499267216010577198944948157594845486864875field' already exists in the ledger")

Related PRs

(Link any related PRs here)

[HarukaMa]

Oops, edited the code but forgot to git add, will change later

[howardwu]

There is the concern that it is possible the user broadcasts 2 or more transactions, whereby the first transaction makes the second (and later) transactions valid. But the verifier will not know this without speculative execution in advance.

[HarukaMa]

This check is also present in the inbound method for UnconfirmedTransaction, so unless the user is broadcasting directly to a validator, they won't succeed as their later transactions won't be accepted by other nodes anyway.

[howardwu]

Great point, we did add that check in as a preemptive safeguard. I agree it wouldn't be a significant feature change to include this one too.

Do you know if there are any major performance implications we should be aware of with this PR?

[HarukaMa]

Actually I did think about if one can just spam invalid transactions, but didn't actually try to profile the check function. It does seems a little expensive as the "basic" check did try to thoroughly verify many things included in the transaction.

Without the check though, the spammer will be spamming all connected nodes instead. But admittedly losing a public API endpoint would be worse.

Probably I need to generate some fake data to test if this could have severe performance implications.

[HarukaMa]

I just realized we have https://github.com/AleoHQ/snarkOS/pull/2942 so we don't need to specifically worry about attacks as you can still DDoS other endpoints, especially /blocks.

I did test the check function and it took 50~100ms on my server to verify a valid transaction and similar transactions with a bad execution proofs. The timing variation seems to come from storage accesses to verify duplicated IDs. I currently don't have resources to actually test if flooding would take down a node, but from the time above, heuristically it's indeed possible even with legitimate traffic (think a large amount of valid transactions during a event of some project).

That said, that amount of traffic could disable other nodes processing UnconfirmedTransaction messages as well if we don't add this check here.