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

ECDSA signature optimization and more DoS prevention #1349

Merged
merged 6 commits into from May 22, 2012

Conversation

Projects
None yet
5 participants
@gavinandresen
Contributor

gavinandresen commented May 18, 2012

These 6 commits:

  1. Implement an ECDSA signature cache, which should cut the amount of CPU time spent verifying signatures in half (because transaction signatures are verified before going into the memory pool and then again when the block is accepted). Please review that commit very carefully, since it is the very heart of transaction security.
  2. Implement several "belt and suspenders" changes to prevent possible denial-of-service attacks involving sending orphan transactions (suggested by Sergio Demian Lerner) or transactions with lots of inputs.

Tested by:

  • writing new unit tests, and stepping through the code in the debugger while running the new tests
  • running the new code under valgrind for more than 24 hours on the main network
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 18, 2012

Member

Blown up by 6d64a0b, after merging by hand it dies because key.cpp doesn't #include <sync.h>. Introduces a harmless looking signed compare warning key.cpp:388:3.

Have it running on a node now.

Member

gmaxwell commented May 18, 2012

Blown up by 6d64a0b, after merging by hand it dies because key.cpp doesn't #include <sync.h>. Introduces a harmless looking signed compare warning key.cpp:388:3.

Have it running on a node now.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik May 18, 2012

Contributor

Code appears correct to me. Comments inline... IMO push those refactors into the tree immediately.

Contributor

jgarzik commented May 18, 2012

Code appears correct to me. Comments inline... IMO push those refactors into the tree immediately.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 18, 2012

Member

@jgarzik if your 'log-worthy event' comment is also on the oversized orphans (like 183 instead of 191), I thought the same thing.

Member

gmaxwell commented May 18, 2012

@jgarzik if your 'log-worthy event' comment is also on the oversized orphans (like 183 instead of 191), I thought the same thing.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik May 18, 2012

Contributor

both, really

Contributor

jgarzik commented May 18, 2012

both, really

Optimize orphan transaction handling
Changes suggested by Sergio Demian Lerner to
help prevent potential DoS attacks.
@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen May 18, 2012

Contributor

@gmaxwell : I want to keep this pull based on 0.6.2 in case we decide we need a 0.6.3.

@jgarzik : ACK, I'll fix the comments. I think encapsulation in a class is overkill.

@SergioDemianLerner : excellent catch on vSpent, and I 100% agree this code needs extremely careful review and as much testing as we can throw at it. I also like your suggestion to only cache valid signatures; it has the added benefit of making the code a lot simpler.

Contributor

gavinandresen commented May 18, 2012

@gmaxwell : I want to keep this pull based on 0.6.2 in case we decide we need a 0.6.3.

@jgarzik : ACK, I'll fix the comments. I think encapsulation in a class is overkill.

@SergioDemianLerner : excellent catch on vSpent, and I 100% agree this code needs extremely careful review and as much testing as we can throw at it. I also like your suggestion to only cache valid signatures; it has the added benefit of making the code a lot simpler.

gavinandresen added some commits May 17, 2012

Remove invalid dependent orphans from memory
Remove orphan transactions from memory once
all of their parent transactions are received
and they're still not valid.
Thanks to Sergio Demian Lerner for suggesting this fix.
Further DoS prevention: Verify signatures last
Loop over all inputs doing inexpensive validity checks first,
and then loop over them a second time doing expensive signature
checks. This helps prevent possible CPU exhaustion attacks
where an attacker tries to make a victim waste time checking
signatures for invalid transactions.
Cache signature verifications
Create a maximum-10MB signature verification result cache.
This should almost double the number of transactions that
can be processed on a given CPU, because before this change
ECDSA signatures were verified when transactions were added
to the memory pool and then again when they appeared in
a block.
@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 18, 2012

It's interesting that cache records will only be removed from the cache when the cache fills up. Can't we remove entries safely before, after a block that verifies the signature is added to the current chain?

Proposed code:

static bool GetValidSigCache(uint256 hash, const std::vector& vchSig, const std::vector& pubKey, bool eraseCacheEntryOnCheck)
{
LOCK(cs_sigcache);
sigdata_type k(hash, vchSig, pubKey);

std::set<sigdata_type>::iterator mi = setValidSigCache.find(k);
if (mi != setValidSigCache.end())
{
if (eraseCacheEntryOnCheck) // two new lines
setValidSigCache.erase(mi);
return true;
}
return false;
}

bool CKey::Verify(uint256 hash, const std::vector& vchSig, bool eraseCacheEntryOnCheck)
{
if (GetValidSigCache(hash, vchSig, GetPubKey(),eraseCacheEntryOnCheck)) // new arg
.... ...
}

And then, in ConnectInputs(), call VerifySignature with a new last argument "fBlock" like this:

if (!VerifySignature(txPrev, *this, i, fStrictPayToScriptHash, 0,fBlock)) // new arg

The drawback is that if the a new chain branch replaces the current chain containing copies of the same transaction, the verification work must be redone.
I think the change is good, but I'm really not sure.

SergioDemianLerner commented on 62922c8 May 18, 2012

It's interesting that cache records will only be removed from the cache when the cache fills up. Can't we remove entries safely before, after a block that verifies the signature is added to the current chain?

Proposed code:

static bool GetValidSigCache(uint256 hash, const std::vector& vchSig, const std::vector& pubKey, bool eraseCacheEntryOnCheck)
{
LOCK(cs_sigcache);
sigdata_type k(hash, vchSig, pubKey);

std::set<sigdata_type>::iterator mi = setValidSigCache.find(k);
if (mi != setValidSigCache.end())
{
if (eraseCacheEntryOnCheck) // two new lines
setValidSigCache.erase(mi);
return true;
}
return false;
}

bool CKey::Verify(uint256 hash, const std::vector& vchSig, bool eraseCacheEntryOnCheck)
{
if (GetValidSigCache(hash, vchSig, GetPubKey(),eraseCacheEntryOnCheck)) // new arg
.... ...
}

And then, in ConnectInputs(), call VerifySignature with a new last argument "fBlock" like this:

if (!VerifySignature(txPrev, *this, i, fStrictPayToScriptHash, 0,fBlock)) // new arg

The drawback is that if the a new chain branch replaces the current chain containing copies of the same transaction, the verification work must be redone.
I think the change is good, but I'm really not sure.

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen May 18, 2012

Owner

One CPU exhaustion attack is to create an OP_CHECKMULTISIG transaction that just duplicates the signature (using OP_DUP) and public key 20 times.

So it is a bad idea to flush the cache entry the first time it is checked as part of a block.

In this age of gigabytes of memory I think a 10 megabyte cache is OK, and letting it grow keeps the code nice and simple.

Owner

gavinandresen replied May 18, 2012

One CPU exhaustion attack is to create an OP_CHECKMULTISIG transaction that just duplicates the signature (using OP_DUP) and public key 20 times.

So it is a bad idea to flush the cache entry the first time it is checked as part of a block.

In this age of gigabytes of memory I think a 10 megabyte cache is OK, and letting it grow keeps the code nice and simple.

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner replied May 18, 2012

Right

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 18, 2012

Why is takes 50 msec to verify 100 signatures that are stored on the cache ?
Shouldn't it be in the order of 1 msec ?
Maybe the timer is not accurate enough?

SergioDemianLerner replied May 18, 2012

Why is takes 50 msec to verify 100 signatures that are stored on the cache ?
Shouldn't it be in the order of 1 msec ?
Maybe the timer is not accurate enough?

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 18, 2012

I spent almost an hour thinking about this code snippet.
I wrote some comments, then I erased them 3 times.
Seems to be ok.

Since we now have the verification cache, and that cache proves to be fast, maybe the safest thing would be not to do anything in this part of the code, remove this pull request and forget about a problem that is now not so important :) I want to stop thinking about this code! I need to go and do my paid work!

SergioDemianLerner commented on 4add41a May 18, 2012

I spent almost an hour thinking about this code snippet.
I wrote some comments, then I erased them 3 times.
Seems to be ok.

Since we now have the verification cache, and that cache proves to be fast, maybe the safest thing would be not to do anything in this part of the code, remove this pull request and forget about a problem that is now not so important :) I want to stop thinking about this code! I need to go and do my paid work!

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 20, 2012

Member

ACK

Member

sipa commented May 20, 2012

ACK

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 21, 2012

Contributor

Gavin: can you estimate the time it takes to verify a signature compared to the time it takes to get the result from the cache? There should be a factor of at least 100x between them, but the test code seems to disagree with this.

Contributor

SergioDemianLerner commented May 21, 2012

Gavin: can you estimate the time it takes to verify a signature compared to the time it takes to get the result from the cache? There should be a factor of at least 100x between them, but the test code seems to disagree with this.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen May 21, 2012

Contributor

@SergioDemianLerner I'll isolate and benchmark the cache unit test to see why cached signatures are taking 50ms to validate. Note that all of the CScript interpreter machinery is still being run (I am testing/measuring at the VerifySignature() level, not a the CKey:: level).

Contributor

gavinandresen commented May 21, 2012

@SergioDemianLerner I'll isolate and benchmark the cache unit test to see why cached signatures are taking 50ms to validate. Note that all of the CScript interpreter machinery is still being run (I am testing/measuring at the VerifySignature() level, not a the CKey:: level).

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen May 22, 2012

Contributor

What I did:

Used valgrind --tool=callgrind and hacked versions of test/DoS_tests.cpp to isolate and measure just the cached signature verification code.

What I found:

Two things slow down cached signature verification:

  1. Decompressing compressed public keys. Using uncompressed public keys almost doubles performance (from 110ms to validate 500 inputs to 60ms). If we need/want to further optimize verification then a compressed->uncompressed cache would get a 25% or so speedup.

  2. The remaining 60ms is dominated by the serializing/hashing done; if we needed/wanted to further optimize this code then having transactions cache their SIGHASH_whatever hashes might be worthwhile so they're not repeatedly recomputed. However, the transaction I'm using for this benchmark is not a typical transaction (it has 100 inputs), and the bottleneck for transaction verification might be somewhere else for more realistic transactions.

Finally, in the future very-high-level optimization schemes for transactions might be implemented; for example, if you have a peer that you can completely trust to give you only valid transactions then you could skip all verifications and take it on faith that all transactions from that peer are valid (with, perhaps, a random 1-in-100 audit to make sure the peer hasn't been corrupted). So spending a lot of time on micro-optimizations may not be the best way forward.

Contributor

gavinandresen commented May 22, 2012

What I did:

Used valgrind --tool=callgrind and hacked versions of test/DoS_tests.cpp to isolate and measure just the cached signature verification code.

What I found:

Two things slow down cached signature verification:

  1. Decompressing compressed public keys. Using uncompressed public keys almost doubles performance (from 110ms to validate 500 inputs to 60ms). If we need/want to further optimize verification then a compressed->uncompressed cache would get a 25% or so speedup.

  2. The remaining 60ms is dominated by the serializing/hashing done; if we needed/wanted to further optimize this code then having transactions cache their SIGHASH_whatever hashes might be worthwhile so they're not repeatedly recomputed. However, the transaction I'm using for this benchmark is not a typical transaction (it has 100 inputs), and the bottleneck for transaction verification might be somewhere else for more realistic transactions.

Finally, in the future very-high-level optimization schemes for transactions might be implemented; for example, if you have a peer that you can completely trust to give you only valid transactions then you could skip all verifications and take it on faith that all transactions from that peer are valid (with, perhaps, a random 1-in-100 audit to make sure the peer hasn't been corrupted). So spending a lot of time on micro-optimizations may not be the best way forward.

@gavinandresen gavinandresen merged commit 62922c8 into bitcoin:master May 22, 2012

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 22, 2012

Contributor

Great! A lot of useful information!
Remember that I had suggested caching (outpoint,script)->bool and not the signature verification (signature,pubkey)->bool to get all possible speedups.

Contributor

SergioDemianLerner commented May 22, 2012

Great! A lot of useful information!
Remember that I had suggested caching (outpoint,script)->bool and not the signature verification (signature,pubkey)->bool to get all possible speedups.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 22, 2012

Member

@SergioDemianLerner Thats actually a bad idea. Otherwise I can just create endless numbers of scripts of the form push $randomnumber pop [normal script] with no computation on my part in order to bypass the cache.

Member

gmaxwell commented May 22, 2012

@SergioDemianLerner Thats actually a bad idea. Otherwise I can just create endless numbers of scripts of the form push $randomnumber pop [normal script] with no computation on my part in order to bypass the cache.

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 22, 2012

Contributor

We're caching only the scripts that verify ok so that kind of attack does not work.

Contributor

SergioDemianLerner commented May 22, 2012

We're caching only the scripts that verify ok so that kind of attack does not work.

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 22, 2012

Contributor

Also I had suggested caching (outpoint,hash(script)) -> bool to reduce memory consumption.

Contributor

SergioDemianLerner commented May 22, 2012

Also I had suggested caching (outpoint,hash(script)) -> bool to reduce memory consumption.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 22, 2012

Member

@SergioDemianLerner PUSH $randomnumber POP {normal script} will also validate if {normal script} would have also validated— but it will have a different hash.

Member

gmaxwell commented May 22, 2012

@SergioDemianLerner PUSH $randomnumber POP {normal script} will also validate if {normal script} would have also validated— but it will have a different hash.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 22, 2012

Member

See follow-up pull #1380.

Member

sipa commented May 22, 2012

See follow-up pull #1380.

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 22, 2012

Contributor

@gmaxwell You are right. Scripts cannot be easily cached. We would need an pushdown automata parser and optimizer to compress scripts and erase all garbage.
It would be interesting to program such an algorithm to allow clients to "standarize" scripts, detect and remove hidden messages while transactions are passed from peer to peer. We may even create a transaction antivirus !! (just joking, I remember transaction signature do not withstands such modifications...)

Contributor

SergioDemianLerner commented May 22, 2012

@gmaxwell You are right. Scripts cannot be easily cached. We would need an pushdown automata parser and optimizer to compress scripts and erase all garbage.
It would be interesting to program such an algorithm to allow clients to "standarize" scripts, detect and remove hidden messages while transactions are passed from peer to peer. We may even create a transaction antivirus !! (just joking, I remember transaction signature do not withstands such modifications...)

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 23, 2012

Contributor

Although IsStandard() check would prevent "PUSH $randomnumber POP {normal script}" scripts from being forwarded, so it may be possible to cache at that level too.

Contributor

SergioDemianLerner commented May 23, 2012

Although IsStandard() check would prevent "PUSH $randomnumber POP {normal script}" scripts from being forwarded, so it may be possible to cache at that level too.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 23, 2012

Member

@SergioDemianLerner I picked a toy example, and besides, we wouldn't want to adopt a design which strongly discouraged expanding IsStandard in the future.

Member

gmaxwell commented May 23, 2012

@SergioDemianLerner I picked a toy example, and besides, we wouldn't want to adopt a design which strongly discouraged expanding IsStandard in the future.

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner May 23, 2012

Contributor

ACK.

Contributor

SergioDemianLerner commented May 23, 2012

ACK.

suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment