Page MenuHomePhabricator

Hash the input of OP_CHECKDATASIG instead of expecting the user to do it.
ClosedPublic

Authored by deadalnix on Aug 12 2018, 22:02.

Details

Summary

This is safer

Co-authored-by: Amaury Séchet <deadalnix@gmail.com>
Co-authored-by: Jason B. Cox <contact@jasonbcox.com>

Test Plan

Updated tests to reflect the change.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Yes, based on feedback we received late in review, I agree this is a safer approach.

This revision is now accepted and ready to land.Aug 13 2018, 00:06
jasonbcox requested changes to this revision.

abc-checkdatasig-activation.py test fails

This revision now requires changes to proceed.Aug 13 2018, 02:26
src/script/interpreter.cpp
942 ↗(On Diff #4565)

Does ss.GetHash() return double-SHA256? That's how I wrote it in the Spec.

Maybe this is crazy, but would it be possible to have the opcode use single-SHA256 for the hash function?
That would make it easier to construct transaction with spend conditions based on SigHashes of other unrelated transactions being signed.

Eg: https://bitco.in/forum/threads/gold-collapsing-bitcoin-up.16/page-1213#post-75916

src/script/interpreter.cpp
942 ↗(On Diff #4565)

Correct, this is double-sha256.

src/script/interpreter.cpp
942 ↗(On Diff #4565)

OK, thanks. That confirms the spec is worded properly.

You can ignore the rest of my above comment.. It was just an idea, but unclear if it's a benefit, and not worth changing this late.

jasonbcox edited reviewers, added: deadalnix; removed: jasonbcox.

Commandeering to fix the test since Amaury is away.

schancel requested changes to this revision.Aug 13 2018, 20:03
schancel added a subscriber: schancel.
schancel added inline comments.
test/functional/abc-checkdatasig-activation.py
55 ↗(On Diff #4579)

Not sure about this fix. It looks like you intentionally NULLDUMMY the fail, and then OP_NOT it. However, that doesn't check that the signature was verified correctly? Are we asserting that the unit test does that instead?

This revision now requires changes to proceed.Aug 13 2018, 20:03
This revision is now accepted and ready to land.Aug 13 2018, 20:20
deadalnix edited reviewers, added: jasonbcox; removed: deadalnix.
test/functional/abc-checkdatasig-activation.py
55 ↗(On Diff #4579)

Yeah, looks like it calls it in way that we can easily see that the opcode is called and should return FALSE.

So test that the opcode is activated, but not whether it works properly for checking various signatures.

Fix abc-checkdatasig-activation.py

This revision was automatically updated to reflect the committed changes.