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

Introduce assumevalid setting to skip validation presumed valid scripts. #9484

Merged
merged 2 commits into from Jan 16, 2017

Conversation

@gmaxwell
Copy link
Member

gmaxwell commented Jan 6, 2017

This disentangles the script validation skipping from checkpoints.

A new option is introduced "assumevalid" which specifies a block whos
ancestors we assume all have valid scriptsigs and so we do not check
them when they are also burried under the best header by a weeks
worth of work.

Unlike checkpoints this has no influence on consensus unless you set
it to a block with an invalid history. Because of this it can be
easily be updated without risk of influencing the network consensus.

This results in a massive IBD speedup.

This approach was independently recommended by Peter Todd and Luke-Jr
since POW based signature skipping (see PR#9180) does not have the
verifiable properties of a specific hash and may create bad incentives.

The downside is that, like checkpoints, the defaults bitrot and older
releases will sync slower. On the plus side users can provide their
own value here, and if they set it to something crazy all that will
happen is more time will be spend validating signatures.

Checkblocks and checklevel are also moved to the hidden debug options:
Especially now that checkblocks has a low default there is little need
to change these settings, and users frequently misunderstand them as
influencing security or IBD speed. By hiding them we offset the
space added by this new option.

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 6, 2017

par=4 chainstate reindex with dbcache 2000 goes from 6.15 hours to 2.96 hours.

@petertodd I know you had a reservation about keeping the default in chainparams but we also have a number of other similar heuristics there (such as bip34 height, total work in the best chain, various flags). I'd suggest in the future we should move those more policy like things into their own section of chain params.

I'd rather do that as a separate refactor later than spread this setting out away from the other ones that need to get changed as part of the review process.

@gmaxwell gmaxwell force-pushed the gmaxwell:script_elide_verified branch Jan 7, 2017

@fanquake fanquake added the Validation label Jan 7, 2017

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

Concept ACK. I like this approach.

@@ -13,6 +13,11 @@ Before every minor and major release:
* Update version in sources (see below)
* Write release notes (see below)
* Update `src/chainparams.cpp` nMinimumChainWork with information from the getblockchaininfo rpc.
* Update `src/chainparams.cpp` defaultAssumeValid with information from the getblockhash rpc.
- The selected value must not be orphaned so it may be useful to set the value two blocks back from the tip.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

I assume when you saiy "set the value two blocks back" you mean "set the value two blocks back from the tip at around the time rcs begin, so that it is a month back when release actually happens"?

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 7, 2017

Author Member

I don't intend for it to be a month back at the release. There is no reason to set it back at all except that if the block falls out of the chain the speedup will be lost. Another possiblity would be to advise setting it at the tip with a note that it must be fixed if the block falls out of the chain.

I also don't see a reason that this couldn't be updated at the last RC or even between an RC and a release. In general we should set it as far forward as possible... there is no benefit in setting it back, and the more forward it is, the longer it takes for the cached value to rot.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

OK. Fair enough, though I disagree that it should be allowed to change pre-release post-rc...for a parameter like this we really want lots of eyes and a few days of letting people reindex prior to release.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 7, 2017

Member

There is no reason to set it back at all except that if the block falls out of the chain the speedup will be lost.

Note we could make it smart enough to see the common parent block, but I'm not sure it's worth the additional effort.

I also don't see a reason that this couldn't be updated at the last RC or even between an RC and a release. In general we should set it as far forward as possible...

If we're setting it last minute, I do think more than 2 blocks back would be best. Maybe 10.

src/validation.cpp Outdated
// it hard to hide the implication of the demand. This also avoids having release candidates
// that are hardly doing any signature verification at all in testing without having to
// artificially set the default assumed verified block further back.
fScriptChecks = (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, chainparams.GetConsensus()) < 60 * 60 * 24 * 7);

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Is there any meaningful difference here between a month and a week? (let the bikeshedding begin!).

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 7, 2017

Author Member

I don't think so. I picked a week here because (1) that is the minimum time in our process between an RC and a release and (2) I think two days is likely sufficient for the purpose in the comment-- enough time for a widespread public response.

But if people preferred a month, that would be okay too. I was half expecting this test to get bikesheded out rather than cranked up.

One downside of a longer value is that users on really slow devices who might put a very current value in from another node of theirs would get a diminished synctime improvement.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Hum, at the risk of overdesign, I'd be ok with only a week or so if the user explicitly specified a trust anchor, but for default-release, I'm afraid a week isnt all that long in the context of relying on widespread public outcry to prevent failures.

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 8, 2017

Author Member

You're concerned about it with respect to the default? I don't think that makes sense: It's correct or it isn't and stepping a few blocks back there doesn't make it more correct. If not for the extra complexity and the benefit that block validation gets better tested in the RC cycle I would have exempted the default from this test.

If the review process won't catch an invalid block in the chain-- a simple comparison with a constant-- how is it going to catch the invalidity being simply made valid by indirect and subtle changes to the consensus behavior?

Petertodd's argument for this approach was that the review effort for these values is trivial compared to any of the rest of the software, making it safe and easy to update them with a minimum of additional scrutiny.

@luke-jr

Note we could make it smart enough to see the common parent block, but I'm not sure it's worth the additional effort.

We could ship a tool that performed the entire set of updates for a release, it would be pretty easy to have such a tool use chain-tips to go back further if there is a competing fork. Similarly, the tool could also be used to check, like we have for signed commits. (though if we do that I think it should be a separate PR.)

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

I personally don't like this part. I think it's simpler to just "artificially set the default assumed verified block further back", don't see that as a big deal. But no strong opinion, if that's a concern, the solution seems good.

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

Micro-nit: regardless of the value chosen, 60 * 60 * 24 * 7 could use a const or #define.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 7, 2017

if they set it to something crazy all that will happen is more time will be spend validating signatures.

That's not the worst-case scenario. They could set it to a block with invalid scripts in the history, which would break from consensus. I do think this is an acceptable risk, but maybe should get at least some warning.

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 8, 2017

@luke-jr yes but only if it is buried by a week (in current code), which is the reason for that additional test. The logprint that it's assuming valid was my warning.. but if you'd like to suggest something more...

@gmaxwell gmaxwell force-pushed the gmaxwell:script_elide_verified branch Jan 8, 2017

@@ -920,6 +921,12 @@ bool AppInitParameterInteraction()
fCheckBlockIndex = GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());
fCheckpointsEnabled = GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED);

hashAssumeValid = uint256S(GetArg("-assumevalid", chainparams.GetConsensus().defaultAssumeValid.GetHex()));

This comment has been minimized.

Copy link
@robmcl4

robmcl4 Jan 9, 2017

Contributor

This seems to result in an out-of-bounds memory access when -assumevalid= (with no value) is supplied. uint256S(..) invokes base_blob::SetHex(const char *), which assumes a supplied length of at least 1. An out-of-bounds access then occurs at src/uint256.cpp:39.

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 9, 2017

Author Member

GetArg returns a std:string, base_blob::SetHex(const std::string& str) invokes c_str() on the pinput and calls the c_str argumented version (which is null terminated). On a zero length input the input array with be pointer to a null byte. This will fail the psz[0] == '0' (note the quotes). No out of bounds access will occur on line 39 (nor below). What am I misreading?

This comment has been minimized.

Copy link
@robmcl4

robmcl4 Jan 9, 2017

Contributor

Oh of course, the short-circuit eval there slipped my mind. Never mind, all good!

doc/release-notes.md Outdated
knew the history of a given block were valid it could skip checking scripts
for its ancestors.

- A new configuration option 'assumevalid' is provided to express this knoweldge

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

knowledge

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

fixed.

src/init.cpp Outdated
@@ -329,8 +329,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-blocknotify=<cmd>", _("Execute command when the best block changes (%s in cmd is replaced by block hash)"));
if (showDebug)
strUsage += HelpMessageOpt("-blocksonly", strprintf(_("Whether to operate in a blocks only mode (default: %u)"), DEFAULT_BLOCKSONLY));
strUsage += HelpMessageOpt("-checkblocks=<n>", strprintf(_("How many blocks to check at startup (default: %u, 0 = all)"), DEFAULT_CHECKBLOCKS));
strUsage += HelpMessageOpt("-checklevel=<n>", strprintf(_("How thorough the block verification of -checkblocks is (0-4, default: %u)"), DEFAULT_CHECKLEVEL));
strUsage +=HelpMessageOpt("-assumevalid=<hex>", strprintf(_("If this block is in the chain _assume_ that it and its ancestors are valid and potentially skip their script verification (0 to verify all, default: %s, testnet: %s)"), Params(CBaseChainParams::MAIN).GetConsensus().defaultAssumeValid.GetHex(), Params(CBaseChainParams::TESTNET).GetConsensus().defaultAssumeValid.GetHex()));

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

Do we use _text_ anywhere else in the help output? It seems a bit strange.

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

removed.

src/validation.cpp Outdated
// it hard to hide the implication of the demand. This also avoids having release candidates
// that are hardly doing any signature verification at all in testing without having to
// artificially set the default assumed verified block further back.
fScriptChecks = (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, chainparams.GetConsensus()) < 60 * 60 * 24 * 7);

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

This seems weird. You're checking

  • Whether the to-be-connected block is an ancestor of the assumed-valid block.
  • Whether a specific headers chain is at least a week ahead of the to-be-connected block.

There is no guarantee however that pindexBestHeader descends from the to-be-connected block, so what is the relevance of it here?

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

Okay, fixed, I also require the block to be an ancestor of the BestHeader.

src/validation.h Outdated
@@ -185,6 +185,9 @@ extern CAmount maxTxFee;
extern int64_t nMaxTipAge;
extern bool fEnableReplacement;

/** Block hash whos ancestors we will assume to have valid scripts without checking them. */

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

whose

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

fixed.

doc/release-notes.md Outdated
Introduction of assumed-valid blocks
-------------------------------------

- A signfigant portion of the initial block download time is spent verifying

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

significant

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

fixed.

src/chainparams.cpp Outdated
@@ -99,6 +99,9 @@ class CMainParams : public CChainParams {
// The best chain should have at least this much work.
consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000002cb971dd56d1c583c20f90");

// By default assume that the signatures in ancestors of this block are valid.
consensus.defaultAssumeValid = uint256S("0x0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2");

This comment has been minimized.

Copy link
@sipa

sipa Jan 9, 2017

Member

Maybe list the height/time of that block in a comment?

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 10, 2017

Author Member

added height. I'm trying to not make the update procedure too much work.

@gmaxwell gmaxwell force-pushed the gmaxwell:script_elide_verified branch 2 times, most recently Jan 10, 2017

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 11, 2017

utACK 739983db5dd799424738bc752eee7721fc94f1fb

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Jan 12, 2017

utACK 739983db5dd799424738bc752eee7721fc94f1fb. I like the approach as well.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 12, 2017

Looks good to me. I'm going to try to write a test case.

One comment: please remove or update the comment in CheckInputs() (validation.cpp, line 1392):

        // Skip ECDSA signature verification when connecting blocks before the
        // last block chain checkpoint. Assuming the checkpoints are valid this
        // is safe because block merkle hashes are still computed and checked,
        // and any change will be caught at the next checkpoint. Of course, if
        // the checkpoint is for a chain that's invalid due to false scriptSigs
        // this optimization would allow an invalid chain to be accepted.
@@ -62,6 +62,7 @@ struct Params {
int64_t nPowTargetTimespan;
int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
uint256 nMinimumChainWork;
uint256 defaultAssumeValid;

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

This can be in CChainParams instead of Consensus::Params, which is the consensus "section of chainparams".

to the software. Unlike the 'checkpoints' in the past this setting does not
force the use of a particular chain: chains that are consistent with it are
processed quicker, but other chains are still accepted if they'd otherwise
be chosen as best. Also unlike 'checkpoints' the user can configure which

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

unlike with 'checkpoints'?

- Because the validity of a chain history is a simple objective fact it is much
easier to review this setting. As a result the software ships with a default
value adjusted to match the current chain shortly before release. The use
of this default value can be disabled by setting -assumevalid=0

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

Perhaps add "To avoid releasing software with a 'too recent' default 'assumevalid', checks will never be skipped for blocks that are only a week or less away from the most-work known tip.". Or something of the sort.

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

Nah, I'm now convinced about using the mechanism, but not "To avoid releasing software with a 'too recent' default 'assumevalid'", rather "To avoid using being fooled with too recent invalid 'assumevalid'".

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 13, 2017

Author Member

I don't think the fine details of the implementation are of general interest for the release nodes. If people think this is less secure than it is -- thats not the end of the world, and a separate FAQ would be fine if there is confusion/concern.

src/validation.cpp Outdated
// effectively caching the result of part of the verification.
BlockMap::const_iterator it = mapBlockIndex.find(hashAssumeValid);
if (it != mapBlockIndex.end()) {
if (it->second->GetAncestor(pindex->nHeight) == pindex && pindexBestHeader->GetAncestor(pindex->nHeight) == pindex) {

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 12, 2017

Member

pindexBestHeader->GetAncestor(pindex->nHeight) == pindex could be moved to first if. Maybe with no gain and perhaps this is clearer.

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 13, 2017

Author Member

That test should pretty much never fail, so it runs later to avoid wasting time trying it for blocks past the assumedvalid point.

@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Jan 12, 2017

utACK 739983d modulo small nits that I guess can be fixed later.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 12, 2017

utACK 739983db5dd799424738bc752eee7721fc94f1fb if we change the time for comparison to anything above 2 weeks, though open to further discussion on IRC.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 12, 2017

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 12, 2017

Actually I vote no - we dont need more parameter interactions and if folks are using -checkpoints=0 to avoid trusting the software they're running without reading the changelog (or the realease notes), then they're really not getting what they want. But others might have a different view?

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jan 13, 2017

Tested this with -reindex-chainstate -dbcache=4096 -par=8
Reindex time to 447885 was 2h12m.

@gmaxwell gmaxwell force-pushed the gmaxwell:script_elide_verified branch Jan 13, 2017

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 13, 2017

Updated, added a test against nMinimumChainWork to address some concerns, and moved the depth check to two weeks to satisfy Matt. I think it's overkill (and it would be better to have some different sanity checks) but it could be reduced in the future; and it's still better than we are now.

src/validation.cpp Outdated
// Skip ECDSA signature verification when connecting blocks before the
// last block chain checkpoint. Assuming the checkpoints are valid this
// Skip script verification when connecting blocks under the
// assumedvalid block. Assuming the that block is valid this

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 13, 2017

Member

nit: s/the that/that the

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Jan 13, 2017

Author Member

fixed

Introduce assumevalid setting to skip presumed valid scripts.
This disentangles the script validation skipping from checkpoints.

A new option is introduced "assumevalid" which specifies a block whos
 ancestors we assume all have valid scriptsigs and so we do not check
 them when they are also burried under the best header by two weeks
 worth of work.

Unlike checkpoints this has no influence on consensus unless you set
 it to a block with an invalid history.  Because of this it can be
 easily be updated without risk of influencing the network consensus.

This results in a massive IBD speedup.

This approach was independently recommended by Peter Todd and Luke-Jr
 since POW based signature skipping (see PR#9180) does not have the
 verifiable properties of a specific hash and may create bad incentives.

The downside is that, like checkpoints, the defaults bitrot and older
 releases will sync slower.  On the plus side users can provide their
 own value here, and if they set it to something crazy all that will
 happen is more time will be spend validating signatures.

Checkblocks and checklevel are also moved to the hidden debug options:
 Especially now that checkblocks has a low default there is little need
 to change these settings, and users frequently misunderstand them as
 influencing security or IBD speed.  By hiding them we offset the
 space added by this new option.

@gmaxwell gmaxwell force-pushed the gmaxwell:script_elide_verified branch to e440ac7 Jan 13, 2017

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jan 13, 2017

utACK e440ac7

Travis may need a kick

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 13, 2017

utACK e440ac7

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Jan 13, 2017

utACK e440ac7 , will also try to find time to test

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 13, 2017

tested ACK.

I've written a new qa testcase that exercises the assumevalid parameter here: jnewbery@fffb047

Tests:

  • without assumevalid we reject a chain containing a transaction with an invalid signature
  • with assumevalid we reject a chain containing a transaction with an invalid signature where the invalid signature is buried by less than two weeks' blocks
  • with assumevalid we accept a chain containing a transaction with an invalid signature where the invalid signature is buried by more than two weeks' blocks
@@ -99,6 +99,9 @@ class CMainParams : public CChainParams {
// The best chain should have at least this much work.
consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000002cb971dd56d1c583c20f90");

// By default assume that the signatures in ancestors of this block are valid.
consensus.defaultAssumeValid = uint256S("0x0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2"); //447235

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jan 13, 2017

Member

ACK 0x0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2

This comment has been minimized.

Copy link
@sipa

sipa Jan 15, 2017

Member

ACK 0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2

This comment has been minimized.

Copy link
@instagibbs

instagibbs Jan 16, 2017

Member

ACK 0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2

This comment has been minimized.

Copy link
@theuni

theuni Jan 16, 2017

Member

post-merge ACK 0000000000000000030abc968e1bd635736e880b946085c93152969b9a81a6e2

This comment has been minimized.

Copy link
@rebroad

rebroad Jan 18, 2017

Contributor

ACK the block, but NACK making this on by default. I love the feature, but I think there needs to be express consent by the user to avoid setting a potentially dangerous precedent. Also, it would be nice to allow the user to stipulate their own block by which to do this - perhaps they trust a particular node on the network and would based their block chosen on that.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 21, 2017

Member

@rebroad This is possible, please read the docs. Also, if you are unsure about the block hash, you can always set -noassumevalid

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Jan 14, 2017

Edit: ACK.

@fanquake fanquake added this to the 0.14.0 milestone Jan 14, 2017

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 14, 2017

Please include @jnewbery's tests.

Add assumevalid testcase
Adds a qa testcase testing the new "-assumevalid" option. The testcase builds
a chain that includes and invalid signature for one of the transactions and
sends that chain to three nodes:

 - node0 has no -assumevalid parameter and rejects the invalid chain.
 - node1 has -assumevalid set and accepts the invalid chain.
 - node2 has -assumevalid set but the invalid block is not buried deep
   enough to assume invalid, and so rejects the invalid chain.
@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 14, 2017

Neat tests, I was surprised at how small it is for what it does.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 15, 2017

utACK 7b5e3fe

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Jan 15, 2017

did some additional light testing... still ACK

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 16, 2017

Tested a full reindex together with #9512 with asan/lsan/ubsan, all good.

@sipa sipa merged commit 7b5e3fe into bitcoin:master Jan 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jan 16, 2017

Merge #9484: Introduce assumevalid setting to skip validation presume…
…d valid scripts.

7b5e3fe Add assumevalid testcase (John Newbery)
e440ac7 Introduce assumevalid setting to skip presumed valid scripts. (Gregory Maxwell)
@btcdrak

This comment has been minimized.

Copy link
Member

btcdrak commented Jan 17, 2017

utACK 7b5e3fe

@@ -49,7 +49,7 @@ Low-level RPC changes
than two arguments.

Removal of Priority Estimation
------------------------------
-------------------------------

This comment has been minimized.

Copy link
@rebroad

rebroad Jan 18, 2017

Contributor

?

@@ -1389,11 +1390,10 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
// Only if ALL inputs pass do we perform expensive ECDSA signature checks.
// Helps prevent CPU exhaustion attacks.

// Skip ECDSA signature verification when connecting blocks before the

This comment has been minimized.

Copy link
@rebroad

rebroad Jan 18, 2017

Contributor

no longer ECDSA?

This comment has been minimized.

Copy link
@sipa

sipa Jan 21, 2017

Member

Script verification includes ECDSA verification, but does much more.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 21, 2017

@rebroad The reasoning is that people are already relying on peer review to make sure the consensus logic isn't being changed (which could also lead to invalid blocks being accepted), and that is much harder than checking whether a single block hash is part of a valid chain. This is different from chrckpoints, which do have an effect beyond skipping validation, and need much more careful choosing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.