Page MenuHomePhabricator

Add activation code for OP_CHECKDATASIG
ClosedPublic

Authored by deadalnix on Aug 5 2018, 20:55.

Details

Summary

As per title.

Depends on D1623 and D1621

Test Plan

Added an integration test to check for proper activation and deactivation.

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 5 2018, 20:55
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 5 2018, 20:55
jasonbcox requested changes to this revision.Aug 5 2018, 22:50
jasonbcox added subscribers: schancel, jasonbcox.
jasonbcox added inline comments.
src/validation.cpp
1928 ↗(On Diff #4507)

acceptign -> accepting

2584 ↗(On Diff #4507)

transactiosn -> transactions

test/functional/abc-checkdatasig-activation.py
76 ↗(On Diff #4507)

OP_AND should be OP_CHECKDATASIG. very confusing otherwise.

87 ↗(On Diff #4507)

spendable_ands -> spendable_checkdatasigs

101 ↗(On Diff #4507)

opreturn -> something about checkdatasig

139 ↗(On Diff #4507)

I'm confused how this magic number 6 was selected. With MTP being the last 11 blocks, does selecting fewer potentially make the test flaky and/or fragile to future changes, as there could be blocks mined before these 6 with unknown timestamps?

146 ↗(On Diff #4507)

good check. I didn't think about testing the RPC in conjunction with the block acceptance. @schancel we should be on the lookout for stuff like this. in the future, a solid testing framework would be able to make checks like this for us.

This revision now requires changes to proceed.Aug 5 2018, 22:50
deadalnix added inline comments.Aug 6 2018, 20:10
test/functional/abc-checkdatasig-activation.py
139 ↗(On Diff #4507)

We are good for several decades.

146 ↗(On Diff #4507)

Yes please.

deadalnix updated this revision to Diff 4519.Aug 6 2018, 20:18

Fix various typos and erroneous comments/variable names.

jasonbcox added inline comments.Aug 7 2018, 14:44
test/functional/abc-checkdatasig-activation.py
139 ↗(On Diff #4507)

You're referring to the MAGNETIC_ANOMALY_START_TIME value, but I'm talking about MTP calculated using 11 blocks' worth of timestamps, not 6.

schancel requested changes to this revision.Aug 7 2018, 19:07
schancel added inline comments.
test/functional/abc-checkdatasig-activation.py
47 ↗(On Diff #4519)

Can you please make this use self.node[0].calculate_fee() on the CTransaction so I don't have to go fix this? Is this even right since it's not multiplied by the size of the transaction?

71 ↗(On Diff #4519)

:(

This revision now requires changes to proceed.Aug 7 2018, 19:07
deadalnix added inline comments.Aug 7 2018, 22:08
test/functional/abc-checkdatasig-activation.py
139 ↗(On Diff #4507)

You need to pour in 6 new timestamp for the median to budge. I don't see what's the problem here.

deadalnix updated this revision to Diff 4530.Aug 8 2018, 19:47

Use calculate_fee

schancel accepted this revision.Aug 8 2018, 19:49
jasonbcox accepted this revision.Aug 8 2018, 20:03
This revision is now accepted and ready to land.Aug 8 2018, 20:03
This revision was automatically updated to reflect the committed changes.