Add activation code for OP_CHECKDATASIG
ClosedPublic
Actions

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

Details

Reviewers

jasonbcox
schancel

Group Reviewers

Restricted Project

Commits

Summary

As per title.

Depends on D1623 and D1621

Test Plan

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

Repository

rABC Bitcoin ABC

Branch

checksigactivation

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3023
Build 4139: Bitcoin ABC Buildbot (legacy)
Build 4138: arc lint + arc unit

jasonbcox requested changes to this revision.Aug 5 2018, 22:50

jasonbcox added inline comments.

src/validation.cpp
1928	acceptign -> accepting
2584	transactiosn -> transactions
test/functional/abc-checkdatasig-activation.py
76	OP_AND should be OP_CHECKDATASIG. very confusing otherwise.
87	spendable_ands -> spendable_checkdatasigs
101	opreturn -> something about checkdatasig
139	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	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

test/functional/abc-checkdatasig-activation.py
139	We are good for several decades.
146	Yes please.

Fix various typos and erroneous comments/variable names.

test/functional/abc-checkdatasig-activation.py
139	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)	:(