Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP 119: CHECKTEMPLATEVERIFY #875

Merged
merged 4 commits into from Jan 24, 2020
Merged

BIP 119: CHECKTEMPLATEVERIFY #875

merged 4 commits into from Jan 24, 2020

Conversation

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Jan 6, 2020

I believe now is an appropriate time to apply for a BIP number/Draft Status for OP_CTV. A few elements will require updating before the BIP moves out of draft (e.g., the activation dates) but activation dates are a separate discussion from the technical considerations which are the focus of this BIP.

@luke-jr luke-jr added the New BIP label Jan 17, 2020
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jan 17, 2020

BTW I looked into the travis error -- it seems that the linter is reading a code block and failing because a C++ lambda looks like a improperly formatted markdown link.

I am happy to add a revert-able patch as below:

bool filter(const CTxIn& c) {
    return c.scriptSig != CScript();
}
uint256 GetStandardTemplateHash(const CTransaction& tx, const uint256& outputs_hash, const uint256& sequences_hash,
                                    const uint32_t input_index) {
        bool skip_scriptSigs = std::find_if(tx.vin.begin(), tx.vin.end(), filter) == tx.vin.end();
        return skip_scriptSigs ? GetStandardTemplateHashEmptyScript(tx, outputs_hash, sequences_hash, input_index) :
            GetStandardTemplateHashWithScript(tx, outputs_hash, sequences_hash, GetScriptSigsSHA256(tx), input_index);
    }

so that you don't have to muck around with a broken linter/supressions, but I think it's better to fix the linter longer term.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 19, 2020

@JeremyRubin This is missing a backwards compatibility section.

@kallewoof Can you look into a better fix for the linter issue?

@kallewoof

This comment has been minimized.

Copy link
Member

kallewoof commented Jan 20, 2020

Looking.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jan 21, 2020

@luke-jr it was unclear to me that a backwards compatibility section was needed for this BIP, referencing BIP-65 and BIP-112 as examples. Neither covers the OP_NOP reinterpretation as being an incompatibility.

Regardless, I can push a section noting that there are no Backwards Incompatibilities.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jan 21, 2020

  • BIP 65 -- CLTV/CSV.
JeremyRubin added 2 commits May 17, 2019
… change 'Implementations' header to 'Reference Implementation'
@JeremyRubin JeremyRubin force-pushed the JeremyRubin:ctv branch from c86c548 to c36e492 Jan 21, 2020
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jan 21, 2020

Added a backwards compatibility section; and rebased to pick up the linter fixes.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 24, 2020

Use BIP 119

@luke-jr luke-jr changed the title Add BIP for CheckTemplateVerify BIP 119: Add BIP for CheckTemplateVerify Jan 24, 2020
@luke-jr luke-jr changed the title BIP 119: Add BIP for CheckTemplateVerify BIP 119: CHECKTEMPLATEVERIFY Jan 24, 2020
JeremyRubin added 2 commits Jan 24, 2020
@JeremyRubin

This comment has been minimized.

Copy link
Contributor Author

JeremyRubin commented Jan 24, 2020

Thanks Luke! I've set the number & fixed links.

@luke-jr luke-jr merged commit 0042dec into bitcoin:master Jan 24, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.