Cache segwit signature hash components inside CTransaction to optimize validation performance

sdaftuar commented at 9:00 pm on February 6, 2017: member

Now that CTransactions are typically created once and passed around as shared pointers, and blocks are frequently constructed from the transactions in our mempool (thanks to BIP 152 compact blocks), we can calculate the PrecomputedTransactionData once and save it in the CTransaction, to avoid doing extra work in ConnectBlock.

This PR adds a new deserialization flag to allow for deserializing a CTransaction without calculating these hashes, which is used in ProcessGetData.

Finally, this PR includes a commit which caches the witness hash inside CTransaction. @jl2012 This was inspired by #9572, although with the approach I’ve taken here, I don’t think it’s necessary to additionally skip the hashing for non-segwit transactions, as very few hashes are typically calculated during block validation now (just the ones that are requested via getblocktxn).

Thanks to @TheBlueMatt and @ryanofsky for reviewing several iterations of this.

Move PrecomputedTransactionData to CTransaction

Now that CTransaction's are typically created once and passed around as
shared pointers, and blocks are frequently constructed from the transactions
in our mempool (after BIP 152/compact blocks), we can calculate the
PrecomputedTransactionData once and save it in the CTransaction, to avoid
extra work during ConnectBlock.

When processing block messages with loose transactions (eg not reconstructed
from the mempool), this should have no additional overhead.

As a side effect, wallet signing code now uses precomputed hashes rather than
recalculating from scratch each time.

e4554eab49

Access segwit hashes through CTransaction accessors 98e584637d

Cache construction can be controlled via (de-)serialization flags f161ce213e

Add debug print if caches aren't ready during validation fa90899db9

Cache witness hash (wtxid) inside CTransaction 5a6e14372b

sipa commented at 9:03 pm on February 6, 2017: member

I strongly dislike moving validation-specific logic into CTransaction (and into primitives/* in general). Wouldn’t it be possible to creating a validation-specific wrapper around CTransaction, and then perhaps change CTransactionRef to point to that wrapper?

TheBlueMatt commented at 9:09 pm on February 6, 2017: member

@sipa I tend to disagree pretty strongly. I know you prefer primitives/* to be very dumb-data-storage objects, but since CTransaction is const, it would be quite a waste to not use that to store any precomputed thing we can in it. @sdaftuar had a previous version of this patch that made CTransactionRef a wrapper around a shared_ptr to the CTransaction as well as the precomputed data, but that, too, goes in primitives and ended up just being more complication for no reason.

Anyway, we’re already halfway between “dumb storage” and “precalculated” anyway, since we precalculate the hash and store that. The distinction of not precalculating the witness hashes seems arbitrary to me.

sipa commented at 9:20 pm on February 6, 2017: member

Ideally, there should be a BlockValidationContext class or something that can cache all things necessary for validation. I understand that’s a non-trivial refactor now, but I do think it should be the goal to even have the hash out of CTransaction…

TheBlueMatt commented at 9:24 pm on February 6, 2017: member

I think that would generate massive layer violations - you would need to use that to pre-compute data in wallet, net, mempool-acceptance, etc.

sipa commented at 9:26 pm on February 6, 2017: member

At least let’s not make the situation worse with data that is completely specific to validation.

TheBlueMatt commented at 9:28 pm on February 6, 2017: member

I think you missed my point - I dont believe there is another route to go down (besides, possibly, creating a wrapper around CTransaction[Ref] which is what is actually used everywhere) to get this kind of win.

We can create a src/validation_primitives/transaction.h, but then literally nothing would use the src/primitives/transaction.h version.

sipa commented at 9:30 pm on February 6, 2017: member

Of course there are alternatives. They may need a lot of refactoring to do cleanly, but it feels so incredibly wrong to burden the data structure with things that only validation cares about. NACK.

TheBlueMatt commented at 9:31 pm on February 6, 2017: member

@sipa An alternative like creating a validation context is probably more layer-violating than this…you need to calculate this data in wallet, and net (ie everywhere), so putting it anywhere else means layer violations like crazy.

sipa commented at 9:32 pm on February 6, 2017: member

Net doesn’t need the signature validation data. The wallet can use it, but it hardly matters.

dcousens commented at 1:04 am on February 7, 2017: contributor

concept NACK for all the reasons @sipa pointed out.

I dont believe there is another route to go down (besides, possibly, creating a wrapper around CTransaction[Ref] which is what is actually used everywhere) to get this kind of win.

Out of interest, what is the magnitude of this win?

fanquake added the label Validation on Feb 7, 2017

sdaftuar commented at 3:19 pm on February 7, 2017: member

@sipa Sorry I missed the discussion yesterday on this; I do have an alternate approach that changes CTransactionRef to point to a wrapper around CTransaction, which I’ll go ahead and PR for discussion. I ran into some issues with that approach that might also make it objectionable, but maybe there are ways to better solve those problems that would make it workable. @dcousens Regarding the performance gain: I’ve been running some benchmarks on my workstation, and over the last 100 blocks, it looks like this speeds up validation by about 7%.

sdaftuar closed this on Feb 7, 2017

MarcoFalke locked this on Sep 8, 2021

Cache segwit signature hash components inside CTransaction to optimize validation performance #9700