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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Aug 12 2018, 22:02
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 12 2018, 22:02

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

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

abc-checkdatasig-activation.py test fails

This revision now requires changes to proceed.Aug 13 2018, 02:26
Mengerian added inline comments.Aug 13 2018, 04:47
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

jasonbcox added inline comments.Aug 13 2018, 15:26
src/script/interpreter.cpp
942 ↗(On Diff #4565)

Correct, this is double-sha256.

Mengerian added inline comments.Aug 13 2018, 17:45
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 commandeered this revision.Aug 13 2018, 19:43
jasonbcox edited reviewers, added: deadalnix; removed: jasonbcox.

Commandeering to fix the test since Amaury is away.

jasonbcox updated this revision to Diff 4579.Aug 13 2018, 19:44

Fixed functional test

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
schancel accepted this revision.Aug 13 2018, 20:20
This revision is now accepted and ready to land.Aug 13 2018, 20:20
deadalnix commandeered this revision.Aug 13 2018, 20:21
deadalnix edited reviewers, added: jasonbcox; removed: deadalnix.
Mengerian added inline comments.Aug 13 2018, 20:22
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.

jasonbcox edited the summary of this revision. (Show Details)Aug 13 2018, 20:23
deadalnix updated this revision to Diff 4580.Aug 13 2018, 20:25

Fix abc-checkdatasig-activation.py

jasonbcox accepted this revision.Aug 13 2018, 20:31
This revision was automatically updated to reflect the committed changes.