Skip to content

Instantly share code, notes, and snippets.

@georgeee
Last active February 22, 2018 08:13
Show Gist options
  • Select an option

  • Save georgeee/4145fa107f8536cc0ccdb09fac1cd853 to your computer and use it in GitHub Desktop.

Select an option

Save georgeee/4145fa107f8536cc0ccdb09fac1cd853 to your computer and use it in GitHub Desktop.
Jose Toro submission review (Serokell test task)

Review

Hi, Jose!

Please see the review for your submission.

Most part of review will be conveyed in terms of validating criteria.

Review process is designed to be interactive, so that we will point you to mistakes you made in your submission and propose you to resolve some of these. Criteria will be checked in blocks, you'll get rundown along with descriptions as result of each iteration.

Also, please note that from now on, latest test task description (along with archive) can be found here.

Off-criteria comments

  1. Regarding comment in code:

should use all fields in transaction if attacker have the ability to send any data on the net, but is ok in this case

Well, attacker can initiate conversation with any node :). See latest description, Security model description is still messy, but at least elaborated.

  1. Could you elaborate on your blockchain solution, are forks deeper than 1 possible? (It's hard for me to determine it from code)?

Validation criteria

Criterion 7: Transaction id is cryptographically insecure

  1. If some hash function is used, it should be strong one.
  2. If no hash is used, equivallent solution should be sufficiently secure

Note, that signature of transaction by ed25519 is not considered a secure solution unless you provide strong argument about that (that signature is unique, resulting value uncorrelated).

Criterion 8: It's possible to create transaction with any sender

Typically, transaction is secured by signature of Sender. If it is not being done or is being done in wrong way, this will allow Adversary to construct arbitrary transaction and abuse Ledger.

Criterion 9: Double-send: repeated send of eavesdropped transaction

If an adversary eavesdrops transaction T = (ReceiverAddress, SenderAddress, Amount) from the conversation, he can send T to network again and this will cause T to be executed second time without intention by Sender (which will lead obviously to decrease of funds on Sender's account).

Criterion 21: Transaction may get lost

Assume all nodes remain online, network parameters (CONC, DELAY) not set, stabilityTimeot=60000.

Some valid transaction is submitted to a single node in cluster. After stabilityTimeout every node returns 0 on QUERY txId.

This criterion is validated by test: few transactions operating with different accounts A1, A2, A3, .. are simultaneously submitted to few different nodes of 8-node cluster (in potentially different order). As a result at least one of submitted transactions is completely lost after stabilityTimeout.

@JoseD92
Copy link

JoseD92 commented Feb 21, 2018

Ok, I can see my solution failing (regarding security) if you allow attacker to resend, although it can be easily fix using all fields of transaction to generate the hash. But don’t change it, I kind of don’t want to fix it :).
Regarding criterion 7, I just picked the first hash function that gave me the required number of bytes that could be read be human (easier to test code that way), a version that return human unreadable bytes would be better but I don’t really think your test will exhaust the first one.
Could you elaborate a bit on criterion 21, thanks.

@georgeee
Copy link
Author

few transactions operating with different accounts A1, A2, A3, .. are simultaneously submitted to few different nodes of 8-node cluster (in potentially different order). As a result at least one of submitted transactions is completely lost after stabilityTimeout.

Imagine this experiment.

Essentially if new block arrives at node N which had some older block (forming a fork), older block will be thrown away (useful transactions not preserved anywhere).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment