-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: limit monero hashes and force coinbase to be tx 0 #5602
fix!: limit monero hashes and force coinbase to be tx 0 #5602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
was initially worried about some of the test code removal but I see it's only the code that tests proofs for arbitrary tx hashes 👍
// Proof for h0 | ||
let proof = create_merkle_proof(&hashes, &hashes[0]).unwrap(); | ||
let proof = create_merkle_proof(&hashes).unwrap(); | ||
assert_eq!(proof.calculate_root(&hashes[0]), expected_root); | ||
assert_eq!(proof.depth, 2); | ||
assert_eq!(proof.branch().len(), 2); | ||
assert_eq!(proof.branch()[0], hashes[1]); | ||
assert_eq!(proof.branch()[1], h2345); | ||
assert_eq!(proof.path_bitmap, 0b00000000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While checking things I added 2 hashes to this test. Up to you if you want to add it, not NB. Obviously if you do you'll need to update/remove the "diagram" :)
let hashes = (1..=8).map(|i| Hash::from([i; 32])).collect::<Vec<_>>();
let h23 = cn_fast_hash2(&hashes[2], &hashes[3]);
let h45 = cn_fast_hash2(&hashes[4], &hashes[5]);
let h67 = cn_fast_hash2(&hashes[6], &hashes[7]);
let h01 = cn_fast_hash2(&hashes[0], &hashes[1]);
let h0123 = cn_fast_hash2(&h01, &h23);
let h4567 = cn_fast_hash2(&h45, &h67);
let expected_root = cn_fast_hash2(&h0123, &h4567);
// Proof for h0
let proof = create_merkle_proof(&hashes).unwrap();
assert_eq!(proof.calculate_root(&hashes[0]), expected_root);
assert_eq!(proof.branch().len(), 3);
assert_eq!(proof.branch()[0], hashes[1]);
assert_eq!(proof.branch()[1], h23);
assert_eq!(proof.branch()[2], h4567)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utAck
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
f757526
to
db73d01
Compare
Description
This limits the merkle proof hashes from monero blocks to be all used stopping miners from adding junk data at the end that is not used.
Also brings the merkle root construction inline with monero rule by forcing the coinbase to be tx 0.
Motivation and Context
If the hashes are not limited, miners can add endless data to the header. This causes TARI-006 and TARI-008, TARI-009, TARI-011 and TARI-012
How Has This Been Tested?
Unit tests
Audit Finding Number
TARI-006
TARI-008
TARI-009
TARI-011
TARI-012