Page MenuHomePhabricator

Add activation code for OP_CHECKDATASIG

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



As per title.

Depends on D1623 and D1621

Test Plan

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

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

jasonbcox requested changes to this revision.Aug 5 2018, 22:50
jasonbcox added subscribers: schancel, jasonbcox.
jasonbcox added inline comments.
1928 ↗(On Diff #4507)

acceptign -> accepting

2584 ↗(On Diff #4507)

transactiosn -> transactions

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
139 ↗(On Diff #4507)

We are good for several decades.

146 ↗(On Diff #4507)

Yes please.

Fix various typos and erroneous comments/variable names.

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.
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
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.

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