search

JoshOrndorff / utxo-workshop

A Substrate UTXO workshop
The Unlicense
117 stars 81 forks source link

Upgrading workshop to 2.0; resolving critical security issues #45

Closed nczhu closed 4 years ago

nczhu commented 4 years ago

Resolves issue #37

Includes:

nczhu commented 4 years ago

new commits solve issue #43

nczhu commented 4 years ago

new commits resolve #36

nczhu commented 4 years ago

new commits resolve #39

nczhu commented 4 years ago

New commits fix #46 and #48

nczhu commented 4 years ago

Latest commits address #49

nczhu commented 4 years ago

I've looked through the code and overall I'm very impressed. In general I think these are good changes, and after some minor cleanups that I mentioned, I think they should be merged.

Feedback on the three specific things you asked me to look for.

Salt Generation Your design is elegant and secure (afaict). Let's stick with it! It leads to a few oddities about storing UTXOs that were created at genesis or as reward payouts, but I think those things are solvable.

Front Running Your design is elegant and secure (afaict). Let's stick with it!

TxPool Use I actually think this change should be reverted, and all validity checks (invalid sig, outputs > inputs, etc) be made in the tx pool. The reason is that UTXO spends are unsigned transactions. If we don't check their validity before they make it into a block, then an attacker can send many invalid transaction all of which will be included in a block without paying fees. When we check validity in the tx pool, the node that receives the tx can immediately realize it is invalid, and not even gossip it. But when we don't check that, the transaction makes it all the way into a block. Further, we are unable to prioritize the transactions by their reward value.

Yeah good point. I made this change when still checked for signed extrinsics. Now that we're accepting gasless extrinsics (which is a cool way to demonstrate Substrate's versatility)... I'm going to revert this change. Thanks!

nczhu commented 4 years ago

New commits address @JoshOrndorff 's awesome pr, including change request to revert my refactoring things out of the trx_pool.

Also addresses issue #35 , moving priority ordering logic in utxo.rs. This ended up being less code as well - so original mission accomplished.

@JoshOrndorff if you are generally ok with this version, ignoring comments/linting/minor things, I'd like to merge this PR asap to unblock several future work that fork off from here. thanks again