Optimize block validation code and block propagation speed #5835
Conversation
|
My recollection was that the prior code was at the level it was at to avoid a DOS attack where you have a transaction with 1000 valid signatures, and one invalid one that you repeat over and over again? Is the thinking that the DoS stuff is now adequate to protect against that? Any idea from profiling where all that time is going, absent the cache? |
|
@gmaxwell : From an email I sent to Sergio earlier today to see if he can think of an attack from getting rid of the low-level signature cache: It is easy for an attacker to force a CHECKSIG cache miss; they can just use CODESEPARATOR (at the cost of an extra byte in whatever Script they're using to try to CPU DoS). Free-floating p2sh transactions are limited to 15 sigops by AreInputsStandard(), so an attacker can't CPU exhaust the transaction validation code. There is an economic disincentive for attackers to create valid-POW but very-expensive-to-validate blocks (those blocks will propagate very slowly), and, again, the sigops-per-block limit will also mitigate possible attacks. |
|
WRT POW, I was worrying about the lose transaction case, not attacks in blocks. The attack is the attacker has 1000 valid signatures, 1 invalid signature; the time is wasted on the valid signatures; the invalid is just there to avoid paying fees. I don't see a way to cheaply invalidate the cache on the valid signatures. The P2SH limit is per signature, I'm pretty sure you can get1000 checksigs in an IsStandard transaction, you'd just use 66 inputs with 15 checksigs each. With unusual script pubkeys (op_dup-ing the signatures) such a transaction would only be under15kb I'd guess. [I think at worst this might just mean having a reason to keep a plain signature cache around. (Also) caching at a higher level makes sense if it saves that much time; though I'm surprised at that!] |
|
The 1,000-valid-plus-one-invalid attack will get the attacker DoS-banned for sending an invalid transaction. They would have to Sybil and reconnect to get around that with either the old or new code; with the old code, they would have to pre-generate more valid signatures than the size of the signature cache AND Sybil to succeed in wasting CPU time. You're right, the new code makes the attack slightly easier. But I think getting rid of the low-level cache is the right thing to do just because of the memory savings. |
|
Hm. So the caching isn't parametrized by the state of CCoinsview cache and validity can be conditional on it, but it's not clear to me that it matters. What happens with this sequence of events? Mempool contains transaction B that spends input X. It would be good to have a test there, I could see this invariant being broken very easily. |
|
It appears that most of the performance comes from avoiding the /seemingly/ redundant interrogation of the CCoinsViewCache in CheckInputs: Results in going from "Verify 1730 txins: 187.01ms (0.108ms/txin)" to "Verify 1730 txins: 13.29ms (0.008ms/txin)" I've not yet convinced myself that its actually redundant, but if it is, it's good to take it out since it wastes time on mempool acceptance too (where it also appears to be behind an existing HaveInputs test). |
|
@gmaxwell: I agree, a unit and/or regression test for one-double-spend-in-mempool, two-in-a-block is a very good idea. RE: removing possibly redundant HaveInputs() : should be a separate pull request. Thanks for reviewing and looking deeper! |
|
I realized this morning memory savings is even better than I thought: it is an extra 4 bytes per transaction in the mempool (not 32 bytes, I was either thinking of sizeof(txid) or confusing 32-bit-integer with 32-bytes). |
|
hah. I read it as bits to begin with (and was very confused by your most recent comment. |
|
I responded to Gavin, but now that I see this thread, I copy parts here: The sigcache was added for security reasons and, as a by-product, it increases performance. With a TxId cache you can achieve the performance part, but not the security protection. The problem is described here https://bitcointalk.org/index.php?topic=136422.0 and only affects transactions during free forwarding (not in blocks). To summarize the attack, the idea is that the attacker can send transactions that cost very little to create (can even reuse signatures with SIGHASH_ANYONECANPAY) and fail to verify after a lot of CPU has been wasted, but still the victim peer does not increase the DoS score of the attacker. For example, re-using 999 good inputs, but failing in the 1000th one. The attacker can prevent DoS score to be increased because, there are two cases where it's not:
So removing the sigcache has consequences. Regarding performance, take into account that each signature is verified twice in AcceptToMemoryPool(), one with standard flags and another with mandatory flags (for extra security), so if you take out sigcache, then you double signature verification time. Several solutions are possible:
so you will be breaking this protection.
Obviously the best is just adding a second level of cache of TxIds, without changing anything security-related. The cache also should cache the block number where the txId can be included, if the transaction uses locktime (if it's not re-verified, of course). |
|
Gavin, were you able to test the effect of this change on overall runtime? Sergio's observation that signatures are checked twice on entry to the mempool seems like it could plausibly make things slower to take out the signature cache, and I've noticed in my initial testing that when I play back historical data through a node running this pull's code, I am seeing a substantial (nearly 2x) slowdown, although ConnectBlock is about 20% faster. |
|
@sdaftuar: thanks for testing! I'll revert the low-level signature cache removal; it will, indeed, speed up initial download and accept-to-mempool. If this pull is accepted there should be a separate follow-up pull request to make the default signature cache much smaller to save memory; it should only need to be as large as one transaction's worth of signatures, if we're not relying on it to optimize block validation time. |
|
I ran this overnight with -debug=bench to get a sense of how good the assumption of "blocks contain mostly transactions that are already in our memory pool" is, and it turns out that is a very good assumption. The last 11 blocks had ALL transactions except the coinbase already in the mempool. The twelfth had 161 of 170 in the memory pool, and I have to go back another 11 blocks to find one that wasn't all cache hits (it had 3 transactions not in my mempool). |
gavinandresen
force-pushed
the
tx_validation_cache2
branch
from
19cf11e
to
c334e76
Compare
|
Rebased to keep the old signature cache code. @SergioDemianLerner : RE locktime transactions: The memory pool does do the right thing with lockTime transactions (a bugfix went into 0.10.0 for that, if I remember correctly). But even if it didn't, AcceptBlock calls ContextualCheckBlock which makes sure all transactions in the block are final, and those checks are always done for new blocks. |
|
Related #5163: Upon cursory review, still an outstanding optimization. |
|
@gavinandresen I've been running a version of this code overnight, and I'm curious if your results are similar to mine -- I'm typically seeing Verify times above 15 microseconds per txin, eg: which is an improvement, but still an order of magnitude slower than what you originally posted (1 microsecond/txin), so just want to confirm if my observations are consistent with yours? For reference, my 10 fastest verify times out of the last 50 blocks (reported in milliseconds per txin) are: (I manually checked, and all but one of those times correspond to blocks with no cache misses; one of those 0.017ms/txin blocks had 1 cache miss.) @gmaxwell I'm puzzled by your comment earlier about the substantial gains from commenting out the duplicate HaveInputs call in CheckInputs -- I don't see how that could result in the speedups you mentioned, because the Verify time still includes one call to HaveInputs for every transaction in the block. So it seems like it shouldn't be possible to get more than a factor of 2 speedup if the only change were to remove the second call, unless I'm missing something? |
|
@sdaftuar : you are seeing verification times of 0.015 milliseconds per txin (0.015ms really does mean 15-thousandths-of-a-millisecond). Note: I found the -bench numbers in brackets confusing; they are cumulative times (total time spent verifying or connecting or whatever, since startup). I should have time to add a double-spend-in-block, one-tx-in-mempool unit test for this tomorrow. |
|
@gavinandresen: Agreed, 0.015 milliseconds/txin. To clarify, I was trying to make the point that the 0.001ms/txin that you reported at the beginning of this conversation is not consistent with what I was seeing, even when I restrict my samples to the very fastest blocks. I've done some more testing and I think the 60x speedup is way off for the performance benefit of this change, even in the case where there are no cache misses. I've tested this code and timed the calls in ConnectBlock which occur in the normal course of a node doing block validation, and this code looks to be roughly 20% faster for verifying blocks (even in the case of no cache misses). Of course I'm not trying to suggest a 20% improvement isn't a nice speedup, I just want to make sure we're all on the same page about what kind of speedup we're expecting! |
|
Updated with new unit test as suggested by @gmaxwell -- with the new unit test, Travis passing, and keeping the old sigcache I think I've addressed all concerns. @sdaftuar : can you email me a longer snippet of your debug.log, showing all the -debug=bench times? And the head of config.log? (you're not benchmarking with -g or -DDEBUG_LOCKORDER, are you?) |
|
@sdaftuar : never mind, I'm seeing similar results outside my benchmarking code. From a profile, it looks like adding a transaction to the memory pool isn't making UTXOs "stick" in the CCoinsViewCache for some reason, so the on-disk database is being hit during ProcessNewBlock. |
The code for limitedmap::insert would leave an entry in rmap with no corresponding entry in the forward map if the map was full and the entry being inserted had the lowest value (so was immediately erased). It also had an off-by-one error; setting max_size to 11 results in a map with room for only 10 entries. Neither of these bugs matter for current use of limitedmap; I found them writing a unit test for new transaction validation caching code.
Store validation flags along with transactions in the memory pool, and when validating a block, skip full validation if the transaction is in the pool and was validated with appropriate SCRIPT_VERIFY_ flags. This significantly speeds up block verification, and, therefore, block propagation.
gavinandresen
force-pushed
the
tx_validation_cache2
branch
2 times, most recently
from
4439250
to
f14a7ae
Compare
|
F.I.Y: i'm getting some new warnings while compiling this: Will test and try to time-profile/compare with current master. |
As suggested by Greg Maxwell-- unit test to make sure a block with a double-spend in it doesn't pass validation if half of the double-spend is already in the memory pool (so full-blown transaction validation is skipped) when the block is received.
gavinandresen
force-pushed
the
tx_validation_cache2
branch
from
f14a7ae
to
2fba2c5
Compare
|
@jonasschnelli : thanks for testing. I fixed the new warning (and switched back to compiling my tree with clang++, so I'll catch them myself next time). |
|
Did run a mainnet node over night with this PR on top. Last 10 blocks: Processor info: Now i switched back to the master and will report again the bench of next 10 blocks. |
|
current master bench (8 recent blocks): |
|
Concept ACK, though I would prefer keeping the cache (which becomes consensus-critical) outside of the mempool code. @SergioDemianLerner would the security concern not also be addressed by a simple per-txin-verification cache (which wouldn't need synchronization across validation threads, etc). |
|
@sipa No, but I don't know if I understood you correctly. The attack uses multiple txs, so the cache at the txin level wouldn't help. Let's try to tackle the contention problem... One solution could be a cache data structure where reads use only a lightweight mutex (such as a per-thread incremental variable), and writes must be delayed until all reads have been performed (using a spin-lock over all the per-thread mutexes). If the bottleneck is block processing, then why not acquire a lock to the cache before processing the block, and delay all writes into thread specific caches until the full block has been processed ? Then all per-thread caches are merged back into the main cache. Then contention is eliminated, at the expense of some kind weakness to a DoS where the same signature is used again and again for all transactions (e.g. using SIGHASH_SINGLE bug). Maybe we could add a special mutex protected cache only for SIGHASH_SINGLE signatures where an input that has an index bigger than the maximum output index. |
|
@SergioDemianLerner Or a per-verification-thread cache? |
|
Closing, @sipa 's version is better (and just got merged). |
I was running some benchmarks on 20MB blocks, and was surprised at how long block validation took even when all transactions had already been seen and were in the signature cache.
So I moved caching up from being a low-level signature cache to a high-level "have we already validated this txid."
Benchmark results on a recent 365K block with all transactions already in the memory pool / signature cache:
Old code: (results from running with -debug=bench and hacked code to make sure all txn pre-validated):
- Verify 1839 txins: 191.66ms (0.104ms/txin)
New code:
- Verify 1839 txins: 2.06ms (0.001ms/txin)
Extrapolating to full (1MB) blocks, the old code would spend about 600ms CPU time per hop propagating blocks with already-seen transactions, this new code will spend under 10ms.
Memory usage is about 60 times smaller, also:
Old code: (sizeof(scriptSig)+std::set overhead) * number of txins (about 200K for the 365K block)
New code: an extra 4 bytes per transaction in the mempool (about 3K for the 365K block)
I first wrote this as a standalone CTxValidationCache class, but it is simpler and more efficient to extend the mempool. The second commit in this pull request fixes two bugs in limitedmap I stumbled across when testing that code.
REBASED to keep the old signature cache (see discussion below)